Skip to content

Commit 2f1bdb2

Browse files
booradboorad
authored
fix: prevent cipher reuse after final() and reset is_finalized in all init() overrides (#948)
1 parent 7c1d6ba commit 2f1bdb2

12 files changed

Lines changed: 169 additions & 24 deletions

‎example/src/tests/cipher/cipher_tests.ts‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,130 @@ test(SUITE, 'GCM wrong key throws error (issue #798)', () => {
165165
expect(() => decipher.final()).to.throw();
166166
});
167167

168+
// --- String encoding tests (issue #945) ---
169+
170+
test(SUITE, 'Buffer concat vs string concat produce same result', () => {
171+
const testKey = Buffer.from(
172+
'KTnGEDonslhj/qGvf6rj4HSnO32T7dvjAs5PntTDB0s=',
173+
'base64',
174+
);
175+
const testIv = Buffer.from('2pXx2krk1wU8RI6AQjuPUg==', 'base64');
176+
const text = 'this is a test.';
177+
178+
// Buffer concat approach
179+
const cipher1 = createCipheriv('aes-256-cbc', testKey, testIv);
180+
const bufResult = Buffer.concat([
181+
cipher1.update(Buffer.from(text, 'utf8')),
182+
cipher1.final(),
183+
]).toString('base64');
184+
185+
// String concat approach (fresh cipher)
186+
const cipher2 = createCipheriv('aes-256-cbc', testKey, testIv);
187+
const strResult =
188+
cipher2.update(text, 'utf8', 'base64') + cipher2.final('base64');
189+
190+
expect(bufResult).to.equal(strResult);
191+
});
192+
193+
test(SUITE, 'update with hex input and output encoding', () => {
194+
const cipher1 = createCipheriv('aes-128-cbc', key16, iv);
195+
const bufResult = Buffer.concat([
196+
cipher1.update(plaintextBuffer),
197+
cipher1.final(),
198+
]).toString('hex');
199+
200+
const cipher2 = createCipheriv('aes-128-cbc', key16, iv);
201+
const hexResult =
202+
cipher2.update(plaintext, 'utf8', 'hex') + cipher2.final('hex');
203+
204+
expect(bufResult).to.equal(hexResult);
205+
});
206+
207+
test(SUITE, 'update with hex input decryption', () => {
208+
// Encrypt
209+
const cipher = createCipheriv('aes-128-cbc', key16, iv);
210+
const encrypted =
211+
cipher.update(plaintext, 'utf8', 'hex') + cipher.final('hex');
212+
213+
// Decrypt using hex input encoding
214+
const decipher = createDecipheriv('aes-128-cbc', key16, iv);
215+
const decrypted =
216+
decipher.update(encrypted, 'hex', 'utf8') + decipher.final('utf8');
217+
218+
expect(decrypted).to.equal(plaintext);
219+
});
220+
221+
test(SUITE, 'update with hex encoding roundtrip (aes-256-cbc)', () => {
222+
// Encrypt
223+
const cipher = createCipheriv('aes-256-cbc', key32, iv);
224+
const encrypted =
225+
cipher.update(plaintext, 'utf8', 'hex') + cipher.final('hex');
226+
227+
// Decrypt
228+
const decipher = createDecipheriv('aes-256-cbc', key32, iv);
229+
const decrypted =
230+
decipher.update(encrypted, 'hex', 'utf8') + decipher.final('utf8');
231+
232+
expect(decrypted).to.equal(plaintext);
233+
});
234+
235+
// --- Cipher state violation tests ---
236+
237+
test(SUITE, 'update after final throws', () => {
238+
const cipher = createCipheriv('aes-128-cbc', key16, iv);
239+
cipher.update(plaintextBuffer);
240+
cipher.final();
241+
242+
expect(() => cipher.update(plaintextBuffer)).to.throw();
243+
});
244+
245+
test(SUITE, 'final called twice throws', () => {
246+
const cipher = createCipheriv('aes-128-cbc', key16, iv);
247+
cipher.update(plaintextBuffer);
248+
cipher.final();
249+
250+
expect(() => cipher.final()).to.throw();
251+
});
252+
253+
test(SUITE, 'decipher update after final throws', () => {
254+
// First encrypt something
255+
const cipher = createCipheriv('aes-128-cbc', key16, iv);
256+
const encrypted = Buffer.concat([
257+
cipher.update(plaintextBuffer),
258+
cipher.final(),
259+
]);
260+
261+
// Decrypt and then try to reuse
262+
const decipher = createDecipheriv('aes-128-cbc', key16, iv);
263+
decipher.update(encrypted);
264+
decipher.final();
265+
266+
expect(() => decipher.update(encrypted)).to.throw();
267+
});
268+
269+
test(SUITE, 'cipher works after re-init (createCipheriv)', () => {
270+
// First use
271+
const cipher1 = createCipheriv('aes-128-cbc', key16, iv);
272+
const enc1 = Buffer.concat([
273+
cipher1.update(plaintextBuffer),
274+
cipher1.final(),
275+
]);
276+
277+
// Second use with same params should produce identical result
278+
const cipher2 = createCipheriv('aes-128-cbc', key16, iv);
279+
const enc2 = Buffer.concat([
280+
cipher2.update(plaintextBuffer),
281+
cipher2.final(),
282+
]);
283+
284+
expect(enc1.toString('hex')).to.equal(enc2.toString('hex'));
285+
286+
// Verify decryption still works
287+
const decipher = createDecipheriv('aes-128-cbc', key16, iv);
288+
const decrypted = Buffer.concat([decipher.update(enc2), decipher.final()]);
289+
expect(decrypted.toString('utf8')).to.equal(plaintext);
290+
});
291+
168292
test(SUITE, 'GCM tampered ciphertext throws error', () => {
169293
const testKey = Buffer.from(randomFillSync(new Uint8Array(32)));
170294
const testIv = randomFillSync(new Uint8Array(12));

‎packages/react-native-quick-crypto/cpp/cipher/CCMCipher.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ void CCMCipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const std::s
5454

5555
std::shared_ptr<ArrayBuffer> CCMCipher::update(const std::shared_ptr<ArrayBuffer>& data) {
5656
checkCtx();
57+
checkNotFinalized();
5758
auto native_data = ToNativeArrayBuffer(data);
5859
size_t in_len = native_data->size();
5960
if (in_len < 0 || in_len > INT_MAX) {
@@ -104,10 +105,11 @@ std::shared_ptr<ArrayBuffer> CCMCipher::update(const std::shared_ptr<ArrayBuffer
104105

105106
std::shared_ptr<ArrayBuffer> CCMCipher::final() {
106107
checkCtx();
108+
checkNotFinalized();
107109

108110
// CCM decryption does not use final. Verification happens in the last update call.
109111
if (!is_cipher) {
110-
// Return an empty buffer, matching Node.js behavior
112+
is_finalized = true;
111113
unsigned char* empty_output = new unsigned char[0];
112114
return std::make_shared<NativeArrayBuffer>(empty_output, 0, [=]() { delete[] empty_output; });
113115
}
@@ -138,6 +140,7 @@ std::shared_ptr<ArrayBuffer> CCMCipher::final() {
138140
throw std::runtime_error("Failed to get auth tag after finalization: " + std::string(err_buf));
139141
}
140142
auth_tag_state = kAuthTagKnown;
143+
is_finalized = true;
141144

142145
unsigned char* final_output = out_buf.release();
143146
return std::make_shared<NativeArrayBuffer>(final_output, out_len, [=]() { delete[] final_output; });

‎packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ void ChaCha20Cipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const s
6464

6565
std::shared_ptr<ArrayBuffer> ChaCha20Cipher::update(const std::shared_ptr<ArrayBuffer>& data) {
6666
checkCtx();
67+
checkNotFinalized();
6768
auto native_data = ToNativeArrayBuffer(data);
6869
size_t in_len = native_data->size();
6970
if (in_len > INT_MAX) {
@@ -89,7 +90,8 @@ std::shared_ptr<ArrayBuffer> ChaCha20Cipher::update(const std::shared_ptr<ArrayB
8990

9091
std::shared_ptr<ArrayBuffer> ChaCha20Cipher::final() {
9192
checkCtx();
92-
// For ChaCha20, final() should return an empty buffer since it's a stream cipher
93+
checkNotFinalized();
94+
is_finalized = true;
9395
unsigned char* empty_output = new unsigned char[0];
9496
return std::make_shared<NativeArrayBuffer>(empty_output, 0, [=]() { delete[] empty_output; });
9597
}

‎packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,12 @@ void ChaCha20Poly1305Cipher::init(const std::shared_ptr<ArrayBuffer> cipher_key,
6060
ctx = nullptr;
6161
throw std::runtime_error("ChaCha20Poly1305Cipher: Failed to set key/IV: " + std::string(err_buf));
6262
}
63-
64-
// Reset final_called flag
65-
final_called = false;
63+
is_finalized = false;
6664
}
6765

6866
std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::update(const std::shared_ptr<ArrayBuffer>& data) {
6967
checkCtx();
68+
checkNotFinalized();
7069
auto native_data = ToNativeArrayBuffer(data);
7170
size_t in_len = native_data->size();
7271
if (in_len > INT_MAX) {
@@ -92,6 +91,7 @@ std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::update(const std::shared_pt
9291

9392
std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::final() {
9493
checkCtx();
94+
checkNotFinalized();
9595

9696
// For ChaCha20-Poly1305, we need to call final to generate the tag
9797
int out_len = 0;
@@ -105,7 +105,7 @@ std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::final() {
105105
throw std::runtime_error("ChaCha20Poly1305Cipher: Failed to finalize: " + std::string(err_buf));
106106
}
107107

108-
final_called = true;
108+
is_finalized = true;
109109
return std::make_shared<NativeArrayBuffer>(out, out_len, [=]() { delete[] out; });
110110
}
111111

@@ -130,7 +130,7 @@ std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::getAuthTag() {
130130
if (!is_cipher) {
131131
throw std::runtime_error("getAuthTag can only be called during encryption");
132132
}
133-
if (!final_called) {
133+
if (!is_finalized) {
134134
throw std::runtime_error("getAuthTag must be called after final()");
135135
}
136136

‎packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.hpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ namespace margelo::nitro::crypto {
66

77
class ChaCha20Poly1305Cipher : public HybridCipher {
88
public:
9-
ChaCha20Poly1305Cipher() : HybridObject(TAG), final_called(false) {}
9+
ChaCha20Poly1305Cipher() : HybridObject(TAG) {}
1010
~ChaCha20Poly1305Cipher() {
1111
// Let parent destructor free the context
1212
ctx = nullptr;
@@ -24,7 +24,6 @@ class ChaCha20Poly1305Cipher : public HybridCipher {
2424
static constexpr int kKeySize = 32;
2525
static constexpr int kNonceSize = 12;
2626
static constexpr int kTagSize = 16; // Poly1305 tag is always 16 bytes
27-
bool final_called;
2827
};
2928

3029
} // namespace margelo::nitro::crypto

‎packages/react-native-quick-crypto/cpp/cipher/HybridCipher.cpp‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ void HybridCipher::checkCtx() const {
2727
}
2828
}
2929

30+
void HybridCipher::checkNotFinalized() const {
31+
if (is_finalized) {
32+
throw std::runtime_error("Unsupported state or unable to authenticate data");
33+
}
34+
}
35+
3036
bool HybridCipher::maybePassAuthTagToOpenSSL() {
3137
if (auth_tag_state == kAuthTagKnown) {
3238
OSSL_PARAM params[] = {OSSL_PARAM_construct_octet_string(OSSL_CIPHER_PARAM_AEAD_TAG, auth_tag, auth_tag_len),
@@ -48,6 +54,7 @@ void HybridCipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const std
4854
EVP_CIPHER_CTX_free(ctx);
4955
ctx = nullptr;
5056
}
57+
is_finalized = false;
5158

5259
// 1. Get cipher implementation by name
5360
const EVP_CIPHER* cipher = EVP_get_cipherbyname(cipher_type.c_str());
@@ -100,6 +107,7 @@ void HybridCipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const std
100107
std::shared_ptr<ArrayBuffer> HybridCipher::update(const std::shared_ptr<ArrayBuffer>& data) {
101108
auto native_data = ToNativeArrayBuffer(data);
102109
checkCtx();
110+
checkNotFinalized();
103111
size_t in_len = native_data->size();
104112
if (in_len > INT_MAX) {
105113
throw std::runtime_error("Message too long");
@@ -125,6 +133,7 @@ std::shared_ptr<ArrayBuffer> HybridCipher::update(const std::shared_ptr<ArrayBuf
125133

126134
std::shared_ptr<ArrayBuffer> HybridCipher::final() {
127135
checkCtx();
136+
checkNotFinalized();
128137
// Block size is max output size for final, unless EVP_CIPH_NO_PADDING is set
129138
int block_size = EVP_CIPHER_CTX_block_size(ctx);
130139
if (block_size <= 0)
@@ -149,8 +158,8 @@ std::shared_ptr<ArrayBuffer> HybridCipher::final() {
149158

150159
// Context should NOT be freed here. It might be needed for getAuthTag() for GCM/OCB.
151160
// The context will be freed by the destructor (~HybridCipher) when the object goes out of scope.
161+
is_finalized = true;
152162

153-
// Return the shared_ptr<NativeArrayBuffer> (implicit upcast to shared_ptr<ArrayBuffer>)
154163
return native_final_chunk;
155164
}
156165

‎packages/react-native-quick-crypto/cpp/cipher/HybridCipher.hpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ class HybridCipher : public HybridCipherSpec {
5353
protected:
5454
// Properties
5555
bool is_cipher = true;
56+
bool is_finalized = false;
5657
std::string cipher_type;
5758
EVP_CIPHER_CTX* ctx = nullptr;
5859
bool pending_auth_failed = false;
@@ -66,6 +67,7 @@ class HybridCipher : public HybridCipherSpec {
6667
// Methods
6768
int getMode();
6869
void checkCtx() const;
70+
void checkNotFinalized() const;
6971
bool maybePassAuthTagToOpenSSL();
7072
};
7173

‎packages/react-native-quick-crypto/cpp/cipher/XChaCha20Poly1305Cipher.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,11 @@ void XChaCha20Poly1305Cipher::init(const std::shared_ptr<ArrayBuffer> cipher_key
4545

4646
data_buffer_.clear();
4747
aad_.clear();
48-
final_called_ = false;
48+
is_finalized = false;
4949
}
5050

5151
std::shared_ptr<ArrayBuffer> XChaCha20Poly1305Cipher::update(const std::shared_ptr<ArrayBuffer>& data) {
52+
checkNotFinalized();
5253
#ifndef BLSALLOC_SODIUM
5354
throw std::runtime_error("XChaCha20Poly1305Cipher: libsodium must be enabled (BLSALLOC_SODIUM)");
5455
#else
@@ -64,6 +65,7 @@ std::shared_ptr<ArrayBuffer> XChaCha20Poly1305Cipher::update(const std::shared_p
6465
}
6566

6667
std::shared_ptr<ArrayBuffer> XChaCha20Poly1305Cipher::final() {
68+
checkNotFinalized();
6769
#ifndef BLSALLOC_SODIUM
6870
throw std::runtime_error("XChaCha20Poly1305Cipher: libsodium must be enabled (BLSALLOC_SODIUM)");
6971
#else
@@ -80,12 +82,12 @@ std::shared_ptr<ArrayBuffer> XChaCha20Poly1305Cipher::final() {
8082
throw std::runtime_error("XChaCha20Poly1305Cipher: encryption failed");
8183
}
8284

83-
final_called_ = true;
85+
is_finalized = true;
8486
size_t ct_len = data_buffer_.size();
8587
return std::make_shared<NativeArrayBuffer>(ciphertext, ct_len, [=]() { delete[] ciphertext; });
8688
} else {
8789
if (data_buffer_.empty()) {
88-
final_called_ = true;
90+
is_finalized = true;
8991
return std::make_shared<NativeArrayBuffer>(nullptr, 0, nullptr);
9092
}
9193

@@ -101,7 +103,7 @@ std::shared_ptr<ArrayBuffer> XChaCha20Poly1305Cipher::final() {
101103
throw std::runtime_error("XChaCha20Poly1305Cipher: decryption failed - authentication tag mismatch");
102104
}
103105

104-
final_called_ = true;
106+
is_finalized = true;
105107
size_t pt_len = data_buffer_.size();
106108
return std::make_shared<NativeArrayBuffer>(plaintext, pt_len, [=]() { delete[] plaintext; });
107109
}
@@ -126,7 +128,7 @@ std::shared_ptr<ArrayBuffer> XChaCha20Poly1305Cipher::getAuthTag() {
126128
if (!is_cipher) {
127129
throw std::runtime_error("getAuthTag can only be called during encryption");
128130
}
129-
if (!final_called_) {
131+
if (!is_finalized) {
130132
throw std::runtime_error("getAuthTag must be called after final()");
131133
}
132134

‎packages/react-native-quick-crypto/cpp/cipher/XChaCha20Poly1305Cipher.hpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ namespace margelo::nitro::crypto {
1616

1717
class XChaCha20Poly1305Cipher : public HybridCipher {
1818
public:
19-
XChaCha20Poly1305Cipher() : HybridObject(TAG), final_called_(false) {}
19+
XChaCha20Poly1305Cipher() : HybridObject(TAG) {}
2020
~XChaCha20Poly1305Cipher();
2121

2222
void init(const std::shared_ptr<ArrayBuffer> cipher_key, const std::shared_ptr<ArrayBuffer> iv) override;
@@ -37,7 +37,6 @@ class XChaCha20Poly1305Cipher : public HybridCipher {
3737
std::vector<uint8_t> aad_;
3838
std::vector<uint8_t> data_buffer_;
3939
uint8_t auth_tag_[kTagSize];
40-
bool final_called_;
4140
};
4241

4342
} // namespace margelo::nitro::crypto

‎packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,14 @@ void XSalsa20Cipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const s
2828
// Copy key and nonce data
2929
std::memcpy(key, native_key->data(), crypto_stream_KEYBYTES);
3030
std::memcpy(nonce, native_iv->data(), crypto_stream_NONCEBYTES);
31+
is_finalized = false;
3132
}
3233

3334
/**
3435
* xsalsa20 call to sodium implementation
3536
*/
3637
std::shared_ptr<ArrayBuffer> XSalsa20Cipher::update(const std::shared_ptr<ArrayBuffer>& data) {
38+
checkNotFinalized();
3739
#ifndef BLSALLOC_SODIUM
3840
throw std::runtime_error("XSalsa20Cipher: libsodium must be enabled to use this cipher (BLSALLOC_SODIUM is not defined).");
3941
#else
@@ -51,9 +53,11 @@ std::shared_ptr<ArrayBuffer> XSalsa20Cipher::update(const std::shared_ptr<ArrayB
5153
* xsalsa20 does not have a final step, returns empty buffer
5254
*/
5355
std::shared_ptr<ArrayBuffer> XSalsa20Cipher::final() {
56+
checkNotFinalized();
5457
#ifndef BLSALLOC_SODIUM
5558
throw std::runtime_error("XSalsa20Cipher: libsodium must be enabled to use this cipher (BLSALLOC_SODIUM is not defined).");
5659
#else
60+
is_finalized = true;
5761
return std::make_shared<NativeArrayBuffer>(nullptr, 0, nullptr);
5862
#endif
5963
}

0 commit comments

Comments
 (0)