8246797: A convenient method to read OPTIONAL element

Reviewed-by: jnimeh, valeriep

src/java.base/share/classes/com/sun/crypto/provider/OAEPParameters.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -40,14 +40,15 @@ import javax.crypto.spec.OAEPParameterSpec;
 /**
  * This class implements the OAEP parameters used with the RSA
  * algorithm in OAEP padding. Here is its ASN.1 definition:
+ * <pre>
  * RSAES-OAEP-params ::= SEQUENCE {
  *   hashAlgorithm      [0] HashAlgorithm     DEFAULT sha1,
  *   maskGenAlgorithm   [1] MaskGenAlgorithm  DEFAULT mgf1SHA1,
  *   pSourceAlgorithm   [2] PSourceAlgorithm  DEFAULT pSpecifiedEmpty
  * }
+ * </pre>
  *
  * @author Valerie Peng
- *
  */
 
 public final class OAEPParameters extends AlgorithmParametersSpi {
@@ -91,61 +92,48 @@ public final class OAEPParameters extends AlgorithmParametersSpi {
         }
     }
 
-    protected void engineInit(byte[] encoded)
+    protected void engineInit(byte[] encoded) throws IOException {
-        throws IOException {
+
-        DerInputStream der = new DerInputStream(encoded);
+        DerInputStream der = DerValue.wrap(encoded).data();
+        var sub = der.getOptionalExplicitContextSpecific(0);
+        if (sub.isPresent()) {
+            mdName = AlgorithmId.parse(sub.get()).getName();
+        } else {
             mdName = "SHA-1";
-        mgfSpec = MGF1ParameterSpec.SHA1;
+        }
-        p = new byte[0];
+        sub = der.getOptionalExplicitContextSpecific(1);
-        DerValue[] datum = der.getSequence(3);
+        if (sub.isPresent()) {
-        for (int i=0; i<datum.length; i++) {
+            AlgorithmId val = AlgorithmId.parse(sub.get());
-            DerValue data = datum[i];
-            if (data.isContextSpecific((byte) 0x00)) {
-                // hash algid
-                mdName = AlgorithmId.parse
-                    (data.data.getDerValue()).getName();
-            } else if (data.isContextSpecific((byte) 0x01)) {
-                // mgf algid
-                AlgorithmId val = AlgorithmId.parse(data.data.getDerValue());
             if (!val.getOID().equals(OID_MGF1)) {
                 throw new IOException("Only MGF1 mgf is supported");
             }
             AlgorithmId params = AlgorithmId.parse(
                     new DerValue(val.getEncodedParams()));
-                String mgfDigestName = params.getName();
+            mgfSpec = switch (params.getName()) {
-                if (mgfDigestName.equals("SHA-1")) {
+                case "SHA-1" -> MGF1ParameterSpec.SHA1;
-                    mgfSpec = MGF1ParameterSpec.SHA1;
+                case "SHA-224" -> MGF1ParameterSpec.SHA224;
-                } else if (mgfDigestName.equals("SHA-224")) {
+                case "SHA-256" -> MGF1ParameterSpec.SHA256;
-                    mgfSpec = MGF1ParameterSpec.SHA224;
+                case "SHA-384" -> MGF1ParameterSpec.SHA384;
-                } else if (mgfDigestName.equals("SHA-256")) {
+                case "SHA-512" -> MGF1ParameterSpec.SHA512;
-                    mgfSpec = MGF1ParameterSpec.SHA256;
+                case "SHA-512/224" -> MGF1ParameterSpec.SHA512_224;
-                } else if (mgfDigestName.equals("SHA-384")) {
+                case "SHA-512/256" -> MGF1ParameterSpec.SHA512_256;
-                    mgfSpec = MGF1ParameterSpec.SHA384;
+                default -> throw new IOException(
-                } else if (mgfDigestName.equals("SHA-512")) {
-                    mgfSpec = MGF1ParameterSpec.SHA512;
-                } else if (mgfDigestName.equals("SHA-512/224")) {
-                    mgfSpec = MGF1ParameterSpec.SHA512_224;
-                } else if (mgfDigestName.equals("SHA-512/256")) {
-                    mgfSpec = MGF1ParameterSpec.SHA512_256;
-                } else {
-                    throw new IOException(
                         "Unrecognized message digest algorithm");
+            };
+        } else {
+            mgfSpec = MGF1ParameterSpec.SHA1;
         }
-            } else if (data.isContextSpecific((byte) 0x02)) {
+        sub = der.getOptionalExplicitContextSpecific(2);
-                // pSource algid
+        if (sub.isPresent()) {
-                AlgorithmId val = AlgorithmId.parse(data.data.getDerValue());
+            AlgorithmId val = AlgorithmId.parse(sub.get());
             if (!val.getOID().equals(OID_PSpecified)) {
                 throw new IOException("Wrong OID for pSpecified");
             }
-                DerInputStream dis = new DerInputStream(val.getEncodedParams());
+            p = DerValue.wrap(val.getEncodedParams()).getOctetString();
-                p = dis.getOctetString();
-                if (dis.available() != 0) {
-                    throw new IOException("Extra data for pSpecified");
-                }
         } else {
-                throw new IOException("Invalid encoded OAEPParameters");
+            p = new byte[0];
-            }
         }
+        der.atEnd();
     }
 
     protected void engineInit(byte[] encoded, String decodingMethod)

src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java

@@ -268,25 +268,17 @@ abstract class PBES2Parameters extends AlgorithmParametersSpi {
         }
         iCount = pBKDF2_params.data.getInteger();
 
-        DerValue prf = null;
         // keyLength INTEGER (1..MAX) OPTIONAL,
-        if (pBKDF2_params.data.available() > 0) {
+        var ksDer = pBKDF2_params.data.getOptional(DerValue.tag_Integer);
-            DerValue keyLength = pBKDF2_params.data.getDerValue();
+        if (ksDer.isPresent()) {
-            if (keyLength.tag == DerValue.tag_Integer) {
+            keysize = ksDer.get().getInteger() * 8; // keysize (in bits)
-                keysize = keyLength.getInteger() * 8; // keysize (in bits)
-            } else {
-                // Should be the prf
-                prf = keyLength;
-            }
         }
+
         // prf AlgorithmIdentifier {{PBKDF2-PRFs}} DEFAULT algid-hmacWithSHA1
-        String kdfAlgo = "HmacSHA1";
+        String kdfAlgo;
-        if (prf == null) {
+        var prfDer = pBKDF2_params.data.getOptional(DerValue.tag_Sequence);
-            if (pBKDF2_params.data.available() > 0) {
+        if (prfDer.isPresent()) {
-                prf = pBKDF2_params.data.getDerValue();
+            DerValue prf = prfDer.get();
-            }
-        }
-        if (prf != null) {
             kdfAlgo_OID = prf.data.getOID();
             KnownOIDs o = KnownOIDs.findMatch(kdfAlgo_OID.toString());
             if (o == null || (!o.stdName().equals("HmacSHA1") &&
@@ -299,17 +291,11 @@ abstract class PBES2Parameters extends AlgorithmParametersSpi {
                         + "derivation function");
             }
             kdfAlgo = o.stdName();
-
+            prf.data.getOptional(DerValue.tag_Null);
-            if (prf.data.available() != 0) {
+            prf.data.atEnd();
-                // parameter is 'NULL' for all HmacSHA KDFs
+        } else {
-                DerValue parameter = prf.data.getDerValue();
+            kdfAlgo = "HmacSHA1";
-                if (parameter.tag != DerValue.tag_Null) {
-                    throw new IOException("PBE parameter parsing error: "
-                            + "not an ASN.1 NULL tag");
         }
-            }
-        }
-
         return kdfAlgo;
     }
 

src/java.base/share/classes/sun/security/pkcs/PKCS7.java

@@ -255,6 +255,16 @@ public class PKCS7 {
         }
     }
 
+    //    SignedData ::= SEQUENCE {
+    //     version Version,
+    //     digestAlgorithms DigestAlgorithmIdentifiers,
+    //     contentInfo ContentInfo,
+    //     certificates
+    //        [0] IMPLICIT ExtendedCertificatesAndCertificates
+    //          OPTIONAL,
+    //     crls
+    //       [1] IMPLICIT CertificateRevocationLists OPTIONAL,
+    //     signerInfos SignerInfos }
     private void parseSignedData(DerValue val)
         throws ParsingException, IOException {
 
@@ -294,9 +304,9 @@ public class PKCS7 {
          * check if certificates (implicit tag) are provided
          * (certificates are OPTIONAL)
          */
-        if ((byte)(dis.peekByte()) == (byte)0xA0) {
+        var certDer = dis.getOptionalImplicitContextSpecific(0, DerValue.tag_SetOf);
-            DerValue[] certVals = dis.getSet(2, true);
+        if (certDer.isPresent()) {
-
+            DerValue[] certVals = certDer.get().subs(DerValue.tag_SetOf, 2);
             len = certVals.length;
             certificates = new X509Certificate[len];
             int count = 0;
@@ -339,9 +349,9 @@ public class PKCS7 {
         }
 
         // check if crls (implicit tag) are provided (crls are OPTIONAL)
-        if ((byte)(dis.peekByte()) == (byte)0xA1) {
+        var crlsDer = dis.getOptionalImplicitContextSpecific(1, DerValue.tag_SetOf);
-            DerValue[] crlVals = dis.getSet(1, true);
+        if (crlsDer.isPresent()) {
-
+            DerValue[] crlVals = crlsDer.get().subs(DerValue.tag_SetOf, 1);
             len = crlVals.length;
             crls = new X509CRL[len];
 

src/java.base/share/classes/sun/security/util/DerInputStream.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,8 @@ import java.io.IOException;
 import java.math.BigInteger;
 import java.util.Arrays;
 import java.util.Date;
+import java.util.Optional;
+import java.util.function.Predicate;
 
 /**
  * A DER input stream, used for parsing ASN.1 DER-encoded data such as
@@ -53,7 +55,6 @@ import java.util.Date;
  * @author Amit Kapoor
  * @author Hemma Prafullchandra
  */
-
 public class DerInputStream {
 
     // The static part
@@ -305,4 +306,105 @@ public class DerInputStream {
      * empty.
      */
     public int available() { return end - pos; }
+
+    /**
+     * Ensures there is no more data. This can be called when the last
+     * expected field is parsed, and we need to make sure no unread is left.
+     *
+     * @throws IOException if the end is NOT reached yet
+     */
+    public void atEnd() throws IOException {
+        if (available() != 0) {
+            throw new IOException("Extra unused bytes");
+        }
+    }
+
+    /**
+     * Checks if the tag of the next DerValue matches the rule.
+     *
+     * @param rule the rule to check for the tag.
+     * @return true if matches, false if not or stream is at end.
+     * @throws IOException if an I/O error happens while peeking the byte
+     */
+    private boolean checkNextTag(Predicate<Byte> rule) {
+        return available() > 0 && rule.test(data[pos]);
+    }
+
+    /**
+     * Detect if the tag of the next DerValue is the specified one.
+     *
+     * @param tag the expected tag
+     * @return true if matches, false if not or stream is at end.
+     * @throws IOException if an I/O error happens while peeking the byte
+     */
+    private boolean checkNextTag(byte tag) {
+        return checkNextTag(t -> t == tag);
+    }
+
+    /**
+     * Returns the next DerValue if its tag is the given one.
+     *
+     * @param tag the expected tag
+     * @return the next DerValue, or empty if not found or stream at end
+     * @throws IOException if an I/O error happens
+     */
+    public Optional<DerValue> getOptional(byte tag) throws IOException {
+        if (checkNextTag(tag)) {
+            return Optional.of(getDerValue());
+        } else {
+            return Optional.empty();
+        }
+    }
+
+    /**
+     * Detect if the next DerValue is a context-specific value
+     * tagged by {@code n}.
+     *
+     * @param n the expected tag
+     * @return true if matches, false if not or stream is at end.
+     * @throws IOException if an I/O error happens while peeking the byte
+     */
+    public boolean seeOptionalContextSpecific(int n) throws IOException {
+        return checkNextTag(t -> (t & 0x0c0) == 0x080 && (t & 0x01f) == n);
+    }
+
+    /**
+     * Returns the inner DerValue if the next DerValue is
+     * an EXPLICIT context-specific value tagged by {@code n}.
+     *
+     * @param n the expected tag
+     * @return the inner DerValue, or empty if not found or stream at end
+     * @throws IOException if an I/O error happens
+     */
+    public Optional<DerValue> getOptionalExplicitContextSpecific(int n)
+            throws IOException {
+        if (seeOptionalContextSpecific(n)) {
+            DerInputStream sub = getDerValue().data(); // stream inside [n]
+            DerValue inner = sub.getDerValue(); // inside [n]
+            sub.atEnd(); // make sure there is only one inner value
+            return Optional.of(inner);
+        } else {
+            return Optional.empty();
+        }
+    }
+
+    /**
+     * Returns the restored DerValue if the next DerValue is
+     * an IMPLICIT context-specific value tagged by {@code n}.
+     *
+     * @param n the expected tag
+     * @param tag the real tag for the IMPLICIT type
+     * @return the restored DerValue, or empty if not found or stream at end
+     * @throws IOException if an I/O error happens
+     */
+    public Optional<DerValue> getOptionalImplicitContextSpecific(int n, byte tag)
+            throws IOException {
+        if (seeOptionalContextSpecific(n)) {
+            DerValue v = getDerValue(); // [n]
+            // restore tag because IMPLICIT has overwritten it
+            return Optional.of(v.withTag(tag));
+        } else {
+            return Optional.empty();
+        }
+    }
 }

src/java.base/share/classes/sun/security/util/DerValue.java

@@ -290,7 +290,7 @@ public class DerValue {
     }
 
     /**
-     * Wraps an DerOutputStream. All bytes currently written
+     * Wraps a DerOutputStream. All bytes currently written
      * into the stream will become the content of the newly
      * created DerValue.
      *
@@ -305,6 +305,34 @@ public class DerValue {
         return new DerValue(tag, out.buf(), 0, out.size(), false);
     }
 
+    /**
+     * Wraps a byte array as a single DerValue.
+     *
+     * Attention: no cloning is made.
+     *
+     * @param buf the byte array containing the DER-encoded datum
+     * @returns a new DerValue
+     */
+    public static DerValue wrap(byte[] buf)
+            throws IOException {
+        return wrap(buf, 0, buf.length);
+    }
+
+    /**
+     * Wraps a byte array as a single DerValue.
+     *
+     * Attention: no cloning is made.
+     *
+     * @param buf the byte array containing the DER-encoded datum
+     * @param offset where the encoded datum starts inside {@code buf}
+     * @param len length of bytes to parse inside {@code buf}
+     * @returns a new DerValue
+     */
+    public static DerValue wrap(byte[] buf, int offset, int len)
+            throws IOException {
+        return new DerValue(buf, offset, len, true, false);
+    }
+
     /**
      * Parse an ASN.1/BER encoded datum. The entire encoding must hold exactly
      * one datum, including its tag and length.
@@ -1229,7 +1257,7 @@ public class DerValue {
      * @param startLen estimated number of sub-values
      * @return the sub-values in an array
      */
-    DerValue[] subs(byte expectedTag, int startLen) throws IOException {
+    public DerValue[] subs(byte expectedTag, int startLen) throws IOException {
         if (expectedTag != 0 && expectedTag != tag) {
             throw new IOException("Not the correct tag");
         }

test/jdk/com/sun/crypto/provider/AlgorithmParameters/OAEPOrder.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import javax.crypto.spec.OAEPParameterSpec;
+import javax.crypto.spec.PSource;
+import java.io.IOException;
+import java.security.AlgorithmParameters;
+import java.security.spec.MGF1ParameterSpec;
+import java.util.Arrays;
+
+/**
+ * @test
+ * @bug 8246797
+ * @summary Ensures OAEPParameters read correct encoding and
+ * reject encoding with invalid ordering
+ */
+
+public class OAEPOrder {
+    public static void main(String[] args) throws Exception {
+        // Do not use default fields
+        OAEPParameterSpec spec = new OAEPParameterSpec(
+                "SHA-384", "MGF1", MGF1ParameterSpec.SHA384,
+                new PSource.PSpecified(new byte[10]));
+        AlgorithmParameters alg = AlgorithmParameters.getInstance("OAEP");
+        alg.init(spec);
+        byte[] encoded = alg.getEncoded();
+
+        // Extract the fields inside encoding
+        // [0] HashAlgorithm
+        byte[] a0 = Arrays.copyOfRange(encoded, 2, encoded[3] + 4);
+        // [1] MaskGenAlgorithm + [2] PSourceAlgorithm
+        byte[] a12 = Arrays.copyOfRange(encoded, 2 + a0.length, encoded.length);
+
+        // and rearrange [1] and [2] before [0]
+        System.arraycopy(a12, 0, encoded, 2, a12.length);
+        System.arraycopy(a0, 0, encoded, 2 + a12.length, a0.length);
+
+        AlgorithmParameters alg2 = AlgorithmParameters.getInstance("OAEP");
+        try {
+            alg2.init(encoded);
+            throw new RuntimeException("Should fail");
+        } catch (IOException ioe) {
+            // expected
+        }
+    }
+}