RFR 8076999: SunJCE support of password-based encryption scheme 2 params (PBES2) not working

Wed Jul 24 20:03:06 UTC 2019

Hi Valerie, I don't recall if I ever responded to you on this as I 
stepped away from this to work another issue.  Comments are in-line, and 
I have another email following this one with more because you had extra 
comments in that other email.

Thanks,
--Jamil

On 7/9/2019 7:44 PM, Valerie Peng wrote:
>
> <src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java>
>
> - line 174: considering add AES128/192/256 w/ CBC oids as they are 
> used by both AlgorithmParameters and Cipher entries.
>
JN: I'm not exactly sure what you're asking for here.  The 3 OIDs in 
that call to createAliasesWithOid are the OIDs for AES128/192/256-CBC.  
If you're asking me to use the variables "aes128Oid", "aes192Oid" and 
"aes256Oid" then I cannot use those just by themselves.  They'd end up 
having to get "2" appended to them, just as it is done for the 
AES/<nn>/CBC Cipher lines around 245-290.  Is there some advantage to 
doing that over what's currently there?  Am I missing the point of what 
you're asking for?
>
> <src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java>
>
> - line 108 seems redundant?
>
> - line 131-132: why not simply: (iv.length != (blockSizeInBits >> 3)?
>
JN: I like your approach much better, I'll fix that.
>
> - line 135: after checking iv.length, should we check for extra 
> trailing bytes and error out if (val.data.available() > 0)?
>
JN: It turns out not to be necessary.  The encapsulation of the DER 
encoded data into the DerValue as the first thing done already checks to 
make sure there's no extra data.  I did add a test to RC5ParametersTests 
that adds another block of DER-encoded data so at least we test that 
specific case explicitly.
>
> - line 191 encoder can be moved inside the block of iv-dumping code as 
> it's only used there.
>
JN: No problem.  Will do.
>
> <test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java>
>
> <test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java>
>
> - Can the tests be renamed? So it's clear that they are tests instead 
> of some utility classes for test.
>
JN: Sure, I can just make it <BASENAME>Tests.java
>
> - The ASN.1 encoding are generated offline using your changes? Just 
> curious.
>
JN: The ASN.1 encodings for PBES2Parameters was done using OpenSSL. For 
RC5Parameters, I did those by hand since OpenSSL doesn't automatically 
generate them through any tool like the pkcs8 command.  Fortunately 
they're small ASN.1 structures, so it's pretty easy to crank out with a 
hex editor.
> Thanks,
> Valerie
> 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?
>>
>> - 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?
>>
>> - 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.
>>
>> - line 207: If encoding to a DerOutputStream, why not decode using a 
>> DerInputStream?
>>
>> - 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.
>>
>> - 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.
>>
>> - 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.
>>
>> - line 379: Since encyptionType is found using cipherAlgo and keysize 
>> in the constructor, why can't we just format 
>> encryptionType.schemeName as in getEncSchemeName()?
>>
>> - 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.
>>
>> - line 494, 630, 735: change the check to use KEYLEN_ANY?
>>
>> -line 638: extra indentation?
>>
>> - line 725-752: Seems better to check the IV length before checking 
>> and set the keysize.
>>
>> - line 788: May return RC2_-1 or RC5_-1?
>>
>> 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/288c3b1e/attachment.htm>