7143872: Improve certificate extension processing

Reviewed-by: mullan

src/share/classes/sun/security/x509/CRLExtensions.java

@@ -32,8 +32,10 @@ import java.lang.reflect.InvocationTargetException;
 import java.security.cert.CRLException;
 import java.security.cert.CertificateException;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Enumeration;
-import java.util.Hashtable;
+import java.util.Map;
+import java.util.TreeMap;
 
 import sun.security.util.*;
 import sun.misc.HexDumpEncoder;
@@ -62,7 +64,8 @@ import sun.misc.HexDumpEncoder;
  */
 public class CRLExtensions {
 
-    private Hashtable<String,Extension> map = new Hashtable<String,Extension>();
+    private Map<String,Extension> map = Collections.synchronizedMap(
+            new TreeMap<String,Extension>());
     private boolean unsupportedCritExt = false;
 
     /**
@@ -215,7 +218,7 @@ public class CRLExtensions {
      * @return an enumeration of the extensions in this CRL.
      */
     public Enumeration<Extension> getElements() {
-        return map.elements();
+        return Collections.enumeration(map.values());
     }
 
     /**

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

@@ -57,7 +57,8 @@ public class CertificateExtensions implements CertAttrSet<Extension> {
 
     private static final Debug debug = Debug.getInstance("x509");
 
-    private Hashtable<String,Extension> map = new Hashtable<String,Extension>();
+    private Map<String,Extension> map = Collections.synchronizedMap(
+            new TreeMap<String,Extension>());
     private boolean unsupportedCritExt = false;
 
     private Map<String,Extension> unparseableExtensions;
@@ -117,7 +118,7 @@ public class CertificateExtensions implements CertAttrSet<Extension> {
             if (ext.isCritical() == false) {
                 // ignore errors parsing non-critical extensions
                 if (unparseableExtensions == null) {
-                    unparseableExtensions = new HashMap<String,Extension>();
+                    unparseableExtensions = new TreeMap<String,Extension>();
                 }
                 unparseableExtensions.put(ext.getExtensionId().toString(),
                         new UnparseableExtension(ext, e));
@@ -218,6 +219,12 @@ public class CertificateExtensions implements CertAttrSet<Extension> {
         return (obj);
     }
 
+    // Similar to get(String), but throw no exception, might return null.
+    // Used in X509CertImpl::getExtension(OID).
+    Extension getExtension(String name) {
+        return map.get(name);
+    }
+
     /**
      * Delete the attribute value.
      * @param name the extension name used in the lookup.
@@ -245,7 +252,7 @@ public class CertificateExtensions implements CertAttrSet<Extension> {
      * attribute.
      */
     public Enumeration<Extension> getElements() {
-        return map.elements();
+        return Collections.enumeration(map.values());
     }
 
     /**

src/share/classes/sun/security/x509/X509CRLEntryImpl.java

@@ -32,13 +32,7 @@ import java.security.cert.CRLReason;
 import java.security.cert.CertificateException;
 import java.security.cert.X509CRLEntry;
 import java.math.BigInteger;
-import java.util.Collection;
-import java.util.Date;
-import java.util.Enumeration;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
-import java.util.HashSet;
+import java.util.*;
 
 import javax.security.auth.x500.X500Principal;
 
@@ -75,7 +69,8 @@ import sun.misc.HexDumpEncoder;
  * @author Hemma Prafullchandra
  */
 
-public class X509CRLEntryImpl extends X509CRLEntry {
+public class X509CRLEntryImpl extends X509CRLEntry
+        implements Comparable<X509CRLEntryImpl> {
 
     private SerialNumber serialNumber = null;
     private Date revocationDate = null;
@@ -196,9 +191,14 @@ public class X509CRLEntryImpl extends X509CRLEntry {
      * @exception CRLException if an encoding error occurs.
      */
     public byte[] getEncoded() throws CRLException {
+        return getEncoded0().clone();
+    }
+
+    // Called internally to avoid clone
+    private byte[] getEncoded0() throws CRLException {
         if (revokedCert == null)
             this.encode(new DerOutputStream());
-        return revokedCert.clone();
+        return revokedCert;
     }
 
     @Override
@@ -352,7 +352,7 @@ public class X509CRLEntryImpl extends X509CRLEntry {
         if (extensions == null) {
             return null;
         }
-        Set<String> extSet = new HashSet<String>();
+        Set<String> extSet = new TreeSet<>();
         for (Extension ex : extensions.getAllExtensions()) {
             if (ex.isCritical()) {
                 extSet.add(ex.getExtensionId().toString());
@@ -373,7 +373,7 @@ public class X509CRLEntryImpl extends X509CRLEntry {
         if (extensions == null) {
             return null;
         }
-        Set<String> extSet = new HashSet<String>();
+        Set<String> extSet = new TreeSet<>();
         for (Extension ex : extensions.getAllExtensions()) {
             if (!ex.isCritical()) {
                 extSet.add(ex.getExtensionId().toString());
@@ -501,13 +501,39 @@ public class X509CRLEntryImpl extends X509CRLEntry {
             getExtension(PKIXExtensions.CertificateIssuer_Id);
     }
 
+    /**
+     * Returns all extensions for this entry in a map
+     * @return the extension map, can be empty, but not null
+     */
     public Map<String, java.security.cert.Extension> getExtensions() {
+        if (extensions == null) {
+            return Collections.emptyMap();
+        }
         Collection<Extension> exts = extensions.getAllExtensions();
-        HashMap<String, java.security.cert.Extension> map =
-            new HashMap<String, java.security.cert.Extension>(exts.size());
+        Map<String, java.security.cert.Extension> map = new TreeMap<>();
         for (Extension ext : exts) {
             map.put(ext.getId(), ext);
         }
         return map;
     }
+
+    @Override
+    public int compareTo(X509CRLEntryImpl that) {
+        int compSerial = getSerialNumber().compareTo(that.getSerialNumber());
+        if (compSerial != 0) {
+            return compSerial;
+        }
+        try {
+            byte[] thisEncoded = this.getEncoded0();
+            byte[] thatEncoded = that.getEncoded0();
+            for (int i=0; i<thisEncoded.length && i<thatEncoded.length; i++) {
+                int a = thisEncoded[i] & 0xff;
+                int b = thatEncoded[i] & 0xff;
+                if (a != b) return a-b;
+            }
+            return thisEncoded.length -thatEncoded.length;
+        } catch (CRLException ce) {
+            return -1;
+        }
+    }
 }

src/share/classes/sun/security/x509/X509CRLImpl.java

@@ -53,7 +53,7 @@ import sun.misc.HexDumpEncoder;
 
 /**
  * <p>
- * An implmentation for X509 CRL (Certificate Revocation List).
+ * An implementation for X509 CRL (Certificate Revocation List).
  * <p>
  * The X.509 v2 CRL format is described below in ASN.1:
  * <pre>
@@ -104,7 +104,8 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
     private X500Principal    issuerPrincipal = null;
     private Date             thisUpdate = null;
     private Date             nextUpdate = null;
-    private Map<X509IssuerSerial,X509CRLEntry> revokedCerts = new LinkedHashMap<X509IssuerSerial,X509CRLEntry>();
+    private Map<X509IssuerSerial,X509CRLEntry> revokedMap = new TreeMap<>();
+    private List<X509CRLEntry> revokedList = new LinkedList<>();
     private CRLExtensions    extensions = null;
     private final static boolean isExplicit = true;
     private static final long YR_2050 = 2524636800000L;
@@ -223,7 +224,8 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
                 badCert.setCertificateIssuer(crlIssuer, badCertIssuer);
                 X509IssuerSerial issuerSerial = new X509IssuerSerial
                     (badCertIssuer, badCert.getSerialNumber());
-                this.revokedCerts.put(issuerSerial, badCert);
+                this.revokedMap.put(issuerSerial, badCert);
+                this.revokedList.add(badCert);
                 if (badCert.hasExtensions()) {
                     this.version = 1;
                 }
@@ -305,8 +307,8 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
                     tmp.putGeneralizedTime(nextUpdate);
             }
 
-            if (!revokedCerts.isEmpty()) {
-                for (X509CRLEntry entry : revokedCerts.values()) {
+            if (!revokedList.isEmpty()) {
+                for (X509CRLEntry entry : revokedList) {
                     ((X509CRLEntryImpl)entry).encode(rCerts);
                 }
                 tmp.write(DerValue.tag_Sequence, rCerts);
@@ -490,14 +492,14 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
             sb.append("\nThis Update: " + thisUpdate.toString() + "\n");
         if (nextUpdate != null)
             sb.append("Next Update: " + nextUpdate.toString() + "\n");
-        if (revokedCerts.isEmpty())
+        if (revokedList.isEmpty())
             sb.append("\nNO certificates have been revoked\n");
         else {
-            sb.append("\nRevoked Certificates: " + revokedCerts.size());
+            sb.append("\nRevoked Certificates: " + revokedList.size());
             int i = 1;
-            for (Iterator<X509CRLEntry> iter = revokedCerts.values().iterator();
-                                             iter.hasNext(); i++)
-                sb.append("\n[" + i + "] " + iter.next().toString());
+            for (X509CRLEntry entry: revokedList) {
+                sb.append("\n[" + i++ + "] " + entry.toString());
+            }
         }
         if (extensions != null) {
             Collection<Extension> allExts = extensions.getAllExtensions();
@@ -543,12 +545,12 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
      * false otherwise.
      */
     public boolean isRevoked(Certificate cert) {
-        if (revokedCerts.isEmpty() || (!(cert instanceof X509Certificate))) {
+        if (revokedMap.isEmpty() || (!(cert instanceof X509Certificate))) {
             return false;
         }
         X509Certificate xcert = (X509Certificate) cert;
         X509IssuerSerial issuerSerial = new X509IssuerSerial(xcert);
-        return revokedCerts.containsKey(issuerSerial);
+        return revokedMap.containsKey(issuerSerial);
     }
 
     /**
@@ -638,24 +640,24 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
      * @see X509CRLEntry
      */
     public X509CRLEntry getRevokedCertificate(BigInteger serialNumber) {
-        if (revokedCerts.isEmpty()) {
+        if (revokedMap.isEmpty()) {
             return null;
         }
         // assume this is a direct CRL entry (cert and CRL issuer are the same)
         X509IssuerSerial issuerSerial = new X509IssuerSerial
             (getIssuerX500Principal(), serialNumber);
-        return revokedCerts.get(issuerSerial);
+        return revokedMap.get(issuerSerial);
     }
 
     /**
      * Gets the CRL entry for the given certificate.
      */
     public X509CRLEntry getRevokedCertificate(X509Certificate cert) {
-        if (revokedCerts.isEmpty()) {
+        if (revokedMap.isEmpty()) {
             return null;
         }
         X509IssuerSerial issuerSerial = new X509IssuerSerial(cert);
-        return revokedCerts.get(issuerSerial);
+        return revokedMap.get(issuerSerial);
     }
 
     /**
@@ -667,10 +669,10 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
      * @see X509CRLEntry
      */
     public Set<X509CRLEntry> getRevokedCertificates() {
-        if (revokedCerts.isEmpty()) {
+        if (revokedList.isEmpty()) {
             return null;
         } else {
-            return new HashSet<X509CRLEntry>(revokedCerts.values());
+            return new TreeSet<X509CRLEntry>(revokedList);
         }
     }
 
@@ -905,7 +907,7 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
         if (extensions == null) {
             return null;
         }
-        Set<String> extSet = new HashSet<String>();
+        Set<String> extSet = new TreeSet<>();
         for (Extension ex : extensions.getAllExtensions()) {
             if (ex.isCritical()) {
                 extSet.add(ex.getExtensionId().toString());
@@ -926,7 +928,7 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
         if (extensions == null) {
             return null;
         }
-        Set<String> extSet = new HashSet<String>();
+        Set<String> extSet = new TreeSet<>();
         for (Extension ex : extensions.getAllExtensions()) {
             if (!ex.isCritical()) {
                 extSet.add(ex.getExtensionId().toString());
@@ -1103,7 +1105,8 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
                 entry.setCertificateIssuer(crlIssuer, badCertIssuer);
                 X509IssuerSerial issuerSerial = new X509IssuerSerial
                     (badCertIssuer, entry.getSerialNumber());
-                revokedCerts.put(issuerSerial, entry);
+                revokedMap.put(issuerSerial, entry);
+                revokedList.add(entry);
             }
         }
 
@@ -1208,7 +1211,8 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
     /**
      * Immutable X.509 Certificate Issuer DN and serial number pair
      */
-    private final static class X509IssuerSerial {
+    private final static class X509IssuerSerial
+            implements Comparable<X509IssuerSerial> {
         final X500Principal issuer;
         final BigInteger serial;
         volatile int hashcode = 0;
@@ -1287,5 +1291,13 @@ public class X509CRLImpl extends X509CRL implements DerEncoder {
             }
             return hashcode;
         }
+
+        @Override
+        public int compareTo(X509IssuerSerial another) {
+            int cissuer = issuer.toString()
+                    .compareTo(another.issuer.toString());
+            if (cissuer != 0) return cissuer;
+            return this.serial.compareTo(another.serial);
+        }
     }
 }

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

@@ -1214,7 +1214,7 @@ public class X509CertImpl extends X509Certificate implements DerEncoder {
             if (exts == null) {
                 return null;
             }
-            Set<String> extSet = new HashSet<String>();
+            Set<String> extSet = new TreeSet<>();
             for (Extension ex : exts.getAllExtensions()) {
                 if (ex.isCritical()) {
                     extSet.add(ex.getExtensionId().toString());
@@ -1244,7 +1244,7 @@ public class X509CertImpl extends X509Certificate implements DerEncoder {
             if (exts == null) {
                 return null;
             }
-            Set<String> extSet = new HashSet<String>();
+            Set<String> extSet = new TreeSet<>();
             for (Extension ex : exts.getAllExtensions()) {
                 if (!ex.isCritical()) {
                     extSet.add(ex.getExtensionId().toString());
@@ -1278,10 +1278,14 @@ public class X509CertImpl extends X509Certificate implements DerEncoder {
             if (extensions == null) {
                 return null;
             } else {
-                for (Extension ex : extensions.getAllExtensions()) {
-                    if (ex.getExtensionId().equals(oid)) {
+                Extension ex = extensions.getExtension(oid.toString());
+                if (ex != null) {
+                    return ex;
+                }
+                for (Extension ex2: extensions.getAllExtensions()) {
+                    if (ex2.getExtensionId().equals((Object)oid)) {
                         //XXXX May want to consider cloning this
-                        return ex;
+                        return ex2;
                     }
                 }
                 /* no such extension in this certificate */
@@ -1480,10 +1484,10 @@ public class X509CertImpl extends X509Certificate implements DerEncoder {
         if (names.isEmpty()) {
             return Collections.<List<?>>emptySet();
         }
-        Set<List<?>> newNames = new HashSet<List<?>>();
+        List<List<?>> newNames = new ArrayList<>();
         for (GeneralName gname : names.names()) {
             GeneralNameInterface name = gname.getName();
-            List<Object> nameEntry = new ArrayList<Object>(2);
+            List<Object> nameEntry = new ArrayList<>(2);
             nameEntry.add(Integer.valueOf(name.getType()));
             switch (name.getType()) {
             case GeneralNameInterface.NAME_RFC822:
@@ -1541,12 +1545,12 @@ public class X509CertImpl extends X509Certificate implements DerEncoder {
             }
         }
         if (mustClone) {
-            Set<List<?>> namesCopy = new HashSet<List<?>>();
+            List<List<?>> namesCopy = new ArrayList<>();
             for (List<?> nameEntry : altNames) {
                 Object nameObject = nameEntry.get(1);
                 if (nameObject instanceof byte[]) {
                     List<Object> nameEntryCopy =
-                                        new ArrayList<Object>(nameEntry);
+                                        new ArrayList<>(nameEntry);
                     nameEntryCopy.set(1, ((byte[])nameObject).clone());
                     namesCopy.add(Collections.unmodifiableList(nameEntryCopy));
                 } else {

test/sun/security/x509/X509CRLImpl/OrderAndDup.java

+/*
+ * Copyright (c) 2012, 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 7143872
+ * @summary Improve certificate extension processing
+ */
+import java.io.ByteArrayInputStream;
+import java.math.BigInteger;
+import java.security.KeyPairGenerator;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509CRLEntry;
+import java.util.Date;
+import sun.security.util.DerInputStream;
+import sun.security.util.DerValue;
+import sun.security.x509.*;
+
+public class OrderAndDup {
+    public static void main(String[] args) throws Exception {
+
+        // Generate 20 serial numbers with dup and a special order
+        int count = 20;
+        BigInteger[] serials = new BigInteger[count];
+        for (int i=0; i<count; i++) {
+            serials[i] = BigInteger.valueOf(i*7%10);
+        }
+
+        // Generates a CRL
+        X509CRLEntry[] badCerts = new X509CRLEntry[count];
+        for (int i=0; i<count; i++) {
+            badCerts[i] = new X509CRLEntryImpl(serials[i],
+                    new Date(System.currentTimeMillis()+i*1000));
+        }
+        X500Name owner = new X500Name("CN=CA");
+        X509CRLImpl crl = new X509CRLImpl(owner, new Date(), new Date(), badCerts);
+        KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA");
+        crl.sign(kpg.genKeyPair().getPrivate(), "SHA1withRSA");
+        byte[] data = crl.getEncodedInternal();
+
+        // Check the encoding
+        checkData(crl, data, serials);
+
+        // Load a CRL from raw data
+        CertificateFactory cf = CertificateFactory.getInstance("X.509");
+        X509CRLImpl crl2 = (X509CRLImpl)cf.generateCRL(new ByteArrayInputStream(data));
+
+        // Check the encoding again
+        data = crl2.getEncodedInternal();
+        checkData(crl2, data, serials);
+    }
+
+    // Check the raw data's ASN.1 structure to see if the revoked certs
+    // have the same number and correct order as inserted
+    static void checkData(X509CRLImpl c, byte[] data, BigInteger[] expected)
+            throws Exception {
+        if (c.getRevokedCertificates().size() != expected.length) {
+            throw new Exception("Wrong count in CRL object, now " +
+                    c.getRevokedCertificates().size());
+        }
+        DerValue d1 = new DerValue(data);
+        // revokedCertificates at 5th place of TBSCertList
+        DerValue[] d2 = new DerInputStream(
+                d1.data.getSequence(0)[4].toByteArray())
+                .getSequence(0);
+        if (d2.length != expected.length) {
+            throw new Exception("Wrong count in raw data, now " + d2.length);
+        }
+        for (int i=0; i<d2.length; i++) {
+            // Serial is first in revokedCertificates entry
+            BigInteger bi = d2[i].data.getBigInteger();
+            if (!bi.equals(expected[i])) {
+                throw new Exception("Entry at #" + i + " is " + bi
+                        + ", should be " + expected[i]);
+            }
+        }
+    }
+}
+