Reject KDF iteration counts that are too large - #81
Open
cplieger wants to merge 1 commit into
Open
Conversation
A PKCS#12 file carries its own KDF iteration counts, and decoding honored them without any bound. The counts are plain int fields unmarshalled from the DER, so a file could request up to the largest value an int holds, while the file itself stayed the same size: on this machine 6 bytes separate a 946-byte file that decodes in 1ms from a 952-byte one that takes 16.5s, and a count of 2^30 takes about 6.6 minutes. Callers cannot impose the bound themselves. The MAC's count and each encryptedData safe's outer algorithm are readable from the plaintext, but a pkcs8ShroudedKeyBag nested inside an encryptedData safe is not: getSafeContents decrypts every encrypted safe and flattens the bags before DecodeChain derives from the first shrouded one, and pbDecrypt, getSafeContents and decodePkcs8ShroudedKeyBag are all unexported. Bound every count a KDF is given: the original PBE and PBES2 paths in crypto.go, and the original MAC and PBMAC1 paths in mac.go. The maximum is 5,000,000, matching the JDK's PKCS12KeyStore.MAX_ITERATION_COUNT, and sits above the highest count OWASP recommends for PBKDF2 (1,400,000, for HMAC-SHA1), so no real file is affected. pbeCipherFor and doMac are shared with the encoder, so this bounds encoding too. That is deliberate: a file this package will not read back is not worth writing. WithIterations now panics above the maximum, so a caller asking for one learns at the call rather than from an encryption error later. The limit is per KDF invocation rather than per file, so a file with several encrypted safes can still cost a multiple of one derivation. That bounds the cost to its size, which is the property that was missing.
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.
Fixes #80.
A PKCS#12 file carries its own KDF iteration counts, and decoding honored them without any bound. The counts are plain
intfields unmarshalled from the DER, so a file could request up to the largest value anintholds while staying the same size: on my machine 6 bytes separate a 946-byte file that decodes in 1 ms from a 952-byte one that takes 16.5 s, and a count of 2^30 takes about 6.6 minutes.Callers cannot impose the bound themselves. The MAC's count and each
encryptedDatasafe's outer algorithm are readable from the plaintext, but apkcs8ShroudedKeyBagnested inside anencryptedDatasafe is not:getSafeContentsdecrypts every encrypted safe and flattens the bags beforeDecodeChainderives from the first shrouded one, andpbDecrypt,getSafeContentsanddecodePkcs8ShroudedKeyBagare all unexported.This bounds every count a KDF is given: the original PBE and PBES2 paths in
crypto.go, and the original MAC and PBMAC1 paths inmac.go. I grepped for a fifth and did not find one. Each check sits before the derivation it guards, with only ASN.1 unmarshalling and OID comparisons ahead of it. The PBMAC1 path deliberately does not checkmacData.Iterations, since RFC 9579 makes that field ignored there.The maximum is 5,000,000, matching the JDK's
PKCS12KeyStore.MAX_ITERATION_COUNT. It sits above the highest count OWASP recommends for PBKDF2 (1,400,000, for HMAC-SHA1), so any file the JDK accepts this still accepts.Two things worth calling out, since neither is obvious from the diff:
pbeCipherForanddoMacare shared with the encoder, so this bounds encoding too. I took that as the right outcome rather than working around it: a file this package will not read back is not worth writing.WithIterationsnow panics above the maximum so a caller asking for one finds out at the call instead of from an encryption error later. If you would rather leave encoding unbounded, the limit can be threaded through those two helpers instead; say the word and I will redo it that way.The limit is per KDF invocation rather than per file, so a file with several encrypted safes can still cost a multiple of one derivation. That bounds cost to file size, which is the property that was missing, but it is not a single-derivation cap.
Tests follow the shape of the ones in c0472ed: a table of over-limit counts asserting the exact error, the boundary value accepted, coverage through
pbDecryptanddoMacfor all four paths, and an end-to-end case throughDecodeTrustStore. The end-to-end one asserts the iteration error specifically, because that file fails on padding anyway and a bare "expected an error" would pass without the check. I verified each test fails when the guard is removed.gofmtclean, andgo vetandgo testpass on amd64, 386, arm and arm64.