RFR 8076999: SunJCE support of password-based encryption scheme 2 params (PBES2) not working
Jamil Nimeh
jamil.j.nimeh at oracle.com
Wed Jul 24 20:04:40 UTC 2019
Hi Valerie, part 2 of my comments (also in-line):
Thanks,
--Jamil
On 7/9/2019 8:59 PM, Valerie Peng wrote:
>
> Hi Jamil,
>
> Please find replies inline below...
>
> On 7/9/2019 9:03 AM, Jamil Nimeh wrote:
>> Hi Valerie, thanks for the comments:
>>
>> On 7/8/2019 4:50 PM, Valerie Peng wrote:
>>>
>>> Hi Jamil,
>>>
>>> Sorry for the late reply. It's been a long while since I looked at
>>> this PBES2 scheme and I need to think a few things through.
>>>
>>> <src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java>
>>>
>>> - line 29-30, 37, I don't see Constructor, BigInteger, and ArrayList
>>> being used?
>>>
>> JN: I must have removed them in my local copy of the code since I
>> posted the .02 webrev. They'll be gone in this next webrev.
>>>
>>> - line 158: the pbes2AlgorithmNameis initialized to null. Is it our
>>> intention to return null if toString() is called upon an PBES2
>>> AlgorithmParameters object without init(...) call?
>>>
>> JN: Hmm...that's a good question. In the case of PBES2 alg params
>> with a fully qualified name (PBEWith<prf>And<cipher>) the
>> pbes2AlgorithmName is initialized to whatever static class is
>> instantiated from SunJCE.java. So we don't have to worry there. In
>> the case of actually using "PBES2" (or its OID counterpart, which is
>> more common), it looks like even before my changes it returned null.
>> What do you think about returning "PBES2" instead? Once it goes
>> through init the field will be set to whatever it truly is based on
>> the DER encoding received.
>
> Sure, PBES2 is way better than just null...
>
>>> - line 176: does "keysize" means the optional keyLength field inside
>>> the PBES2-params struct? Its value is from various sources. A proper
>>> definition would help ensure its value is correctly set.
>>>
>> JN: Well, keysize does mean that, but it affects the resulting
>> toString name and can also have an impact on init-time consistency
>> checks in some cases.
>>
>> Keysize is one of the challenging parts of this class, because it can
>> be defined in multiple ways. It can be implicit (such as in DES or
>> DESede), it can be explicit by OID like it is for AES, and it can be
>> defined by parameters such as in RC2 - there is one OID, no matter
>> whether you do RC2_40, RC2_64 or RC2_128. Or it can be not defined
>> at all (RC5) in which case you have to assume a reasonable default
>> (like I assume RC5_128) or hope that the KDF parameter segment
>> asserts a key length. So it gets set in different parts of the code
>> depending on the algorithm and how it's specified.
>>
>> Maybe you can expand a bit on what you're looking for by "proper
>> definition" and I can try to make that happen.
>
> Well, one straightforward thought is to map "keysize" strictly based
> only on the optional keyLength field of the ASN.1 encoding. The
> keysize default value can be left to the particular cipher or key
> derivation impl.
>
JN: I think for certain ciphers, you run into a chicken/egg problem if
the key length is not defined. If you were using something with a
variable key length where that length can be defined in the encryption
scheme parameters (RC2 is the example I am thinking of), if the encoding
does not specify a PBKDF2 key length you cannot safely assume a default
value as your default length may not agree with what you later parse in
encryption scheme parameters section. From what I have seen for RC2,
openssl appears to always encode the key length, but that is just one
data point. I don't know how it would handle an inbound PBES2
parameters with an omitted key length though.
Since it is a "convenience" field, I'm hesitant to make it the only
source of information to use for the key length, especially where things
like RC2's parameters have a more clear definition of what effective key
bits means. Where the key length is already defined by the cipher/OID,
then a default value makes sense, and any declared value I would think
would have to be consistent. Having not (yet) tried to use edge cases
where we mismatch key length against the implicit value, it's hard to
know what to do here. I will give it a try and see what openssl does
with these kinds of PKCS#8 keys.
>>
>>> - line 207: If encoding to a DerOutputStream, why not decode using a
>>> DerInputStream?
>>>
>> JN: There's no reason why it couldn't be done that way. I used
>> DerValue mainly because decoding in PBES2Parameters has been done
>> with DerValue objects from before I started working with the code.
>> So I kept it the same since it seemed to work well.
> DerInputStream makes more sense than DerValue as DerValue contains tag
> and length. Essentially you just want to get the data you want off the
> stream just like where you directly put out the bytes to the stream in
> decoding case.
JN: Since we are only doing HMAC-SHA* variants that have no parameters,
and that's the only place ParamProcessor is used anymore, I'm just going
to remove the whole thing and directly handle the optional null in
parseKDF. I'll omit the NULL entirely in engineGetEncoded. If the
specs ever allow for PRFs that have parameters we can go back and deal
with ParamProcessor then. It made more sense to have it when encryption
schemes had their own ParamProcessors but that's handled by AlgorithmId
/ AlgorithmParameters now.
>>>
>>> - line 203: based on RFC3370, 5911, the preferred way for HMAC is to
>>> omit the parameters instead of encoding a NULL. So, we probably
>>> should not encode a null on line 203. Also, for decode, line 520 to
>>> 522 can be moved here so that the decode can handle both cases, i.e.
>>> omitted and present NULL, for max compatibility.
>>>
>> JN: From all the OpenSSL parameter blocks I've looked at, Hmac params
>> will assert NULL. There's some interesting history to
>> AlgorithmIdentifier's optional parameter field when there are no
>> parameters in RFC 4055, sec 2.1. In short, a change happened in the
>> 1997 syntax that removed "optional" which was later fixed, but by
>> then people were getting around the now mandatory field by putting
>> NULL in there. Honestly, I see NULLs asserted more commonly than it
>> being omitted for message digest and HMAC AlgorithmIdentifiers. But
>> that might just be me. According to 4055, omitting the parameters
>> field is the correct way to do it, you are correct about that.
>> Implementations are supposed to accept both forms. I stuck with it
>> because a.) we were already doing that and nobody was having problems
>> with it, b.) I see NULL asserted more frequently so it seemed like
>> the safer way to go.
>>
>> I can try omitting it and see if openssl will accept it. If it does
>> then I can make the change permanent. If it doesn't like it, we
>> should probably leave the NULL in there. I've got no idea how anyone
>> else encodes AlgorithmIdentifiers...maybe I can play around with NSS
>> and see what it does with Hmac parameters if I can find a tool that
>> will encode some.
>>
> Sure~
>>>
>>> - line 295 - 298: if there is a comment on line 271 which explains
>>> when "keyLen" is for, then we don't need this block of comment.
>>> Essentially, keyLen holds that restricted key length value, right?
>>> KEYLEN_ANY means no restriction whatsoever.
>>>
>> JN: Or it can mean that the key length is implicit (e.g. DES is
>> always 56 bits, so I don't need to encode it in ASN.1 and I don't
>> need toString to say DES_56...DES is fine).
>>
>> Maybe rather than a comment on 271, maybe a similar description for
>> KEYLEN_ANY up on 154 might be a good way to go.
>>>
>>> - line 376: the impl of validateEncParams() seems to allow null
>>> cipherParams as it returns immediately if its value is null. I am
>>> not sure if we should allow null cipherParam though as this
>>> cipherParam object is needed for encoding "encryptionScheme" field
>>> of the PBES2-params struct. For example, if the parameters field for
>>> AES_128_CBC must contain 16-byte IV, then a null cipherParams should
>>> be rejected. Same goes to other encryption schemes.
>>>
>> JN: There are tests in the PKCS#12 family of tests that were breaking
>> when I strictly enforced non-null cipherParams from the
>> PBEParameterSpec. I didn't want to risk messing up a use case in
>> PKCS#12-land that didn't apply directly to what I was fixing. What I
>> do need to do is have it fail on encoding when null parameters are
>> used, that will be in the next rev of the code. I hate not being
>> able to fail-fast, but it would at least remain consistent with how
>> PBES2Parameters works today.
> Hmm, interesting, maybe PKCS#12 uses PBES2 as PBES1? Well, I don't
> know the history of all this. Given that we are in RPD1 phase already,
> I understand with the approach you chose.
>>>
>>> - line 379: Since encyptionType is found using cipherAlgo and
>>> keysize in the constructor, why can't we just format
>>> encryptionType.schemeName as in getEncSchemeName()?
>>>
>> JN: Hmm...maybe we can do that. Let me give it a try and see what
>> happens.
>>>
>>> - line 419-422: Shouldn't this check be moved up to line 414, i.e.
>>> in the block of code which handles the buggy encoding? Otherwise, it
>>> looks like a duplicate check of line 400.
>>>
>> JN: I don't think so. I think you need both. On line 400,
>> regardless of the structure (PBES2 AlgorithmIdentifier vs.
>> PBES2-params) it must start with a sequence, so that check has to be
>> there. If it's out at the PBES2 AlgId layer (the error, which would
>> manifest itself as an OID as the next object in the DerValue stream)
>> then line 414 peels that away and what's next is your PBKDF2-params.
>>
>> Line 419 has to check if the next ASN.1 structure that sits as a peer
>> to PBKDF2-params is also a sequence, which it has to be, therefore
>> the check on the tag is performed before sending it in to be parsed.
>>
> The DER code can be confusing... Let me re-phrase my comments and
> questions.
>
> Line 413, 414, I think we should check for SEQUENCE tag on the newly
> peeled "pBES2_params", agree?
>
JN: Yes, I think that's a sensible check to perform.
>
> By calling data.getDerValue(), we are essentially peeling one layer
> off, right? If you still agree with me at this point, then note that
> pBES2_params is a local variable and its value should be the same
> unless explicitly re-assigned (as on line 413). Thus, per my reading
> of the code, the tag that you are checking on line 419 is not the one
> for encryption scheme, but rather the outer sequence tag encapsulating
> kdf and encryption scheme. Its current location is very misleading
> though, in between kdf and encryption scheme. To really check the tag
> for kdf and encryption scheme, the tag checking should be in
> parseKDF(...) and parseES(...) against the DerValue argument.
>
JN: Ah, I think I see what you're getting at. I think you may be right
- let me see if I can rework this a bit. It might make more sense to
start at the very beginning with a DerInputStream.getSequence and then
see what element zero is. if it's an OID, we can just skip to the next
object (the outer sequence) and do another DerInputStream.getSequence()
which should give us the two AlgorithmIdentifiers. That might be a more
straightforward parsing approach. Let me experiment a bit and see what
I can come up with.
> Thanks,
> Valerie
>>>
>>> - line 494, 630, 735: change the check to use KEYLEN_ANY?
>>>
>> JN: makes sense.
>>>
>>> -line 638: extra indentation?
>>>
>> JN: Yes, fixed.
>>>
>>> - line 725-752: Seems better to check the IV length before checking
>>> and set the keysize.
>>>
>> JN: OK I can switch the order on those.
>>>
>>> - line 788: May return RC2_-1 or RC5_-1?
>>>
>> JN: Hmm...maybe in the DER parsing case, now that
>> AlgorithmId/AlgorithmParameters handles the DER decoding I can't set
>> the keysize as part of the parsing process any longer. Let me take a
>> closer look at it and get back to you on that one. I think RC5 can
>> fall into that trap, not sure about RC2.
>>>
>>> Will send you comments for the rest of webrev separately.
>>>
>>> Thanks,
>>>
>>> Valerie
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> <src/java.base/share/classes/com/sun/crypto/provider/RC2Parameters.java>
>>>
>>>
>>> <src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java>
>>>
>>>
>>> <src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java>
>>>
>>>
>>> <test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java>
>>>
>>>
>>> <test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java>
>>>
>>>
>>> ||
>>>
>>> On 6/20/2019 6:59 PM, Jamil Nimeh wrote:
>>>> Hello all,
>>>>
>>>> I've updated the fix to 8076999 with the following changes:
>>>>
>>>> * We now use sun.security.x509.AlgorithmId and it internally uses
>>>> AlgorithmParameters implementations to handle the DER encoding
>>>> and decoding of encryption scheme parameters.
>>>> o This means that we need to add one new standard name and
>>>> some OID aliases for some AlgorithmParameters. See the CSR
>>>> link below for details.
>>>> * Added a new RC5Parameter AlgorithmParameters implementation to
>>>> SunJCE, plus unit tests.
>>>>
>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8221936
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8076999
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.02
>>>>
>>>>
>>>> On 5/24/2019 3:51 PM, Jamil Nimeh wrote:
>>>>>
>>>>> Hello all, happy Friday!
>>>>>
>>>>> Please review the following CSR and code review. This makes
>>>>> updates to the SunJCE implementation of PBES2-based
>>>>> AlgorithmParameters. Many of the details are in the CSR (see the
>>>>> link below). But a short list of the updates:
>>>>>
>>>>> * Add DER Encode/Decode support for the following OIDS from RFC
>>>>> 8018:
>>>>> o PRFs: HmacSHA512/224, HmacSHA512/256
>>>>> o Encryption Schemes: AES-192-CBC, DES, Triple-DES, RC2, RC5
>>>>> * Enforce init-time type consistency between
>>>>> AlgorithmParameterSpec objects and the algorithms they are
>>>>> used with (i.e. No using RC5ParameterSpec with AES-128-CBC.
>>>>> * Enforce sanity checks on AlgorithmParameterSpec objects used
>>>>> to init (e.g. IV length checks, integer range checks, etc.)
>>>>> * Fixed a bug where explicit DER decoding of the optional key
>>>>> length field in PBKDF2-params would cause the PRF to be forced
>>>>> to HmacSHA1 even if the DER indicated otherwise
>>>>> * Allow incoming DER encoded AlgorithmIdentifier structures to
>>>>> honor the OPTIONAL qualifier on the parameters field for both
>>>>> PRFs and Encryption Schemes.
>>>>> * If a null encryption scheme AlgorithmParameterSpec is provided
>>>>> during init time, omit the PBES2-params.encryptionScheme's
>>>>> parameter segment since it is OPTIONAL per the ASN.1 from RFC 5280
>>>>>
>>>>> More details are in the CSR.
>>>>>
>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8221936
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8076999
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.01/
>>>>>
>>>>> --Jamil
>>>>>
>>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190724/b3816e23/attachment.htm>
More information about the security-dev
mailing list