8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getE…

…xtendedKeyUsage do not throw CertificateParsingException if extension is unparseable

Reviewed-by: weijun

src/java.base/share/classes/sun/security/x509/CertificateExtensions.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 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
@@ -28,13 +28,10 @@
 import java.io.IOException;
 import java.io.OutputStream;
 import java.lang.reflect.Constructor;
-import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.security.cert.CertificateException;
 import java.util.*;
 
-import sun.security.util.HexDumpEncoder;
-
 import sun.security.util.*;
 
 /**
@@ -109,11 +106,10 @@ private void parseExtension(Extension ext) throws IOException {
 
             Object[] passed = new Object[] {Boolean.valueOf(ext.isCritical()),
                     ext.getExtensionValue()};
-                    CertAttrSet<?> certExt = (CertAttrSet<?>)
-                            cons.newInstance(passed);
-                    if (map.put(certExt.getName(), (Extension)certExt) != null) {
-                        throw new IOException("Duplicate extensions not allowed");
-                    }
+            CertAttrSet<?> certExt = (CertAttrSet<?>) cons.newInstance(passed);
+            if (map.put(certExt.getName(), (Extension)certExt) != null) {
+                throw new IOException("Duplicate extensions not allowed");
+            }
         } catch (InvocationTargetException invk) {
             Throwable e = invk.getCause();
             if (ext.isCritical() == false) {
@@ -352,32 +348,3 @@ public String toString() {
     }
 
 }
-
-class UnparseableExtension extends Extension {
-    private String name;
-    private String exceptionDescription;
-
-    public UnparseableExtension(Extension ext, Throwable why) {
-        super(ext);
-
-        name = "";
-        try {
-            Class<?> extClass = OIDMap.getClass(ext.getExtensionId());
-            if (extClass != null) {
-                Field field = extClass.getDeclaredField("NAME");
-                name = (String)(field.get(null)) + " ";
-            }
-        } catch (Exception e) {
-            // If we cannot find the name, just ignore it
-        }
-
-        this.exceptionDescription = why.toString();
-    }
-
-    @Override public String toString() {
-        return super.toString() +
-                "Unparseable " + name + "extension due to\n" +
-                exceptionDescription + "\n\n" +
-                new HexDumpEncoder().encodeBuffer(getExtensionValue());
-    }
-}

src/java.base/share/classes/sun/security/x509/UnparseableExtension.java

@@ -0,0 +1,68 @@
+/*
+ * 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.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * 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.
+ */
+
+package sun.security.x509;
+
+import java.lang.reflect.Field;
+import sun.security.util.HexDumpEncoder;
+
+/**
+ * An extension that cannot be parsed due to decoding errors or invalid
+ * content.
+ */
+class UnparseableExtension extends Extension {
+    private String name;
+    private String exceptionDescription;
+    private String exceptionMessage;
+
+    UnparseableExtension(Extension ext, Throwable why) {
+        super(ext);
+
+        name = "";
+        try {
+            Class<?> extClass = OIDMap.getClass(ext.getExtensionId());
+            if (extClass != null) {
+                Field field = extClass.getDeclaredField("NAME");
+                name = (String)(field.get(null)) + " ";
+            }
+        } catch (Exception e) {
+            // If we cannot find the name, just ignore it
+        }
+
+        this.exceptionDescription = why.toString();
+        this.exceptionMessage = why.getMessage();
+    }
+
+    String exceptionMessage() {
+        return exceptionMessage;
+    }
+
+    @Override public String toString() {
+        return super.toString() +
+                "Unparseable " + name + "extension due to\n" +
+                exceptionDescription + "\n\n" +
+                new HexDumpEncoder().encodeBuffer(getExtensionValue());
+    }
+}

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

@@ -1439,24 +1439,45 @@ public boolean[] getKeyUsage() {
     }
 
     /**
-     * This method are the overridden implementation of
+     * This method is the overridden implementation of the
      * getExtendedKeyUsage method in X509Certificate in the Sun
      * provider. It is better performance-wise since it returns cached
      * values.
      */
