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

[Valerie Peng]

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/20190708/065c84cf/attachment.htm>