<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Jamil,</p>
<p>Please find replies inline below...<br>
</p>
<div class="moz-cite-prefix">On 7/9/2019 9:03 AM, Jamil Nimeh wrote:<br>
</div>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Hi Valerie, thanks for the comments:<br>
<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>
</blockquote>
JN: I must have removed them in my local copy of the code since I
posted the .02 webrev. They'll be gone in this next webrev.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<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>
</blockquote>
JN: Hmm...that's a good question. In the case of PBES2 alg params
with a fully qualified name (PBEWith<prf>And<cipher>)
the pbes2AlgorithmName is initialized to whatever static class is
instantiated from SunJCE.java. So we don't have to worry there.
In the case of actually using "PBES2" (or its OID counterpart,
which is more common), it looks like even before my changes it
returned null. What do you think about returning "PBES2"
instead? Once it goes through init the field will be set to
whatever it truly is based on the DER encoding received.<br>
</blockquote>
<p>Sure, PBES2 is way better than just null...</p>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com">
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<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>
</blockquote>
JN: Well, keysize does mean that, but it affects the resulting
toString name and can also have an impact on init-time consistency
checks in some cases.<br>
<br>
Keysize is one of the challenging parts of this class, because it
can be defined in multiple ways. It can be implicit (such as in
DES or DESede), it can be explicit by OID like it is for AES, and
it can be defined by parameters such as in RC2 - there is one OID,
no matter whether you do RC2_40, RC2_64 or RC2_128. Or it can be
not defined at all (RC5) in which case you have to assume a
reasonable default (like I assume RC5_128) or hope that the KDF
parameter segment asserts a key length. So it gets set in
different parts of the code depending on the algorithm and how
it's specified.<br>
<br>
Maybe you can expand a bit on what you're looking for by "proper
definition" and I can try to make that happen.<br>
</blockquote>
<p>Well, one straightforward thought is to map "keysize" strictly
based only on the optional keyLength field of the ASN.1 encoding.
The keysize default value can be left to the particular cipher or
key derivation impl. <br>
</p>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com"> <br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p>- line 207: If encoding to a DerOutputStream, why not decode
using a DerInputStream? <br>
</p>
</blockquote>
JN: There's no reason why it couldn't be done that way. I used
DerValue mainly because decoding in PBES2Parameters has been done
with DerValue objects from before I started working with the
code. So I kept it the same since it seemed to work well.<br>
</blockquote>
DerInputStream makes more sense than DerValue as DerValue contains
tag and length. Essentially you just want to get the data you want
off the stream just like where you directly put out the bytes to the
stream in decoding case.<br>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com">
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p> </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>
</blockquote>
JN: From all the OpenSSL parameter blocks I've looked at, Hmac
params will assert NULL. There's some interesting history to
AlgorithmIdentifier's optional parameter field when there are no
parameters in RFC 4055, sec 2.1. In short, a change happened in
the 1997 syntax that removed "optional" which was later fixed, but
by then people were getting around the now mandatory field by
putting NULL in there. Honestly, I see NULLs asserted more
commonly than it being omitted for message digest and HMAC
AlgorithmIdentifiers. But that might just be me. According to
4055, omitting the parameters field is the correct way to do it,
you are correct about that. Implementations are supposed to
accept both forms. I stuck with it because a.) we were already
doing that and nobody was having problems with it, b.) I see NULL
asserted more frequently so it seemed like the safer way to go.<br>
<br>
I can try omitting it and see if openssl will accept it. If it
does then I can make the change permanent. If it doesn't like it,
we should probably leave the NULL in there. I've got no idea how
anyone else encodes AlgorithmIdentifiers...maybe I can play around
with NSS and see what it does with Hmac parameters if I can find a
tool that will encode some.<br>
<br>
</blockquote>
Sure~<br>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com">
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<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>
</blockquote>
JN: Or it can mean that the key length is implicit (e.g. DES is
always 56 bits, so I don't need to encode it in ASN.1 and I don't
need toString to say DES_56...DES is fine).<br>
<br>
Maybe rather than a comment on 271, maybe a similar description
for KEYLEN_ANY up on 154 might be a good way to go.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<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>
</blockquote>
JN: There are tests in the PKCS#12 family of tests that were
breaking when I strictly enforced non-null cipherParams from the
PBEParameterSpec. I didn't want to risk messing up a use case in
PKCS#12-land that didn't apply directly to what I was fixing.
What I do need to do is have it fail on encoding when null
parameters are used, that will be in the next rev of the code. I
hate not being able to fail-fast, but it would at least remain
consistent with how PBES2Parameters works today.<br>
</blockquote>
Hmm, interesting, maybe PKCS#12 uses PBES2 as PBES1? Well, I don't
know the history of all this. Given that we are in RPD1 phase
already, I understand with the approach you chose.<br>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com">
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p> </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>
</blockquote>
JN: Hmm...maybe we can do that. Let me give it a try and see what
happens.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p><span class="changed"> </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>
</blockquote>
JN: I don't think so. I think you need both. On line 400,
regardless of the structure (PBES2 AlgorithmIdentifier vs.
PBES2-params) it must start with a sequence, so that check has to
be there. If it's out at the PBES2 AlgId layer (the error, which
would manifest itself as an OID as the next object in the DerValue
stream) then line 414 peels that away and what's next is your
PBKDF2-params.<br>
<br>
Line 419 has to check if the next ASN.1 structure that sits as a
peer to PBKDF2-params is also a sequence, which it has to be,
therefore the check on the tag is performed before sending it in
to be parsed.<br>
<br>
</blockquote>
<p>The DER code can be confusing... Let me re-phrase my comments and
questions.</p>
<p>Line 413, 414, I think we should check for SEQUENCE tag on the
newly peeled "pBES2_params", agree?</p>
<p>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.</p>
Thanks,<br>
Valerie<br>
<blockquote type="cite"
cite="mid:ac5a11ca-4d07-130b-a6fb-3fb39751079a@oracle.com">
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p>- line 494, 630, 735: change the check to use KEYLEN_ANY?</p>
</blockquote>
JN: makes sense.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p>-line 638: extra indentation?</p>
</blockquote>
JN: Yes, fixed.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p>- line 725-752: Seems better to check the IV length before
checking and set the keysize.<br>
</p>
</blockquote>
JN: OK I can switch the order on those.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p> </p>
<p>- line 788: May return RC2_-1 or RC5_-1?<br>
</p>
</blockquote>
JN: Hmm...maybe in the DER parsing case, now that
AlgorithmId/AlgorithmParameters handles the DER decoding I can't
set the keysize as part of the parsing process any longer. Let me
take a closer look at it and get back to you on that one. I think
RC5 can fall into that trap, not sure about RC2.<br>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<p> </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>
<br>
</blockquote>
</body>
</html>