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

[Jamil Nimeh]

Hello all,

I'm finally getting back around to this after dusting off the cobwebs.

Webrev: https://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.03

Bug: https://bugs.openjdk.java.net/browse/JDK-8076999

Valerie, you had some comments from way back (7/9/2019).  Just a short 
summary of what's been done to address them:

  * Removed unused imports
  * Added a default "PBES2" string value for when the toString method is
    called on a PBES2Parameters object before the init() method.
  * Tested encoding of PBKDF2 parameters AlgorithmIdentifiers with the
    optional parameters field not present (as opposed to an ASN.1
    NULL).  OpenSSL seems happy with it so that's how we'll encode those.
  * Switched order on the IV and keysize parsing for RC2 parameters
  * Using KEYLEN_ANY (changed to KEYLEN_UNSPEC) now in lieu of -1 in the
    conditionals you cited.
  * For algorithms where the key length is implicit either due to the
    algorithm or the specific OID, we no longer assert the key length in
    the KDF parameters.  This is consistent with other implementations
    such as OpenSSL.
  * Regarding the comment from the parsing in engineInit(byte[])

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

I did add a DerValue check in parseKDF because it is appropriate as you 
stated.  I also removed the check from line 419 in the old webrev.  With 
the new code that check is redundant as we are using AlgorithmId.parse() 
now as the initial operation in parseES, which in turn does the sequence 
tag check for us.

The check itself on 419 though is testing the ASN.1 tag for the 
Encryption scheme, not the higher level sequence for PBES2-params.  
Otherwise neither the KDF nor the encryption scheme would parse properly 
and none of the tests would pass.

With this new code, parseKDF and parseES are testing the outer SEQUENCE 
tags for each of the AlgorithmIdentifier objects described by 
keyDerivationFunc and encryptionScheme per RFC 8018.

  * keysize setting: This one is a bit tricky because key size is
    specified in multiple ways.  The basic flow is this.  Key size will
    start as KEYLEN_UNSPEC.  If it is at any point specified, either via
    the constructor, by KDF params, or by enc params then that is the
    value that is set.  At near the end of parsing a validation of the
    encryption parameters occurs and at that point the key length is
    checked against the algorithm.  If it is an alg that uses a fixed
    value then keysize has to be consistent with it.  If it is variable
    length and it is still KEYSIZE_UNSPEC that is also a failure (since
    something needs to be specified to distinguish RC2_40 from RC2_128,
    for example).
      o This approach seems to work and catches the issue you found
        where certain DER encodings could yield things like
        PBEWithHmacSHA256AndRC5_-1.
  * Added some new tests to handle the changes listed above.

I'll be updating the CSR shortly to reflect comments there and I'll send 
a separate review notice for that.

Thanks,

--Jamil

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200317/b08d582c/attachment.htm>