Hi Valerie, I don't recall if I ever responded to you on this as I
stepped away from this to work another issue. Comments are in-line, and
I have another email following this one with more because you had extra
comments in that other email.
Thanks,
--Jamil
On 7/9/2019 7:44 PM, Valerie Peng wrote:
<src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java>
- line 174: considering add AES128/192/256 w/ CBC oids as they are
used by both AlgorithmParameters and Cipher entries.
JN: I'm not exactly sure what you're asking for here. The 3 OIDs in
that call to createAliasesWithOid are the OIDs for AES128/192/256-CBC.
If you're asking me to use the variables "aes128Oid", "aes192Oid" and
"aes256Oid" then I cannot use those just by themselves. They'd end up
having to get "2" appended to them, just as it is done for the
AES/<nn>/CBC Cipher lines around 245-290. Is there some advantage to
doing that over what's currently there? Am I missing the point of what
you're asking for?
<src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java>
- line 108 seems redundant?
- line 131-132: why not simply: (iv.length != (blockSizeInBits >> 3)?
JN: I like your approach much better, I'll fix that.
- line 135: after checking iv.length, should we check for extra
trailing bytes and error out if (val.data.available() > 0)?
JN: It turns out not to be necessary. The encapsulation of the DER
encoded data into the DerValue as the first thing done already checks to
make sure there's no extra data. I did add a test to RC5ParametersTests
that adds another block of DER-encoded data so at least we test that
specific case explicitly.
- line 191 encoder can be moved inside the block of iv-dumping code as
it's only used there.
JN: No problem. Will do.
<test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java>
<test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java>
- Can the tests be renamed? So it's clear that they are tests instead
of some utility classes for test.
JN: Sure, I can just make it <BASENAME>Tests.java
- The ASN.1 encoding are generated offline using your changes? Just
curious.
JN: The ASN.1 encodings for PBES2Parameters was done using OpenSSL. For
RC5Parameters, I did those by hand since OpenSSL doesn't automatically
generate them through any tool like the pkcs8 command. Fortunately
they're small ASN.1 structures, so it's pretty easy to crank out with a
hex editor.
Thanks,
Valerie
On 7/8/2019 4:50 PM, Valerie Peng wrote:
Hi Jamil,
Sorry for the late reply. It's been a long while since I looked at
this PBES2 scheme and I need to think a few things through.
<src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java>
- line 29-30, 37, I don't see Constructor, BigInteger, and ArrayList
being used?
- line 158: the pbes2AlgorithmNameis initialized to null. Is it our
intention to return null if toString() is called upon an PBES2
AlgorithmParameters object without init(...) call?
- line 176: does "keysize" means the optional keyLength field inside
the PBES2-params struct? Its value is from various sources. A proper
definition would help ensure its value is correctly set.
- line 207: If encoding to a DerOutputStream, why not decode using a
DerInputStream?
- line 203: based on RFC3370, 5911, the preferred way for HMAC is to
omit the parameters instead of encoding a NULL. So, we probably
should not encode a null on line 203. Also, for decode, line 520 to
522 can be moved here so that the decode can handle both cases, i.e.
omitted and present NULL, for max compatibility.
- line 295 - 298: if there is a comment on line 271 which explains
when "keyLen" is for, then we don't need this block of comment.
Essentially, keyLen holds that restricted key length value, right?
KEYLEN_ANY means no restriction whatsoever.
- line 376: the impl of validateEncParams() seems to allow null
cipherParams as it returns immediately if its value is null. I am not
sure if we should allow null cipherParam though as this cipherParam
object is needed for encoding "encryptionScheme" field of the
PBES2-params struct. For example, if the parameters field for
AES_128_CBC must contain 16-byte IV, then a null cipherParams should
be rejected. Same goes to other encryption schemes.
- line 379: Since encyptionType is found using cipherAlgo and keysize
in the constructor, why can't we just format
encryptionType.schemeName as in getEncSchemeName()?
- line 419-422: Shouldn't this check be moved up to line 414, i.e. in
the block of code which handles the buggy encoding? Otherwise, it
looks like a duplicate check of line 400.
- line 494, 630, 735: change the check to use KEYLEN_ANY?
-line 638: extra indentation?
- line 725-752: Seems better to check the IV length before checking
and set the keysize.
- line 788: May return RC2_-1 or RC5_-1?
Will send you comments for the rest of webrev separately.
Thanks,
Valerie
<src/java.base/share/classes/com/sun/crypto/provider/RC2Parameters.java>
<src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java>
<src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java>
<test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java>
<test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java>
||
On 6/20/2019 6:59 PM, Jamil Nimeh wrote:
Hello all,
I've updated the fix to 8076999 with the following changes:
* We now use sun.security.x509.AlgorithmId and it internally uses
AlgorithmParameters implementations to handle the DER encoding
and decoding of encryption scheme parameters.
o This means that we need to add one new standard name and
some OID aliases for some AlgorithmParameters. See the CSR
link below for details.
* Added a new RC5Parameter AlgorithmParameters implementation to
SunJCE, plus unit tests.
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.02
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
