Fix false positives in WOLFSSL_CIPHER_TEXT_CHECK for small records#10288
Closed
Frauschi wants to merge 1 commit intowolfSSL:masterfrom
Closed
Fix false positives in WOLFSSL_CIPHER_TEXT_CHECK for small records#10288Frauschi wants to merge 1 commit intowolfSSL:masterfrom
Frauschi wants to merge 1 commit intowolfSSL:masterfrom
Conversation
Merged
Contributor
Author
|
Closing as the same fixes have been merged to master in #10296 |
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.
Problem
The glitch-detection check guarded by
WOLFSSL_CIPHER_TEXT_CHECK(enabled by--enable-maxstrength) saves up to 8 bytes of plaintext before encryption and compares them to the start of the ciphertext afterward. If they match, it assumes the AEAD pass-through'd and returnsENCRYPT_ERROR.The copy and compare both used
min(dataSz, sizeof(sanityCheck)), so when the plaintext is smaller than 8 bytes the check only compares that many bytes of ciphertext, which collides legitimately with random probability 1/256^dataSz. For a 1-byte plaintext (e.g. an empty TLS 1.3 application-data record, whose only plaintext is the content-type byte) that's ~1/256 per record.This surfaced as sporadic failures of
test_tls13_empty_record_limitintroduced in #10187, which buildsWOLFSSL_MAX_EMPTY_RECORDSsuch records in a row, resulting in a combined false-positive rate of ~8–9%.Fix
In both
EncryptTls13(src/tls13.c) andEncrypt(src/internal.c), only arm the sanity check when there is a fullsizeof(sanityCheck)(8) bytes of plaintext, and always compare the full 8 bytes. Shorter records skip the check entirely rather than running an unreliable version of it (a 1-byte glitch check isn't trustworthy anyway).