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

Wed Jul 10 02:44:42 UTC 2019

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

<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)?

- line 135: after checking iv.length, should we check for extra trailing 
bytes and error out if (val.data.available() > 0)?

- line 191 encoder can be moved inside the block of iv-dumping code as 
it's only used there.

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

- The ASN.1 encoding are generated offline using your changes? Just curious.

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/20190709/6bc2aca1/attachment.htm>