anxkhn opened a new pull request, #3655:
URL: https://github.com/apache/parquet-java/pull/3655
### 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.verifyFooterIntegrity` recomputes the tag and
compares
it against the stored one to detect tampering.
That comparison used `java.util.Arrays.equals`, which returns as soon as it
hits
a 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?
- In `ParquetMetadataConverter.verifyFooterIntegrity`, replace
`Arrays.equals(gcmTag, calculatedTag)` with
`MessageDigest.isEqual(gcmTag, calculatedTag)` and add
`import java.security.MessageDigest;`. A short comment notes the CWE-208
rationale. `MessageDigest.isEqual` has the same boolean contract (true iff
both
arrays 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`. (The `Arrays` import stays; it is used
elsewhere in
the file.)
- Add `TestFooterIntegrity` in `parquet-hadoop` covering the signed
plaintext-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.equals` calls in the crypto module compare non-secret
identifiers (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.
- The existing encryption suites still pass with the change:
`mvn -pl parquet-hadoop
-Dtest='ColumnEncryptorTest,TestPropertiesDrivenEncryption,TestFooterIntegrity'
test`
-> 43 tests, 0 failures, 0 errors.
- The negative test was confirmed to be meaningful: temporarily neutralizing
the
tag check makes `tamperedFooterSignatureIsRejected` fail (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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
