Fix audit bugs and remove v13 NanoTDF support#43
Conversation
- Remove v13 NanoTDF creation; all NanoTDFs use v12 headers and v12 HKDF salt. - Fix NanoTDF Collection nonce padding to [IV][zeros]. - Fix KASRewrapClient clientPublicKey PEM conversion. - Fix TDF-JSON/CBOR EC wrap/unwrap mismatch and set .ecWrapped type. - Fix TDF-CBOR algorithm and key-access-type fidelity. - Fix KASService rewrap endpoint path to /kas/v2/rewrap. - Reject zero-length embedded policy bodies. - Use constant-time policy binding comparison. - Use SecRandomCopyBytes for nonce/key generation. - Remove stray print statements and other low-severity cleanup.
![superninja-app[bot]](https://avatars.githubusercontent.com/in/1700253?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — PR #43: Fix audit bugs and remove v13 NanoTDF support
Reviewed as a senior cryptography / Swift / OpenTDF engineer. I read the full diff (24 files, +523/−322) and the surrounding code for context (CryptoHelper.adjustNonce, getPayloadPlaintext, TDFCrypto EC wrap/unwrap, KASService.verifyPolicyBinding, TDFFormatDetector, the ParsingError/KASRewrapError enums and all updated throws call-sites). Overall this is a high-quality, security-positive PR that fixes several real correctness and hardening bugs. Detailed findings below.
1. Cryptographic Correctness ✅ (strong)
-
Nonce padding fix is correct and now internally consistent.
CryptoHelper.adjustNonce(_:to:)pads a short nonce by appending zeros, yielding[b1,b2,b3,0,0,0,0,0,0,0,0,0]. The create path (createNanoTDF,NanoTDF.swift:259–261) and decrypt path (getPayloadPlaintext,NanoTDF.swift:67) both go throughadjustNonce. The previousNanoTDFCollection/Decryptorlayout[0×9, b1,b2,b3]did not match that convention. The change inNanoTDFCollection.swift(toGCMNonce()andnonceBuffer[0..2]) andNanoTDFCollectionDecryptor.swiftnow aligns all four paths on IV-first. This is the correct fix and the round-trip is covered bytestPrivateKeyDecryptorand the updatedNanoTDFCollectionTests. -
CSPRNG hardening is a genuine improvement.
TDFCrypto.randomBytes(count:)previously usedSystemRandomNumberGeneratorelement-by-element; it now usesSecRandomCopyByteswith anOSStatuscheck and throws on failure. Same pattern applied toCryptoHelper.generateNonce(...), which nowthrowsinstead of silently ignoring theSecRandomCopyBytesresult via_ =. This removes a class of silent-failure / weak-randomness bugs for IV and key generation. -
EC wrap/unwrap symmetry fixed. TDF-JSON/CBOR encrypt sets
type = .ecWrappedand the decrypt path now callsunwrapSymmetricKeyWithEC(ECDH → HKDF-SHA256(empty salt/info) → AES-GCM) instead of the mismatchedunwrapSymmetricKeyWithRSA. The unwrap (TDFCrypto.swift:341+) mirrorswrapWithP256exactly (same HKDF parameters, AES-GCM combined formatnonce[12]+ct+tag[16]), guardswrappedData.count > 28, andsecureZero()s the decrypted key viadefer. Correct ECIES construction. Round-trips proven bytestTDFJSONRoundTripWithECPrivateKey/testTDFCBORRoundTripWithECPrivateKey. -
HKDF salt is now consistently v12 across
createNanoTDF,NanoTDFCollectionDecryptor, andKASService.rewrapKeyInternal. No salt mismatch remains. -
clientPublicKeyPEM bug fixed (high impact).KASRewrapClient.swift:355previously didString(data: clientKeyPair.publicKey, encoding: .utf8) ?? ""on a compressed EC point — that is not valid UTF-8, so it silently sent an emptyclientPublicKey, which would break the rewrap response unwrap. The newpemRepresentation(compressedPublicKey:)properly reconstructs SPKI/PEM from the 33-byte point. Good, and it round-trips throughextractCompressedKeyFromPEMintestClientPublicKeyPEMConversion.
2. Cybersecurity Concerns ✅
- Timing-attack mitigation:
verifyPolicyBindingnow usesconstantTimeEquals(XOR-accumulate overzip,result |= a ^ b). Correct constant-time equality for equal-length inputs. The earlyguard lhs.count == rhs.countleaks only the (public) tag length — standard and acceptable; a one-line comment noting this would be ideal. - Input validation hardening:
BinaryParsernow rejects zero-length embedded policy bodies (contentLength == 0 → throw .invalidPolicy), matching spec §3.4.2.3.2 (min length 1). Previously it fabricatedData([0x00]), which masked malformed input. Covered bytestZeroLengthEmbeddedPolicyBodyFails. - KAS request hygiene:
KASRewrapClientnow rejects empty / non-base64 policy bodies before building the rewrap request. - Information-leak cleanup: removal of stray
print(...)fromBinaryParser/NanoTDFis good — those could have leaked structural details about malformed/attacker-supplied inputs into logs.
3. Swift Implementation Quality ✅ (minor nits)
KASRewrapClient→public final class … Sendable: good concurrency-safety tightening.KeyStore.publicKey/privateKeynow use safebuffer.prefix/suffixslicing instead ofbaseAddress!pointer arithmetic — removes force-unwrap crash risk on empty buffers. Nice.- Migration of
return X-style switches to Swift's expression-switch(PayloadKeyAccess.kasKeyCurve,PolicyKeyAccess.curve, CBOR enum mappings) is idiomatic for Swift 6.2. - Nit — redundant rethrow: in
BinaryParser.parseHeader(both v12 paths) thewrapper adds nothing;do { ... policy = p } catch { throw error }
try readPolicyField(...)could be called directly. Harmless but worth simplifying. - The
RewrapURLProtocoltest mock usingnonisolated(unsafe) static varis acceptable for a single-threaded test but technically a shared-mutable-state hazard; fine for now.
4. OpenTDF Specification Compliance ✅
- Collection nonce
[IV][zeros]now matches the NanoTDF spec/adjustNonceconvention. KASService.rewrapKeyendpoint corrected to/kas/v2/rewrap(was/rewrap) — aligns with the KAS v2 protocol.- v12 (
0x4C"L1L") is now the single emitted header form;Header.toData()no longer branches onkasPublicKeyemptiness. Consistent with the stated intent to standardize on v12 (and otdfctl interop). - CBOR key-access types (
wrapped/remote/remoteWrapped/ecWrapped) and symmetric algorithms (AES-128/256-GCM/CBC) now encode/decode with fidelity instead of silently collapsing towrapped/AES-256-GCM. Hash-alg mapping now throws on unknown values instead of silently defaulting toHS256— a correctness win that prevents accepting/forging mislabeled bindings. TDFFormatDetector.cborMagicreduced to the 3-byte self-describe tag plus a check that the 4th byte is a CBOR map major type (& 0xE0 == 0xA0). Theheader[cborMagic.count]access is safe under the existingdata.count >= 4guard — no out-of-bounds — and is more spec-faithful than the hardcoded0xA5.
5. General Code Quality ✅
- Error handling moves decisively from
print + return nilto typed thrown errors (ParsingError.invalidPolicy,TDFCBORError.*,KASRewrapError.*) — better failure propagation and testability. - Strong test coverage added for every behavioral change: PEM conversion, rewrap endpoint path (
RewrapURLProtocolmock), EC round-trips, CBOR algorithm/key-access fidelity, zero-length policy rejection, and the new collection nonce layout. Author reportsswift test: 216 passed / 0 failed and a cleanswiftformat --lint. All updatedthrowscall-sites (generateNonce,enumToHashAlg/hashAlgToEnum) are consistently propagated.
6. Summary & Recommendations
This PR meaningfully improves both correctness and security and is well-tested. Recommendation: Approve. None of the findings below are blocking, but please address them in a follow-up.
Should-fix / acknowledge (non-blocking):
- Backward compatibility — biggest item. Removing the v13 HKDF-salt fallback in
KASService.rewrapKeyInternal(and dropping theversionhint) means any NanoTDF previously created with the v13 (0x4D) salt can no longer be rewrapped/decrypted by this service. If no v13 artifacts exist in the wild this is fine, but it should be explicitly stated as a breaking change and ideally noted in the migration doc / release notes. - Silent capability reduction in
fetchKasEcPublicKey: it now rejectsecP384/ecP521(onlyecP256accepted). Reasonable given only P-256 parsing is implemented, but callers relying on P-384/521 will now getunsupportedKeyAlgorithm. Worth a CHANGELOG line. TDFArchiveWriterdefault changed from.deflateto.none. This changes on-the-wire output size/format for default callers; confirm this is intentional (e.g., for spec/interop) rather than an incidental change.
Minor / nits:
- Remove the redundant
do { … } catch { throw error }wrappers inBinaryParser.parseHeader. - Add a one-line comment in
constantTimeEqualsnoting the length-comparison is intentionally non-secret. TDFCrypto.randomBytesthrowsTDFCryptoError.keyWrapFailed(nil)on RNG failure — a dedicatedrandomGenerationFailedcase would read better than reusingkeyWrapFailed.
Excellent, surgical hardening work overall — particularly the CSPRNG fix, the EC wrap/unwrap pairing, the constant-time binding check, and the clientPublicKey PEM correction. 👍
- Add P-256/P-384/P-521 parsing to KASRewrapClient.validateEcPublicKeyPEM. - Introduce KasEcPublicKey enum to type parsed EC keys by curve. - Simplify redundant do/catch wrappers in BinaryParser.parseHeader. - Add comment noting constant-time compare length check is intentionally public. - Add TDFCryptoError.randomGenerationFailed and use it for SecRandomCopyBytes failures. - Add unit tests for P-256/P-384/P-521 PEM validation.
|
@gitar-bot review |
On it — running a full review now. |
|
@gitar-bot review Addressed both findings:
All 220 tests pass and swiftformat is clean. |
On it — running a full review now. |
Code Review ✅ Approved 2 resolved / 2 findingsEliminates v13 NanoTDF support and addresses multiple audit findings, including corrected nonce padding, constant-time policy verification, and secure random number generation. The removal of dead code and resolution of cryptographic implementation inconsistencies have been verified. ✅ 2 resolved✅ Quality: Dead code: hkdfSaltV13 unused after v13 removal
✅ Edge Case: v13 NanoTDFs can be parsed but no longer rewrapped (no v13 salt)
Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
This PR addresses the bugs found in the recent codebase audit and removes v13 NanoTDF support.
Highlights
[IV][zeros]).KASRewrapClientsending an emptyclientPublicKeyby converting the compressed EC point to PEM.keyAccess.type = .ecWrapped.KASService.rewrapKeyendpoint path (/kas/v2/rewrap).BinaryParser.SecRandomCopyBytesfor nonce/key generation and check return status.print(...)statements from library code.Verification
swift test: 216 tests, 0 failuresswiftformat --lint --swiftversion 6.2 .: 0 files require formattingBranch
bugfix/audit-fixes