Fix GCM tag state after one-shot decrypt; RSA decrypt2 exact length - #88
Closed
MarkAtwood wants to merge 2 commits into
Closed
MarkAtwood wants to merge 2 commits into
MarkAtwood wants to merge 2 commits into
Conversation
GnuTLS calls the provider's tag callback to get the tag it then compares with the received one. wolfssl_cipher_aead_decrypt() set ctx->enc to 0, and on such a handle the callback copied the tag FROM the caller's buffer instead of writing it. After a one-shot gnutls_aead_cipher_decrypt(), a gnutls_aead_cipher_decryptv2() with no ciphertext therefore accepted any tag over any AAD (an all-zero tag over attacker AAD verified) and rejected the correct one, and encryptv/encryptv2 on empty plaintext returned the caller's buffer as the tag. The callback now always writes the tag: the tag of the data processed or, with no data, the tag over the AAD alone. The tag ends the message, so a handle reused without a new IV does not return it again. If it cannot compute one it writes random bytes (gnutls_rnd, then getrandom), which no received tag will match, and aborts if neither gives random bytes. aead_decrypt no longer changes ctx->enc, and the "tag set externally" state is removed. New test test_aesgcm_tag_state, values from pyca/cryptography AESGCM.
gnutls_privkey_decrypt_data2() gives the exact plaintext size. A valid PKCS#1 v1.5 or OAEP message shorter than that decrypted "successfully" with the caller's buffer only partly written, and a failed decryption could leave decrypted bytes in it. TLS RSA key exchange prefills the premaster buffer with random bytes and ignores decrypt2's result, so on any failure the buffer must be left unchanged, or the handshake becomes a padding oracle. decrypt2 now always decrypts into a zeroed scratch buffer and copies to the caller's buffer with a constant-time masked select that writes the plaintext only when its length is exactly the requested size; otherwise it returns GNUTLS_E_DECRYPTION_FAILED, as GnuTLS's own implementation does. The scratch buffer is wiped before return. New test test_rsa_decrypt2_length: 48 bytes into a 48-byte buffer decrypts; 16-, 47- and 64-byte messages and corrupted or truncated ciphertexts fail and leave the prefilled buffer unchanged. The test passes without the provider (GnuTLS's nettle path) as the oracle.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unconditional Linux-only random API breaks the supported macOS build, and several contract tests need strengthening.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Fixes AES-GCM tag state reuse and enforces exact-length RSA decrypt_data2 output.
Changes:
- Makes GCM tag callbacks output-only and resets message state.
- Uses constant-time RSA plaintext selection with scratch-buffer clearing.
- Adds regression tests for both fixes.
| File | Description |
|---|---|
src/cipher.c |
Corrects GCM tag generation and state handling. |
src/pk.c |
Enforces exact RSA plaintext length. |
tests/test_aesgcm_tag_state.c |
Tests reused-handle GCM behavior. |
tests/test_rsa_decrypt2_length.c |
Tests RSA length and failure handling. |
tests/Makefile |
Registers the new tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include "gnutls_compat.h" | ||
| #include "logging.h" | ||
| #include <stdlib.h> | ||
| #include <sys/random.h> |
Comment on lines
+1140
to
+1142
| if (!alloc_plaintext) { | ||
| ret = rsa_decrypt2_select(plaintext, plain, ret); | ||
| gnutls_memset(plain, 0, scratch_size); |
Comment on lines
+61
to
+64
| if (ret >= 0) { | ||
| printf("FAILURE - reported as decrypted\n"); | ||
| return 1; | ||
| } |
Comment on lines
+1133
to
+1135
| /* The tag is output only: gnutls compares it with the received tag after | ||
| * a decrypt, so it must always be written. When it cannot be computed it | ||
| * is filled with random bytes, which no received tag will match. */ |
Author
|
Closing in favour of #91, per Reda. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two fixes in the wrapper, each with a test that fails on
mainand passes here.AES-GCM tag callback (cipher.c).
wolfssl_cipher_aead_decrypt()setctx->enc = 0, andwolfssl_cipher_tag()then treated its buffer as an input tag on that handle. GnuTLS calls the tag callback to get the tag it compares with the received one. So after a one-shotgnutls_aead_cipher_decrypt()on a handle,gnutls_aead_cipher_decryptv2()with empty ciphertext on the same handle accepted any tag over any AAD (an all-zero tag over attacker AAD verified) and rejected the correct one.encryptv/encryptv2on empty plaintext returned the caller's buffer as the tag. This needs that specific API sequence on one handle; TLS does not use it. The tag callback is now output only: the tag of the data processed, or with no data the tag over the AAD. Returning the tag ends the message (tag_setreset). If the tag cannot be computed it writes random bytes (gnutls_rnd, thengetrandom(), elseabort()), which no received tag matches.RSA decrypt2 (pk.c).
gnutls_privkey_decrypt_data2()gives an exact plaintext size, but a valid shorter message returned success with the buffer partly written. TLS RSA key exchange (lib/auth/rsa.c,rsa_psk.c) prefills that buffer with a random premaster and ignores the result, so on failure the buffer must stay unchanged. Otherwise the handshake outcome tells the client whether the padding was valid. decrypt2 now decrypts into a scratch buffer and copies it into the caller's buffer with a constant-time masked select only when the length is exactly the requested size. Any other result, length or padding, returnsGNUTLS_E_DECRYPTION_FAILEDwith the caller's buffer unchanged, as nettle'srsa_sec_decryptdoes. The scratch buffer is cleared before return.Tests:
tests/test_aesgcm_tag_state.c: one-shot decrypt, then decryptv2 with the correct tag (accepted) and a zero tag (rejected); encryptv2 tag equals the encrypt tag.tests/test_rsa_decrypt2_length.c: 48-byte buffer prefilled with a pattern; the 48-byte message decrypts; 16, 47 and 64-byte messages and a corrupted or truncated ciphertext fail and leave the buffer unchanged.Checked on wolfSSL master (non-FIPS):
make test27/27 with the provider and withGNUTLS_NO_PROVIDER=1; both new tests valgrind clean; both new tests fail againstmain.