Hi Jamil,
I looked at java.security.AlgorithmParameters and need to update my
earlier comment below
- By convention, each init() is a fresh start and wipes out the effect
previous init() calls. But in the current webrev, they seems to apply
changes on top of each other. This may not be the right model of how
things should be handled. In addition, with the existing code handles
both PBES2 and PBEwith<KDF>And<CIPHER>, we may need extra logic to
restore the fields back to when they are first constructed at the
beginning of every engineInit(...).
It turns out that AlgorithmParameters have an "initialized" flag which
would only accept one successful init() call.
Subsequent call would be rejected by throwing either IOException or
InvalidParameterSpecException. So, I guess we don't need to worry about
multiple init() calls. Just that the supplied data is legit and is not
conflicting with the assumed algorithm values if the object is requested
under the "friendly" name.
Thanks,
Valerie
On 3/30/2020 8:25 PM, Valerie Peng wrote:
Hi Jamil,
Thanks for being so patient. It take me sometime to play around with
the changes and think about various scenarios...
Here are some comments:
<RC5Parameters.java>
- Line 38 has RFC 2268 which is for RC2, RC5 is in RFC 2040.
- Line 48-51 comments can be simplified further, essentially, IV
(provided or not) shall have the same length as the block size. If not
provided explicitly, then its values are all 0s. Just some nit.
<RC5ParametersTests.java>
- line 187: variable name 'pbeSpec' should probably be named as
'rc5Spec' as it has nothing to do with PBE.
<PBES2ParametersTests.java>
- The test is very extensive... May I ask how are the test vectors
generated? Are they from some RFCs or external website like NIST or
internally generated using these new impl? In particular, I wonder
about the test "HmacSHA256 and AES-256-CBC [No Enc parameters]", this
is expected to pass, but AES CBC requires IV and if no parameters are
explicitly specified, how do things work for decryption where IV is
required and cannot generate random values as default IV? I am also
not sure about the InitByDERAndEncodeTest, I think we should not alter
the given encoding based on our own assumptions/defaults. If given a
DER encoding bytes, the same (minor difference such as an absent null
params is ok) encoding should be returned when getEncoded() is called.
- Line 143 expVals[4] should be expVals[3]?
<PBES2Parameters.java>
- Current changes are a bit too complicated and I am not sure if it's
worthwhile to support RC2, RC5, DESede and DES and do their
algorithm-specific checking. PBES2 AlgorithmParameters parsing is
already complicated due we support the original name (PBES2) and also
the "friendly" JCA naming convention of including the cipher name and
KDF name. On top of it, the current webrev seem to trying to support
parsing of all possible algorithms stated in PKCS#5 even when SunJCE
provider only support the AES_xxx variants. If only parsing is done,
the overhead may be acceptable, but then when there is also algorithm
specific checking, I feel this is a bit much as I doubt that they will
ever be used.
- There are a lot of String parsing inside the String-arg constructor
which can be avoided if we replace this String-arg constructor with a
2-arg constructor with PrfType and EncType. This should simplify the
constructor code greatly and make it more robust. Then we probably can
remove the getByName() method for both enum types. No need for the
constructor to throw NoSuchAlgorithmException as all inputs are
provided by provider and unsupported algorithm should be detected
before calling this constructor.
- Line 259, add "," after the word "key"?
- The ordering of things under engineInit(AlgorithmParameterSpec
paramSpec) seems a bit un-intuitive. User-supplied values should only
be stored after pass validation. The assignment (line 324-326) should
be done after the various checks. The check at line 338 should be
moved up before assigning the default kdf type.
- Missing PBE subclasses for AES_192?
- Some of the static oid constants seems unnecessary as they are only
used inside the enum and can be moved there.
- By convention, each init() is a fresh start and wipes out the effect
previous init() calls. But in the current webrev, they seems to apply
changes on top of each other. This may not be the right model of how
things should be handled. In addition, with the existing code handles
both PBES2 and PBEwith<KDF>And<CIPHER>, we may need extra logic to
restore the fields back to when they are first constructed at the
beginning of every engineInit(...).
- I am not too sure about the usefulness of "pbes2AlgorithmName"
field. In the current impl, it is set in various places. If it is
meant to reflect the latest KDF and CIPHER algorithm used, it's more
robust to construct its value when needed. Otherwise, we need to
remember updating this value whenever one of these 3 values, i.e. KDF,
CIPHER, and keysize, changed.
- Consider grouping the fields of salt, iteration count, keysize,
prfType into separate class and move the PBKDF2 parsing/encoding code
there. This simplifies the validation and setting of these 4 fields.
- Avoid algorithm-specific checking in this class as it is not
scalable and duplicates the checking in the algorithm-specific
classes. If you feel they must be done, delegate to the
algorithm-specific classes as much as possible. Instead of explicitly
checking the parameter spec, use the sequence of
AlgorithmParameters.getInstance(String), and its init(...) call and
see if the call passes.
- There are also code which sets the keysize for RC2 and RC5 key sizes
based on the PBKDF2 and cipher parameters. I think it's reasonable to
derive the value from the PBKDF2 params, but not cipher parameters. In
the case of RC5, it even assigns a default value (line 760) when all
else is failed. Given the purpose of this class is for PBES2 algorithm
parameters and we don't support PBES2 cipher with RC2 or RC5, I think
we should not go this far. Is there anything that I missed for
requiring to set the keysize in this class? If "PBES2"
AlgorithmParameters are requested and initialized with DER encoding,
we should return the same encoding (unless it's mistakenly encoded as
stated in line ) when getEncoded() is called. It's also somewhat
strange that the toString() method returns only the "friendly"
expanded name without other info. This is different from other
AlgorithmParameters impl. But this is just nit comparing to other things.
I adapted your changes with most of my feedback above (except the one
on each engineInit() call being independent) and you can find the
changes here: http://cr.openjdk.java.net/~valeriep/pbes2Exp/webrev/
(The regression test PBES2ParametersTest.java has to be updated a
little in order to pass). Hope this can help you understand my comments.
Thanks!
Valerie
On 3/18/2020 3:59 PM, Valerie Peng wrote:
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
