added explicit validation of version number and extension repeats

beconf · Aug 13, 2017 · 8df14f6 · 8df14f6
1 parent 08c0045
commit 8df14f6
Show file tree

Hide file tree

Showing 6 changed files with 1,246 additions and 192 deletions.
diff --git a/core/src/main/java/org/bouncycastle/asn1/x509/Extensions.java b/core/src/main/java/org/bouncycastle/asn1/x509/Extensions.java
@@ -56,6 +56,11 @@ private Extensions(
         {
             Extension ext = Extension.getInstance(e.nextElement());
 
+            if (extensions.containsKey(ext.getExtnId()))
+            {
+                throw new IllegalArgumentException("repeated extension found: " + ext.getExtnId());
+            }
+
             extensions.put(ext.getExtnId(), ext);
             ordering.addElement(ext.getExtnId());
         }

diff --git a/core/src/main/java/org/bouncycastle/asn1/x509/TBSCertificate.java b/core/src/main/java/org/bouncycastle/asn1/x509/TBSCertificate.java
@@ -1,5 +1,7 @@
 package org.bouncycastle.asn1.x509;
 
+import java.math.BigInteger;
+
 import org.bouncycastle.asn1.ASN1Integer;
 import org.bouncycastle.asn1.ASN1Object;
 import org.bouncycastle.asn1.ASN1Primitive;
@@ -87,6 +89,22 @@ private TBSCertificate(
             version = new ASN1Integer(0);
         }
 
+        boolean isV1 = false;
+        boolean isV2 = false;
+
+        if (version.getValue().equals(BigInteger.valueOf(0)))
+        {
+            isV1 = true;
+        }
+        else if (version.getValue().equals(BigInteger.valueOf(1)))
+        {
+            isV2 = true;
+        }
+        else if (!version.getValue().equals(BigInteger.valueOf(2)))
+        {
+            throw new IllegalArgumentException("version number not recognised");
+        }
+
         serialNumber = ASN1Integer.getInstance(seq.getObjectAt(seqStart + 1));
 
         signature = AlgorithmIdentifier.getInstance(seq.getObjectAt(seqStart + 2));
@@ -107,7 +125,13 @@ private TBSCertificate(
         //
         subjectPublicKeyInfo = SubjectPublicKeyInfo.getInstance(seq.getObjectAt(seqStart + 6));
 
-        for (int extras = seq.size() - (seqStart + 6) - 1; extras > 0; extras--)
+        int extras = seq.size() - (seqStart + 6) - 1;
+        if (extras != 0 && isV1)
+        {
+            throw new IllegalArgumentException("version 1 certificate contains extra data");
+        }
+
+        while (extras > 0)
         {
             ASN1TaggedObject extra = (ASN1TaggedObject)seq.getObjectAt(seqStart + 6 + extras);
 
@@ -120,8 +144,13 @@ private TBSCertificate(
                 subjectUniqueId = DERBitString.getInstance(extra, false);
                 break;
             case 3:
+                if (isV2)
+                {
+                    throw new IllegalArgumentException("version 2 certificate cannot contain extensions");
+                }
                 extensions = Extensions.getInstance(ASN1Sequence.getInstance(extra, true));
             }
+            extras--;
         }
     }
 

diff --git a/core/src/main/java/org/bouncycastle/asn1/x509/TBSCertificateStructure.java b/core/src/main/java/org/bouncycastle/asn1/x509/TBSCertificateStructure.java
@@ -29,6 +29,7 @@
  * <p>
  * Note: issuerUniqueID and subjectUniqueID are both deprecated by the IETF. This class
  * will parse them, but you really shouldn't be creating new ones.
+ * @deprecated use TBSCertificate
  */
 public class TBSCertificateStructure
     extends ASN1Object

diff --git a/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/x509/CertificateFactory.java b/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/x509/CertificateFactory.java
@@ -237,7 +237,7 @@ else if (currentStream != in) // reset if input stream has changed
         }
         catch (Exception e)
         {
-            throw new ExCertificateException(e);
+            throw new ExCertificateException("parsing issue: " + e.getMessage(), e);
         }
     }
 

