<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Valerie, part 2 of my comments (also in-line):<br>
    <br>
    Thanks,<br>
    --Jamil<br>
    <br>
    <div class="moz-cite-prefix">On 7/9/2019 8:59 PM, Valerie Peng
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:d30f4f63-a0c8-6b7b-232b-09b802c37ee7@oracle.com">
      <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
      <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>
    JN: I think for certain ciphers, you run into a chicken/egg problem
    if the key length is not defined.  If you were using something with
    a variable key length where that length can be defined in the
    encryption scheme parameters (RC2 is the example I am thinking of),
    if the encoding does not specify a PBKDF2 key length you cannot
    safely assume a default value as your default length may not agree
    with what you later parse in encryption scheme parameters section. 
    From what I have seen for RC2, openssl appears to always encode the
    key length, but that is just one data point.  I don't know how it
    would handle an inbound PBES2 parameters with an omitted key length
    though.<br>
    <br>
    Since it is a "convenience" field, I'm hesitant to make it the only
    source of information to use for the key length, especially where
    things like RC2's parameters have a more clear definition of what
    effective key bits means.  Where the key length is already defined
    by the cipher/OID, then a default value makes sense, and any
    declared value I would think would have to be consistent.  Having
    not (yet) tried to use edge cases where we mismatch key length
    against the implicit value, it's hard to know what to do here.  I
    will give it a try and see what openssl does with these kinds of
    PKCS#8 keys.<br>
    <br>
    <blockquote type="cite"
      cite="mid:d30f4f63-a0c8-6b7b-232b-09b802c37ee7@oracle.com">
      <p> </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>
    JN: Since we are only doing HMAC-SHA* variants that have no
    parameters, and that's the only place ParamProcessor is used
    anymore, I'm just going to remove the whole thing and directly
    handle the optional null in parseKDF.  I'll omit the NULL entirely
    in engineGetEncoded.  If the specs ever allow for PRFs that have
    parameters we can go back and deal with ParamProcessor then.  It
    made more sense to have it when encryption schemes had their own
    ParamProcessors but that's handled by AlgorithmId /
    AlgorithmParameters now.<br>
    <blockquote type="cite"
      cite="mid:d30f4f63-a0c8-6b7b-232b-09b802c37ee7@oracle.com">
      <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>
    </blockquote>
    JN: Yes, I think that's a sensible check to perform.<br>
    <blockquote type="cite"
      cite="mid:d30f4f63-a0c8-6b7b-232b-09b802c37ee7@oracle.com">
      <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>
    </blockquote>
    JN: Ah, I think I see what you're getting at.  I think you may be
    right - let me see if I can rework this a bit.  It might make more
    sense to start at the very beginning with a
    DerInputStream.getSequence and then see what element zero is.  if
    it's an OID, we can just skip to the next object (the outer
    sequence) and do another DerInputStream.getSequence() which should
    give us the two AlgorithmIdentifiers.  That might be a more
    straightforward parsing approach.  Let me experiment a bit and see
    what I can come up with.<br>
    <br>
    <blockquote type="cite"
      cite="mid:d30f4f63-a0c8-6b7b-232b-09b802c37ee7@oracle.com">
      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>
    </blockquote>
    <br>
  </body>
</html>