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

[Valerie Peng]

Hi, Jamil,

Here are some comments:

1. ParamProcessor and its various implementations: Essentially, you are 
doing the same thing as parsing/encoding of AlgorithmParameters for 
various algorithms. For RC2, we already have code for doing this and it 
seems that for max consistency and interoperability, we should try to 
leverage what we have instead of writing new code. As RC2_CBC can be 
considered as one usage case of RC2Parameters, I think we can do without 
the new rc2ParamProcessor class and just add PBES2-specific checking 
once the parsing is done. For RC5 parameters which isn't currently 
supported, we can add support for AlgorithmParameters. Or, another 
alternative is to put these encoding/decoding code into a separate class 
and refactor existing code to all call into this new class, and do 
PBES2-specific checking inside PBES2Parameters.

2. line 169, move the constant KEYLEN_ANY up to where the other 
constants are defined, i.e. ~line149. line 170, I think you mean to 
initialize keysize as KEYLEN_ANY? I think it'll be clearer to do so, 
instead of just "-1".

3. validateRC2PS(), line 987, possible NPE? We should check and reject 
null IV as CBC mode requires IV. However RC2ParameterSpec may return 
null IV.

4. validateRC5PS(), line 1008 - 1014: we already have same check inside 
javax.crypto.spec.RC5ParameterSpec, so we don't have to duplicate it here.

5. line 886 - 888: seems strange that you check that kdfType.prfParams 
is not null, but then hard code the 1st argument to be null when calling 
kdfType.prfParams.encode(...). When setting up these enums, you already 
construct them with nullParamProcessor, but yet when calling the 
encode(...), the caller has to know to pass null argument again, seems a 
bit redundant to define a nullParamProcessor but then still have to pass 
null.

I am still reviewing the rest of changes, but thought that I will 
forward you what I have so you have more time to think about it.

Thanks,

Valerie


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/20190613/a87910f8/attachment.htm>