pkcs12: KDF support #1154
pkcs12: KDF support #1154
Conversation
…y, removing unused error class, code simplifications, and new test cases for very long output keys
Co-authored-by: Tony Arcieri <bascule@gmail.com>
|
|
||
| /// Transform a utf-8 string in a unicode (utf16) string as binary array. | ||
| /// The Utf16 code points are stored in big endian format with two trailing zero bytes. | ||
| fn str_to_unicode(utf8_str: &str) -> Vec<u8> { |
There was a problem hiding this comment.
The RFC talks about BMPString which happens to be UTF16, but renaming that str_to_bmpstring or something would be great I think.
There was a problem hiding this comment.
Hmm, I guess we should really add BMPString support to der
There was a problem hiding this comment.
I'm a bit uncomfortable with the use of str for inputs here. This will backfire with any 4-Byte UTF8 character. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c4edb02a6d7bf8fb45147475e43f1930
I don't know what the best course of action is here though, the best I can think of would be to add a der::asn1::BmpString but it is meant to only be subtyped from what I can read (https://www.oss.com/asn1/resources/asn1-made-simple/asn1-quick-reference/bmpstring.html)
There was a problem hiding this comment.
(sorry last comment was sent before I refreshed the page, well, go for a BmpString then)
There was a problem hiding this comment.
The format of an ASN1 BMPString differs slightly from what is here required: It has a leading id byte 0x1e, one or bytes indicating the length in bytes, and no terminating zeros. I.e. using BMPString would also requires some pre-processing. Multi-Byte UTF8 characters should work fine here, at least the example you provided gives the exact same result als openssl's function PKCS12_gen_key_utf8 (see the added test case and the respective test program).
Openssl provides similar functions for PKCS12_gen_key_uni and PCKS12_gen_asc for unicode and ASCII string passwords (see here: https://www.openssl.org/docs/man3.0/man3/PKCS12_key_gen_utf8.html) Maybe a similar approach could be taken here.
However, I am not sure how to interpret the remark "There are no Unicode byte order marks." in the RFC.
There was a problem hiding this comment.
last commit does not really fix the issue with unicode characters that do not encode on two bytes.
RFC 7292 does not really specify the behavior with those.
And I would be in favor of outright rejecting them (or maybe hide a From<Vec<u8>> under hazmat feature flag for a der::asn1::BmpString).
There was a problem hiding this comment.
However, I am not sure how to interpret the remark "There are no Unicode byte order marks." in the RFC.
There was a problem hiding this comment.
A BOM would precede the string, and if it had one in this case would be FEFF to indicate big endian.
BmpString (if it existed) would still be a nice place to handle all of the string conversions. The header doesn't matter: we can still use EncodeValue to avoid it, and two trailing digest bytes are easily added to the Digest prior to finalization.
We can worry about that all later though. It isn't needed for an initial PR.
pkcs12/src/kdf.rs
Outdated
| let mut init_key = vec![0u8; ilen]; | ||
| for i in 0..slen { | ||
| init_key[i] = salt[i % salt.len()]; | ||
| } | ||
| for i in 0..plen { | ||
| init_key[slen + i] = pass_utf16[i % pass_utf16.len()]; | ||
| } | ||
| pass_utf16.zeroize(); |
There was a problem hiding this comment.
This looks like step2,3 & 4
2. Concatenate copies of the salt together to create a string S of
length v(ceiling(s/v)) bits (the final copy of the salt may be
truncated to create S). Note that if the salt is the empty
string, then so is S.
3. Concatenate copies of the password together to create a string P
of length v(ceiling(p/v)) bits (the final copy of the password
may be truncated to create P). Note that if the password is the
empty string, then so is P.
4. Set I=S||P to be the concatenation of S and P.
Could you move code around (to preserve ordering) and copy the algorithm from the RFC as comments?
There was a problem hiding this comment.
I have added comments to the code describing the single steps of the implementation, as far as possible since I derive at some point slightly from the algorithm.
| let mut m = key_len; | ||
| let mut n = 0; | ||
| let mut out = vec![0u8; key_len]; | ||
| loop { |
There was a problem hiding this comment.
RFC calls for a c = ceiling(n/u) and for the loop to be:
for _i in 1..=c {
}
There was a problem hiding this comment.
Using a loop instead has the advantage that the last step 6. could be skipped.
Co-authored-by: Tony Arcieri <bascule@gmail.com>
Co-authored-by: Tony Arcieri <bascule@gmail.com>
…crypt support to use it. added a test case with a pfx from PKITS to exercise the kdf. added implementation of EncryptedPrivateKeyInfo.
|
I copied the current kdf.rs from this PR into my work-in-progress PKCS #12 encoder/decoder project and successfully decoded and decrypted a PKCS #12 from the PKITS data set using the new KDF and compared the resulting cert and key to reference values. This commit has the complete assembly (and will need to be pared down for a PR): carl-wallace@2dbc115. One minor comment on this PR. Using a string password is out of sync with how decrypt_in_place is implemented in the pkcs5 crate. |
I have factored out the conversion from utf8 to byte array and added an additional function |
PKCS#5 doesn't stipulate a string encoding for the password, but the PKCS#12 KDF does (which is arguably a design flaw in the PKCS#12 KDF, but it would only be one of many). I think we might consider making the password argument a |
|
Thanks. I had missed that detail, but ran into the difference relative to pkcs5. It's not really an issue either way. |
|
For a common API you'd probably want to accept a |
|
That sounds right. We'll likely need BMPString to deal with friendlyName attributes for P12 as well. Those are pretty common. |
Implementation of the KDF in RFC7292 Appendix B