diff --git a/prov/src/main/java/org/bouncycastle/jce/provider/PKIXCertPathValidatorSpi.java b/prov/src/main/java/org/bouncycastle/jce/provider/PKIXCertPathValidatorSpi.java
@@ -1,12 +1,14 @@
 package org.bouncycastle.jce.provider;
 
+import java.math.BigInteger;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.PublicKey;
 import java.security.cert.CertPath;
 import java.security.cert.CertPathParameters;
 import java.security.cert.CertPathValidatorException;
 import java.security.cert.CertPathValidatorResult;
 import java.security.cert.CertPathValidatorSpi;
+import java.security.cert.CertificateEncodingException;
 import java.security.cert.PKIXCertPathChecker;
 import java.security.cert.PKIXCertPathValidatorResult;
 import java.security.cert.PKIXParameters;
@@ -23,6 +25,7 @@
 import org.bouncycastle.asn1.x500.X500Name;
 import org.bouncycastle.asn1.x509.AlgorithmIdentifier;
 import org.bouncycastle.asn1.x509.Extension;
+import org.bouncycastle.asn1.x509.TBSCertificate;
 import org.bouncycastle.jcajce.PKIXExtendedBuilderParameters;
 import org.bouncycastle.jcajce.PKIXExtendedParameters;
 import org.bouncycastle.jcajce.util.BCJcaJceHelper;
@@ -116,15 +119,17 @@ else if (params instanceof PKIXExtendedParameters)
         {
             trust = CertPathValidatorUtilities.findTrustAnchor((X509Certificate) certs.get(certs.size() - 1),
                     paramsPKIX.getTrustAnchors(), paramsPKIX.getSigProvider());
+
+            if (trust == null)
+            {
+                throw new CertPathValidatorException("Trust anchor for certification path not found.", null, certPath, -1);
+            }
+
+            checkCertificate(trust.getTrustedCert());
         }
         catch (AnnotatedException e)
         {
-            throw new CertPathValidatorException(e.getMessage(), e, certPath, certs.size() - 1);
-        }
-
-        if (trust == null)
-        {
-            throw new CertPathValidatorException("Trust anchor for certification path not found.", null, certPath, -1);
+            throw new CertPathValidatorException(e.getMessage(), e.getUnderlyingException(), certPath, certs.size() - 1);
         }
 
         // RFC 5280 - CRLs must originate from the same trust anchor as the target certificate.
@@ -292,6 +297,15 @@ else if (params instanceof PKIXExtendedParameters)
             cert = (X509Certificate) certs.get(index);
             boolean verificationAlreadyPerformed = (index == certs.size() - 1);
 
+            try
+            {
+                checkCertificate(cert);
+            }
+            catch (AnnotatedException e)
+            {
+                throw new CertPathValidatorException(e.getMessage(), e.getUnderlyingException(), certPath, index);
+            }
+
             //
             // 6.1.3
             //
@@ -454,4 +468,20 @@ else if (params instanceof PKIXExtendedParameters)
         throw new CertPathValidatorException("Path processing failed on policy.", null, certPath, index);
     }
 
+    static void checkCertificate(X509Certificate cert)
+        throws AnnotatedException
+    {
+        try
+        {
+            TBSCertificate.getInstance(cert.getTBSCertificate());
+        }
+        catch (CertificateEncodingException e)
+        {
+            throw new AnnotatedException("unable to process TBSCertificate");
+        }
+        catch (IllegalArgumentException e)
+        {
+            throw new AnnotatedException(e.getMessage());
+        }
+    }
 }
-Original file line number
+Diff line change
@@ Expand Up @@
             }
             catch (Exception e)
             {
-                throw new ExCertificateException(e);
+                throw new ExCertificateException("parsing issue: " + e.getMessage(), e);
             }
         }
@@ Expand Down @@