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

[Valerie Peng]

Right, I recall reviewing this and made some comments. Will take a look 
at the updated webrev.

Thanks,
Valerie
On 3/17/2020 4:48 PM, Jamil Nimeh wrote:
>
> 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/20200318/c6dddcd8/attachment.htm>