8296442: EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters

wangweij · wangweij · commit 68d3ed5ceec3 · 2022-11-16T20:30:34.000Z
Reviewed-by: xuelei, kdriver, mullan
diff --git a/src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java b/src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
@@ -57,17 +57,20 @@
 
 public class EncryptedPrivateKeyInfo {
 
-    // the "encryptionAlgorithm" field
+    // The "encryptionAlgorithm" is stored in either the algid or
+    // the params field. Precisely, if this object is created by
+    // {@link #EncryptedPrivateKeyInfo(AlgorithmParameters, byte[])}
+    // with an uninitialized AlgorithmParameters, the AlgorithmParameters
+    // object is stored in the params field and algid is set to null.
+    // In all other cases, algid is non null and params is null.
     private final AlgorithmId algid;
-
-    // the algorithm name of the encrypted private key
-    private String keyAlg;
+    private final AlgorithmParameters params;
 
     // the "encryptedData" field
     private final byte[] encryptedData;
 
     // the ASN.1 encoded contents of this class
-    private byte[] encoded;
+    private final byte[] encoded;
 
     /**
      * Constructs (i.e., parses) an {@code EncryptedPrivateKeyInfo} from
@@ -100,6 +103,7 @@ public EncryptedPrivateKeyInfo(byte[] encoded) throws IOException {
         }
 
         this.algid = AlgorithmId.parse(seq[0]);
+        this.params = null;
         if (seq[0].data.available() != 0) {
             throw new IOException("encryptionAlgorithm field overrun");
         }
@@ -141,6 +145,7 @@ public EncryptedPrivateKeyInfo(String algName, byte[] encryptedData)
                 throw new NullPointerException("the algName parameter " +
                                                "must be non-null");
         this.algid = AlgorithmId.get(algName);
+        this.params = null;
 
         if (encryptedData == null) {
             throw new NullPointerException("the encryptedData " +
@@ -181,7 +186,22 @@ public EncryptedPrivateKeyInfo(AlgorithmParameters algParams,
         if (algParams == null) {
             throw new NullPointerException("algParams must be non-null");
         }
-        this.algid = AlgorithmId.get(algParams);
+        AlgorithmId tmp;
+        try {
+            tmp = AlgorithmId.get(algParams);
+        } catch (IllegalStateException e) {
+            // This exception is thrown when algParams.getEncoded() fails.
+            // While the spec of this constructor requires that
+            // "getEncoded should return...", in reality people might
+            // create with an uninitialized algParams first and only
+            // initialize it before calling getEncoded(). Thus we support
+            // this case as well.
+            tmp = null;
+        }
+
+        // one and only one is non null
+        this.algid = tmp;
+        this.params = this.algid != null ? null : algParams;
 
         if (encryptedData == null) {
             throw new NullPointerException("encryptedData must be non-null");
@@ -197,7 +217,6 @@ public EncryptedPrivateKeyInfo(AlgorithmParameters algParams,
         this.encoded = null;
     }
 
-
     /**
      * Returns the encryption algorithm.
      * <p>Note: Standard name is returned instead of the specified one
@@ -209,15 +228,15 @@ public EncryptedPrivateKeyInfo(AlgorithmParameters algParams,
      * @return the encryption algorithm name.
      */
     public String getAlgName() {
-        return this.algid.getName();
+        return algid == null ? params.getAlgorithm() : algid.getName();
     }
 
     /**
      * Returns the algorithm parameters used by the encryption algorithm.
      * @return the algorithm parameters.
      */
     public AlgorithmParameters getAlgParameters() {
-        return this.algid.getParameters();
+        return algid == null ? params : algid.getParameters();
     }
 
     /**
@@ -252,14 +271,13 @@ public PKCS8EncodedKeySpec getKeySpec(Cipher cipher)
         byte[] encoded;
         try {
             encoded = cipher.doFinal(encryptedData);
-            checkPKCS8Encoding(encoded);
+            return pkcs8EncodingToSpec(encoded);
         } catch (GeneralSecurityException |
                  IOException |
                  IllegalStateException ex) {
             throw new InvalidKeySpecException(
                     "Cannot retrieve the PKCS8EncodedKeySpec", ex);
         }
-        return new PKCS8EncodedKeySpec(encoded, keyAlg);
     }
 
     private PKCS8EncodedKeySpec getKeySpecImpl(Key decryptKey,
@@ -270,21 +288,20 @@ private PKCS8EncodedKeySpec getKeySpecImpl(Key decryptKey,
         try {
             if (provider == null) {
                 // use the most preferred one
-                c = Cipher.getInstance(algid.getName());
+                c = Cipher.getInstance(getAlgName());
             } else {
-                c = Cipher.getInstance(algid.getName(), provider);
+                c = Cipher.getInstance(getAlgName(), provider);
             }
-            c.init(Cipher.DECRYPT_MODE, decryptKey, algid.getParameters());
+            c.init(Cipher.DECRYPT_MODE, decryptKey, getAlgParameters());
             encoded = c.doFinal(encryptedData);
-            checkPKCS8Encoding(encoded);
+            return pkcs8EncodingToSpec(encoded);
         } catch (NoSuchAlgorithmException nsae) {
             // rethrow
             throw nsae;
         } catch (GeneralSecurityException | IOException ex) {
             throw new InvalidKeyException(
                     "Cannot retrieve the PKCS8EncodedKeySpec", ex);
         }
-        return new PKCS8EncodedKeySpec(encoded, keyAlg);
     }
 
     /**
@@ -388,14 +405,23 @@ public byte[] getEncoded() throws IOException {
             DerOutputStream tmp = new DerOutputStream();
 
             // encode encryption algorithm
-            algid.encode(tmp);
+            if (algid != null) {
+                algid.encode(tmp);
+            } else {
+                try {
+                    // Let's hope params has been initialized by now.
+                    AlgorithmId.get(params).encode(tmp);
+                } catch (Exception e) {
+                    throw new IOException("not initialized", e);
+                }
+            }
 
             // encode encrypted data
             tmp.putOctetString(encryptedData);
 
             // wrap everything into a SEQUENCE
             out.write(DerValue.tag_Sequence, tmp);
-            this.encoded = out.toByteArray();
+            return out.toByteArray();
         }
         return this.encoded.clone();
     }
@@ -409,7 +435,7 @@ private static void checkTag(DerValue val, byte tag, String valName)
     }
 
     @SuppressWarnings("fallthrough")
-    private void checkPKCS8Encoding(byte[] encodedKey)
+    private static PKCS8EncodedKeySpec pkcs8EncodingToSpec(byte[] encodedKey)
         throws IOException {
         DerInputStream in = new DerInputStream(encodedKey);
         DerValue[] values = in.getSequence(3);
@@ -420,9 +446,9 @@ private void checkPKCS8Encoding(byte[] encodedKey)
             /* fall through */
         case 3:
             checkTag(values[0], DerValue.tag_Integer, "version");
-            keyAlg = AlgorithmId.parse(values[1]).getName();
+            String keyAlg = AlgorithmId.parse(values[1]).getName();
             checkTag(values[2], DerValue.tag_OctetString, "privateKey");
-            break;
+            return new PKCS8EncodedKeySpec(encodedKey, keyAlg);
         default:
             throw new IOException("invalid key encoding");
         }
