MINOR: Use constant-time comparison for signed footer GCM tag#3655
Open
anxkhn wants to merge 1 commit into
Open
MINOR: Use constant-time comparison for signed footer GCM tag#3655anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
The signature of an encrypted file with a plaintext (signed) footer is verified in ParquetMetadataConverter.verifyFooterIntegrity by comparing the stored GCM authentication tag against a recomputed tag. It used java.util.Arrays.equals, which returns as soon as it finds a mismatching byte, so the comparison time depends on how many leading bytes match. For a MAC/authentication-tag check that is a timing side channel (CWE-208). Replace it with java.security.MessageDigest.isEqual, which has the same boolean contract (true iff both arrays are non-null, of equal length, and byte-for-byte equal) but runs in time that does not depend on the number of matching bytes. The change is behavior-preserving; tampered footers are still rejected with a TagVerificationException. Add TestFooterIntegrity covering both the accepted valid signature and a tampered GCM tag that must be rejected. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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.
Rationale for this change
When an encrypted file is written with a plaintext (signed) footer, the footer
carries an AES-GCM authentication tag. On read,
ParquetMetadataConverter.verifyFooterIntegrityrecomputes the tag and comparesit against the stored one to detect tampering.
That comparison used
java.util.Arrays.equals, which returns as soon as it hitsa mismatching byte, so its running time depends on how many leading bytes match.
Comparing a secret authentication tag this way is a timing side channel
(CWE-208): an attacker who can measure verification time over many crafted files
could, in principle, recover the correct tag byte by byte. Constant-time
comparison is the standard practice for MAC/authentication-tag checks.
What changes are included in this PR?
ParquetMetadataConverter.verifyFooterIntegrity, replaceArrays.equals(gcmTag, calculatedTag)withMessageDigest.isEqual(gcmTag, calculatedTag)and addimport java.security.MessageDigest;. A short comment notes the CWE-208rationale.
MessageDigest.isEqualhas the same boolean contract (true iff botharrays are non-null, of equal length, and byte-for-byte equal) but is
implemented to run in time independent of the number of matching bytes, so the
change is behavior-preserving: tampered footers are still rejected with a
TagVerificationException. (TheArraysimport stays; it is used elsewhere inthe file.)
TestFooterIntegrityinparquet-hadoopcovering the signedplaintext-footer path end to end: a valid signature is accepted, and a footer
whose GCM tag has one byte flipped is rejected with
TagVerificationException.The other
Arrays.equalscalls in the crypto module compare non-secretidentifiers (AAD prefix, key-metadata reuse checks), not a secret MAC, so they
are intentionally left unchanged.
Are these changes tested?
Yes.
mvn -pl parquet-hadoop -Dtest=TestFooterIntegrity test-> both cases pass.mvn -pl parquet-hadoop -Dtest='ColumnEncryptorTest,TestPropertiesDrivenEncryption,TestFooterIntegrity' test-> 43 tests, 0 failures, 0 errors.
tag check makes
tamperedFooterSignatureIsRejectedfail (nothing thrown),which shows the tampered bytes actually reach the reader and the tag check is
what rejects them.
A true timing measurement is impractical to assert deterministically in a unit
test, so this PR verifies functional equivalence (accept valid, reject tampered)
rather than the timing property itself.
Are there any user-facing changes?
No. This is an internal hardening of the footer-signature check with identical
observable behavior (same acceptance and rejection outcomes, same exception on
mismatch). No API, format, or configuration changes.