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