diff --git a/src/java.base/share/classes/sun/security/pkcs12/MacData.java b/src/java.base/share/classes/sun/security/pkcs12/MacData.java
@@ -116,36 +116,6 @@ class MacData {
 
     }
 
-    MacData(AlgorithmParameters algParams, byte[] digest,
-        byte[] salt, int iterations) throws NoSuchAlgorithmException
-    {
-        if (algParams == null)
-           throw new NullPointerException("the algParams parameter " +
-                                               "must be non-null");
-
-        AlgorithmId algid = AlgorithmId.get(algParams);
-        this.digestAlgorithmName = algid.getName();
-        this.digestAlgorithmParams = algid.getParameters();
-
-        if (digest == null) {
-            throw new NullPointerException("the digest " +
-                                           "parameter must be non-null");
-        } else if (digest.length == 0) {
-            throw new IllegalArgumentException("the digest " +
-                                                "parameter must not be empty");
-        } else {
-            this.digest = digest.clone();
-        }
-
-        this.macSalt = salt;
-        this.iterations = iterations;
-
-        // delay the generation of ASN.1 encoding until
-        // getEncoded() is called
-        this.encoded = null;
-
-    }
-
     String getDigestAlgName() {
         return digestAlgorithmName;
     }
diff --git a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
@@ -100,6 +100,8 @@ public AlgorithmId(ObjectIdentifier oid) {
      *
      * @param oid the identifier for the algorithm.
      * @param algparams the associated algorithm parameters, can be null.
+     * @exception IllegalStateException if algparams is not initialized
+     *                                  or cannot be encoded
      */
     public AlgorithmId(ObjectIdentifier oid, AlgorithmParameters algparams) {
         algid = oid;
@@ -108,9 +110,9 @@ public AlgorithmId(ObjectIdentifier oid, AlgorithmParameters algparams) {
             try {
                 encodedParams = algParams.getEncoded();
             } catch (IOException ioe) {
-                // Ignore this at the moment. This exception can occur
-                // if AlgorithmParameters was not initialized yet. Will
-                // try to re-getEncoded() again later.
+                throw new IllegalStateException(
+                        "AlgorithmParameters not initialized or cannot be decoded",
+                        ioe);
             }
         }
     }
@@ -157,17 +159,11 @@ protected void decodeParams() throws IOException {
      * @exception IOException on encoding error.
      */
     @Override
-    public void encode (DerOutputStream out) throws IOException {
+    public void encode(DerOutputStream out) throws IOException {
         DerOutputStream bytes = new DerOutputStream();
 
         bytes.putOID(algid);
 
-        // Re-getEncoded() from algParams if it was not initialized
-        if (algParams != null && encodedParams == null) {
-            encodedParams = algParams.getEncoded();
-            // If still not initialized. Let the IOE be thrown.
-        }
-
         if (encodedParams == null) {
             // Changes backed out for compatibility with Solaris
 
@@ -488,6 +484,8 @@ public static AlgorithmId get(String algname)
      *
      * @param algparams the associated algorithm parameters.
      * @exception NoSuchAlgorithmException on error.
+     * @exception IllegalStateException if algparams is not initialized
+     *                                  or cannot be encoded
      */
     public static AlgorithmId get(AlgorithmParameters algparams)
             throws NoSuchAlgorithmException {
diff --git a/test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetAlgName.java b/test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetAlgName.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2007, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2022, 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
@@ -23,16 +23,11 @@
 
 /**
  * @test
- * @bug 4941596
+ * @bug 4941596 8296442
  * @summary Test the EncryptedPrivateKeyInfo.getAlgName(...) methods.
  * @author Valerie Peng
  */
-import java.util.*;
-import java.nio.*;
-import java.io.*;
 import java.security.*;
-import java.util.Arrays;
-import java.security.spec.*;
 import javax.crypto.*;
 import javax.crypto.spec.*;
 
@@ -61,8 +56,18 @@ public static void main(String[] argv) throws Exception {
             AlgorithmParameters ap = c.getParameters();
             epki = new EncryptedPrivateKeyInfo(ap, BYTES);
             if (!epki.getAlgName().equalsIgnoreCase(algo)) {
-                System.out.println("...expect: " + algo);
-                System.out.println("...got: " + epki.getAlgName());
+                System.out.println("...expected: " + algo);
+                System.out.println("     ...got: " + epki.getAlgName());
+                status = false;
+            }
+
+            // Make sure EncryptedPrivateKeyInfo can be created with an
+            // uninitialized AlgorithmParameters.
+            AlgorithmParameters ap2 = AlgorithmParameters.getInstance(ap.getAlgorithm());
+            epki = new EncryptedPrivateKeyInfo(ap2, BYTES);
+            if (!epki.getAlgName().equalsIgnoreCase(algo)) {
+                System.out.println("...expected: " + algo);
+                System.out.println("     ...got: " + epki.getAlgName());
                 status = false;
             }
         }
diff --git a/test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetEncoded.java b/test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetEncoded.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 2022, 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
@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 8261779
+ * @bug 8261779 8296442
  * @summary Check that EncryptedPrivateKeyInfo.getEncoded() calls
  *          AlgorithmParameters.getEncoded() when first called
  */
@@ -56,6 +56,14 @@ public static void main(String[] argv) throws Exception {
         AlgorithmParameters ap1 = AlgorithmParameters.getInstance("EC");
         EncryptedPrivateKeyInfo epki1 =
             new EncryptedPrivateKeyInfo(ap1, new byte[] {1, 2, 3, 4});
+
+        try {
+            epki1.getEncoded();
+            throw new Exception("Should have thrown IOException");
+        } catch (IOException ioe) {
+            // test passed, expected exception
+        }
+
         ap1.init(new ECGenParameterSpec("secp256r1"));
 
         EncryptedPrivateKeyInfo epki2 =
diff --git a/test/jdk/sun/security/x509/AlgorithmId/Uninitialized.java b/test/jdk/sun/security/x509/AlgorithmId/Uninitialized.java
@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2022, 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.
+ */
+
+/*
+ * @test
+ * @bug 8296442
+ * @summary EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters
+ * @modules java.base/sun.security.x509
+ */
+
+import sun.security.x509.AlgorithmId;
+
+import java.security.AlgorithmParameters;
+
+public class Uninitialized {
+    public static void main(String[] args) throws Exception {
+        AlgorithmParameters ap = AlgorithmParameters.getInstance("EC");
+        boolean success;
+        try {
+            AlgorithmId.get(ap);
+            success = true;
+        } catch (Exception e) {
+            success = false;
+        }
+        if (success) {
+            throw new RuntimeException();
+        }
+    }
+}
