<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java></p>
<p>- line 174: considering add AES128/192/256 w/ CBC oids as they
are used by both AlgorithmParameters and Cipher entries. </p>
<p><src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java></p>
<p>- line 108 seems redundant? <br>
</p>
<p>- line 131-132: why not simply: (iv.length != (blockSizeInBits
>> 3)?<br>
</p>
<p>- line 135: after checking iv.length, should we check for extra
trailing bytes and error out if (val.data.available() > 0)?</p>
<p>- line 191 encoder can be moved inside the block of iv-dumping
code as it's only used there.<br>
</p>
<p> </p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java></p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java></p>
<p>- Can the tests be renamed? So it's clear that they are tests
instead of some utility classes for test.</p>
<p>- The ASN.1 encoding are generated offline using your changes?
Just curious.</p>
Thanks,<br>
Valerie<br>
<div class="moz-cite-prefix">On 7/8/2019 4:50 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Hi Jamil,</p>
<p>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.<br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java></p>
<p>- line 29-30, 37, I don't see Constructor, BigInteger, and
ArrayList being used?</p>
<p>- 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?</p>
<p>- 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.</p>
<p>- line 207: If encoding to a DerOutputStream, why not decode
using a DerInputStream? <br>
</p>
<p>- 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.</p>
<p>- 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.</p>
<p>- 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.<br>
</p>
<p>- line 379: Since encyptionType is found using cipherAlgo and
keysize in the constructor, why can't we just format <span
class="changed">encryptionType.schemeName as in</span> <span
class="changed">getEncSchemeName()? </span><span
class="changed"><br>
</span></p>
<p>- 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.</p>
<p>- line 494, 630, 735: change the check to use KEYLEN_ANY?</p>
<p>-line 638: extra indentation?</p>
<p>- line 725-752: Seems better to check the IV length before
checking and set the keysize.<br>
</p>
<p>- line 788: May return RC2_-1 or RC5_-1?<br>
</p>
<p>Will send you comments for the rest of webrev separately.</p>
<p>Thanks,</p>
<p>Valerie<br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/RC2Parameters.java></p>
<p><br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java></p>
<p><br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java></p>
<p><br>
</p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java></p>
<p><br>
</p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java><br>
</p>
<p><br>
<code></code></p>
<div class="moz-cite-prefix">On 6/20/2019 6:59 PM, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bd96ce08-18d5-74bc-045f-3ed48e534fff@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
Hello all,<br>
<br>
I've updated the fix to 8076999 with the following changes:<br>
<ul>
<li>We now use sun.security.x509.AlgorithmId and it internally
uses AlgorithmParameters implementations to handle the DER
encoding and decoding of encryption scheme parameters.<br>
</li>
<ul>
<li>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.</li>
</ul>
<li>Added a new RC5Parameter AlgorithmParameters
implementation to SunJCE, plus unit tests.</li>
</ul>
<p>CSR: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8221936"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8221936</a></p>
<p>Bug: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8076999"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8076999</a></p>
<p>Webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.02"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.02</a></p>
<br>
<div class="moz-cite-prefix">On 5/24/2019 3:51 PM, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6afc52c5-cf29-827a-0591-27714bcd6068@oracle.com">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<p>Hello all, happy Friday!<br>
</p>
<p>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:</p>
<ul>
<li>Add DER Encode/Decode support for the following OIDS
from RFC 8018:<br>
</li>
<ul>
<li>PRFs: HmacSHA512/224, HmacSHA512/256</li>
<li>Encryption Schemes: AES-192-CBC, DES, Triple-DES, RC2,
RC5</li>
</ul>
<li>Enforce init-time type consistency between
AlgorithmParameterSpec objects and the algorithms they are
used with (i.e. No using RC5ParameterSpec with
AES-128-CBC.</li>
<li>Enforce sanity checks on AlgorithmParameterSpec objects
used to init (e.g. IV length checks, integer range checks,
etc.)</li>
<li>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</li>
<li>Allow incoming DER encoded AlgorithmIdentifier
structures to honor the OPTIONAL qualifier on the
parameters field for both PRFs and Encryption Schemes.</li>
<li>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</li>
</ul>
<p>More details are in the CSR.<br>
</p>
<p>CSR: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8221936"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8221936</a></p>
<p>Bug: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8076999"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8076999</a></p>
<p>Webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.01/</a></p>
<p>--Jamil<br>
</p>
<p><br>
</p>
</blockquote>
<br>
</blockquote>
</blockquote>
</body>
</html>