+    @Override
     public synchronized List<String> getExtendedKeyUsage()
         throws CertificateParsingException {
         if (readOnly && extKeyUsage != null) {
             return extKeyUsage;
-        } else {
-            ExtendedKeyUsageExtension ext = getExtendedKeyUsageExtension();
-            if (ext == null) {
-                return null;
+        }
+        ExtendedKeyUsageExtension ext = (ExtendedKeyUsageExtension)
+            getExtensionIfParseable(PKIXExtensions.ExtendedKeyUsage_Id);
+        if (ext == null) {
+            return null;
+        }
+        extKeyUsage = Collections.unmodifiableList(ext.getExtendedKeyUsage());
+        return extKeyUsage;
+    }
+
+    /**
+     * Returns the extension identified by OID or null if it doesn't exist
+     * and is not unparseable.
+     *
+     * @throws CertificateParsingException if extension is unparseable
+     */
+    private Extension getExtensionIfParseable(ObjectIdentifier oid)
+            throws CertificateParsingException {
+        Extension ext = getExtension(oid);
+        if (ext == null) {
+            // check if unparseable
+            UnparseableExtension unparseableExt =
+                   (UnparseableExtension)getUnparseableExtension(oid);
+            if (unparseableExt != null) {
+                throw new CertificateParsingException(
+                        unparseableExt.exceptionMessage());
             }
-            extKeyUsage =
-                Collections.unmodifiableList(ext.getExtendedKeyUsage());
-            return extKeyUsage;
         }
+        return ext;
     }
 
     /**
@@ -1602,19 +1623,21 @@ private static Collection<List<?>> cloneAltNames(Collection<List<?>> altNames) {
     }
 
     /**
-     * This method are the overridden implementation of
+     * This method is the overridden implementation of the
      * getSubjectAlternativeNames method in X509Certificate in the Sun
      * provider. It is better performance-wise since it returns cached
      * values.
      */
+    @Override
     public synchronized Collection<List<?>> getSubjectAlternativeNames()
         throws CertificateParsingException {
         // return cached value if we can
         if (readOnly && subjectAlternativeNames != null)  {
             return cloneAltNames(subjectAlternativeNames);
         }
         SubjectAlternativeNameExtension subjectAltNameExt =
-            getSubjectAlternativeNameExtension();
+            (SubjectAlternativeNameExtension)getExtensionIfParseable(
+                PKIXExtensions.SubjectAlternativeName_Id);
         if (subjectAltNameExt == null) {
             return null;
         }
@@ -1632,7 +1655,7 @@ public synchronized Collection<List<?>> getSubjectAlternativeNames()
 
     /**
      * This static method is the default implementation of the
-     * getSubjectAlternaitveNames method in X509Certificate. A
+     * getSubjectAlternativeNames method in X509Certificate. A
      * X509Certificate provider generally should overwrite this to
      * provide among other things caching for better performance.
      */
@@ -1666,19 +1689,21 @@ public static Collection<List<?>> getSubjectAlternativeNames(X509Certificate cer
     }
 
     /**
-     * This method are the overridden implementation of
+     * This method is the overridden implementation of the
      * getIssuerAlternativeNames method in X509Certificate in the Sun
      * provider. It is better performance-wise since it returns cached
      * values.
      */
+    @Override
     public synchronized Collection<List<?>> getIssuerAlternativeNames()
         throws CertificateParsingException {
         // return cached value if we can
         if (readOnly && issuerAlternativeNames != null) {
             return cloneAltNames(issuerAlternativeNames);
         }
         IssuerAlternativeNameExtension issuerAltNameExt =
-            getIssuerAlternativeNameExtension();
+            (IssuerAlternativeNameExtension)getExtensionIfParseable(
+                PKIXExtensions.IssuerAlternativeName_Id);
         if (issuerAltNameExt == null) {
             return null;
         }
@@ -1696,7 +1721,7 @@ public synchronized Collection<List<?>> getIssuerAlternativeNames()
 
     /**
      * This static method is the default implementation of the
-     * getIssuerAlternaitveNames method in X509Certificate. A
+     * getIssuerAlternativeNames method in X509Certificate. A
      * X509Certificate provider generally should overwrite this to
      * provide among other things caching for better performance.
      */

test/jdk/java/security/cert/X509Certificate/GetUnparseableExtensions.java

@@ -0,0 +1,86 @@
+/*
+ * 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 java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import jdk.test.lib.security.CertUtils;
+
+/*
+ * @test
+ * @bug 8251468
+ * @library /test/lib
+ * @summary Check that X509Certificate.getSubjectAlternativeNames,
+ *          getIssuerAlternativeNames and getExtendedKeyUsage throw
+ *          CertificateParsingException if extension is unparseable/invalid.
+ */
+public class GetUnparseableExtensions {
+
+    // Cert has 3 badly encoded extensions:
+    // 1. SubjectAlternativeNameExtension with a null RFC822Name
+    // 2. IssuerAlternativeNameExtension with a null RFC822Name
+    // 3. ExtendedKeyUsageExtension encoded as a Set instead of a Sequence
+    private final static String CERT_WITH_UNPARSEABLE_EXTS = """
+        -----BEGIN CERTIFICATE-----
+        MIIDDTCCAfWgAwIBAgIIc/2fukmBqZgwDQYJKoZIhvcNAQELBQAwDTELMAkGA1UE
+        AxMCQ0EwHhcNMjExMDI2MTgxNzU4WhcNMjIwMTI0MTgxNzU4WjANMQswCQYDVQQD
+        EwJFMTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKIIIsP6ghZ94hIl
+        U3G+D11vASSP7JMmX6G6opGKel60dZ5So3Tdj0E6VHehwcxEdh94LEIxvOkS3Cdh
+        hd1iiB4/q2InvLuGzSVvWAwYNkB7GbR31pIst0GAFdH+z/tVjjHcacBGwmysk8DN
+        Rmxm66AyJxHA1U/v64vx5kSGNoUAEcWm+OslxfdJ7nygnU+S6a/06sRvifVn7q7c
+        Kg4AIFE1+vWrcgYbApkPNjLiRGAr33TAb1y+nN1zW/luEMEQIJBd+YJW9OrPGMUr
+        SDSXfB06HscH1bTUfN2ngsxT2hy9eiCiDtagPIwYA0sVNjyUtfPsjAH1oxDPXzC5
+        quJB5/sCAwEAAaNxMG8wHQYDVR0OBBYEFCqWMQBki21cSqKIHp4rFitf6pQ2MAsG
+        A1UdEQQEMAKBADALBgNVHRIEBDACgQAwHwYDVR0jBBgwFoAUJbvRN17hEohYsg1M
+        icq1mMpIwoAwEwYDVR0lBAwxCgYIKwYBBQUHAwEwDQYJKoZIhvcNAQELBQADggEB
+        ABBqJ/yYXD1xueB63GRY4ZotO6ukEkJiPZIwrr2vZW+GMws2b9gqNoD+dL9AeYCA
+        Zb6RYbaNDY5OoJmEty9KbtON7Rt1LtCFuZbYKxrhW0dJgXLyNOgXr+x0g2btbvWV
+        r8U1icwHapZM5IqDKLivzZNzwv52mrJDuzWqmlhAIlPLIU1QfNQ1oC8HpFkL71Bb
+        sB/4OIxxjRzf0AGmb7aeQNfxag2oKlOwqzum1FLt8BaVjylc0aUATPtkgothK8nK
+        m5jXJmA4zA11ck0uJW39gDRuR0D1k4qm/s/5Iuhd6MRVDXEhVbJXuH2yzHcmbgRs
+        paD/he6C1JszWf7YrTsX3Fc=
+        -----END CERTIFICATE-----
+        """;
+
+    @FunctionalInterface
+    public interface TestCase<T> {
+        T test() throws Exception;
+    }
+
+    public static void main(String[] args) throws Exception {
+
+        X509Certificate cert =
+                CertUtils.getCertFromString(CERT_WITH_UNPARSEABLE_EXTS);
+        getExtension(() -> cert.getSubjectAlternativeNames());
+        getExtension(() -> cert.getIssuerAlternativeNames());
+        getExtension(() -> cert.getExtendedKeyUsage());
+    }
+
+    private static void getExtension(TestCase<?> t) throws Exception {
+        try {
+            t.test();
+            throw new Exception("Test FAILED");
+        } catch (CertificateParsingException cpe) {
+            System.out.println(cpe.getMessage());
+        }
+    }
+}