Skip to content

Reject KDF iteration counts that are too large - #81

Open
cplieger wants to merge 1 commit into
SSLMate:masterfrom
cplieger:bound-decode-kdf-iterations
Open

Reject KDF iteration counts that are too large#81
cplieger wants to merge 1 commit into
SSLMate:masterfrom
cplieger:bound-decode-kdf-iterations

Conversation

@cplieger

Copy link
Copy Markdown

Fixes #80.

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 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 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.

This bounds 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. 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 check macData.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:

pbeCipherFor and doMac are 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. WithIterations now 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 pbDecrypt and doMac for all four paths, and an end-to-end case through DecodeTrustStore. 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. gofmt clean, and go vet and go test pass on amd64, 386, arm and arm64.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decode has no bound on KDF iterations, so a 950-byte file dictates arbitrary CPU

1 participant