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

Mon Jun 17 18:32:15 UTC 2019

Hi Valerie, thank you for all the helpful comments Valerie, comments 
in-line:

On 6/13/2019 7:11 PM, Valerie Peng wrote:
>
> 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.
>
JN: I think we can make this work using AESParameters, DESParameters, 
DESedeParameters and RC2Parameters.  I had originally tried to avoid 
making and exposing a new AlgorithmParameters standard name to 
accommodate RC5, but if we move to using those to do the parsing then I 
may as well.  Most of the code I've written should carry over.
>
> 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".
>
JN: Yes, good call.  I'll do that.
>
> 3. validateRC2PS(), line 987, possible NPE? We should check and reject 
> null IV as CBC mode requires IV. However RC2ParameterSpec may return 
> null IV.
>
JN: An NPE can't happen here, because the calling function checks for a 
null cipherParam field and throws an exception if it is not provided.  
But IIRC I was trying to handle cases where init(AlgorithmParameterSpec) 
was called with a null parameter (which does happen, BTW...some PKCS#12 
unit tests require it).  But I don't allow a null RC2ParameterSpec.  
Maybe I need to.  RFC 8018 already says that a missing effective key 
bits should default to 32 and a missing IV parameter spec *could* be 
interpreted by an AlgorithmParameters consumer as all zeroes.

Before I started on this fix we allowed people to init with a null APS 
and we would return a PBEParameterSpec with a null value for the 
underlying cipher parameter spec.  I don't think I want to change that 
behavior.

Long story short, if I do decide to allow a null RC2ParameterSpec on the 
way in, I do need to check for a null as you suggest.
>
> 4. validateRC5PS(), line 1008 - 1014: we already have same check 
> inside javax.crypto.spec.RC5ParameterSpec, so we don't have to 
> duplicate it here.
>
JN: Yes, you're right.  I can remove this.
>
> 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.
>
JN: Well, here was my thinking on this one.  Right now every PRF we work 
with doesn't need parameters and always encodes it using an ASN.1 NULL 
in the AlgorithmIdentifier parameters field.  When I made the PrfType 
enum, I didn't want to build it in such a way that didn't account for 
the possibility (remote, I will admit) that we might get a PRF down the 
line that needs one, like maybe HmacSHA512T (the one where you can 
truncate to a variable length).

In making the ParamProcessor interface, I wanted it to be generic to any 
AlgorithmIdentifier param implementation, and some of these need 
parameters (the encryption ones) and some don't (the PRFs currently).  
So when I got down to the line you cited, I took a short cut and just 
threw the null in there for the time being, since even the new PRF algs 
I've added support for don't need any AlgorithmParameterSpec.  I figured 
I could do that for now for simplicity and if we ever got a PRF that 
needs parameters we could rework the logic.  Also it is worth noting 
that more would have to change to even support PRFs with parameters, as 
we have no way through our public PBES2 interfaces to provide them today.

With the changes from your #1 comment above, I might not need a 
ParamProcessor interface any longer since every PRF we support takes no 
parameters and there's no public interface to provide them yet, and all 
the encryption schemes would be handled in separate classes.  Scaling 
this back seems like the better way to go.

> 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/20190617/a701bc79/attachment.htm>