Merge pull request #1 from CBitLabs/permissive-parsing

pjsg · web-flow · commit 3b22ea6a78e4 · 2023-10-06T14:20:58.000-04:00
Add support to parse invalid certs but indicate why they are invalid.
diff --git a/core/src/main/java/org/bouncycastle/asn1/x509/Certificate.java b/core/src/main/java/org/bouncycastle/asn1/x509/Certificate.java
@@ -7,6 +7,7 @@
 import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.ASN1TaggedObject;
 import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.util.IllegalArgumentWarningException;
 
 /**
  * an X509Certificate structure.
@@ -53,12 +54,19 @@ private Certificate(
     {
         this.seq = seq;
 
+        IllegalArgumentWarningException exception = null;
+
         //
         // correct x509 certficate
         //
         if (seq.size() == 3)
         {
+          try {
             tbsCert = TBSCertificate.getInstance(seq.getObjectAt(0));
+          } catch (IllegalArgumentWarningException ex) {
+            tbsCert = (TBSCertificate) ex.getObject(TBSCertificate.class);
+            exception = ex;
+          }
             sigAlgId = AlgorithmIdentifier.getInstance(seq.getObjectAt(1));
 
             sig = ASN1BitString.getInstance(seq.getObjectAt(2));
@@ -67,6 +75,10 @@ private Certificate(
         {
             throw new IllegalArgumentException("sequence wrong size for a certificate");
         }
+
+        if (exception != null) {
+          throw new IllegalArgumentWarningException(this, exception);
+        }
     }
 
     public TBSCertificate getTBSCertificate()
@@ -124,6 +136,7 @@ public ASN1BitString getSignature()
         return sig;
     }
 
+    @Override
     public ASN1Primitive toASN1Primitive()
     {
         return seq;
diff --git a/core/src/main/java/org/bouncycastle/asn1/x509/Extensions.java b/core/src/main/java/org/bouncycastle/asn1/x509/Extensions.java
@@ -1,9 +1,5 @@
 package org.bouncycastle.asn1.x509;
 
-import java.util.Enumeration;
-import java.util.Hashtable;
-import java.util.Vector;
-
 import org.bouncycastle.asn1.ASN1Encodable;
 import org.bouncycastle.asn1.ASN1EncodableVector;
 import org.bouncycastle.asn1.ASN1Object;
@@ -12,6 +8,11 @@
 import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.ASN1TaggedObject;
 import org.bouncycastle.asn1.DERSequence;
+import org.bouncycastle.util.IllegalArgumentWarningException;
+
+import java.util.Enumeration;
+import java.util.Hashtable;
+import java.util.Vector;
 
 /**
  * <pre>
@@ -71,19 +72,25 @@ private Extensions(
         ASN1Sequence seq)
     {
         Enumeration e = seq.getObjects();
+        String error = null;
 
         while (e.hasMoreElements())
         {
             Extension ext = Extension.getInstance(e.nextElement());
 
             if (extensions.containsKey(ext.getExtnId()))
             {
-                throw new IllegalArgumentException("repeated extension found: " + ext.getExtnId());
+                error = "repeated extension found: " + ext.getExtnId();
+                continue;
             }
             
             extensions.put(ext.getExtnId(), ext);
             ordering.addElement(ext.getExtnId());
         }
+        
+        if (error != null) {
+          throw new IllegalArgumentWarningException(error, this);
+        }
     }
 
     /**
@@ -163,6 +170,7 @@ public ASN1Encodable getExtensionParsedValue(ASN1ObjectIdentifier oid)
      *        extnValue         OCTET STRING }
      * </pre>
      */
+    @Override
     public ASN1Primitive toASN1Primitive()
     {
         ASN1EncodableVector vec = new ASN1EncodableVector(ordering.size());
diff --git a/core/src/main/java/org/bouncycastle/asn1/x509/TBSCertificate.java b/core/src/main/java/org/bouncycastle/asn1/x509/TBSCertificate.java
@@ -10,8 +10,13 @@
 import org.bouncycastle.asn1.DERSequence;
 import org.bouncycastle.asn1.DERTaggedObject;
 import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.util.IllegalArgumentWarningException;
 import org.bouncycastle.util.Properties;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
 /**
  * The TBSCertificate object.
  * <pre>
@@ -47,6 +52,7 @@ public class TBSCertificate
     ASN1BitString           issuerUniqueId;
     ASN1BitString           subjectUniqueId;
     Extensions              extensions;
+    List<String> errors;
 
     public static TBSCertificate getInstance(
         ASN1TaggedObject obj,
@@ -103,7 +109,8 @@ else if (version.hasValue(1))
         }
         else if (!version.hasValue(2))
         {
-            throw new IllegalArgumentException("version number not recognised");
+          addError(
+              String.format("Certificate version number value %d not 0, 1 or 2", version.getValue()));
         }
 
         serialNumber = ASN1Integer.getInstance(seq.getObjectAt(seqStart + 1));
@@ -129,7 +136,8 @@ else if (!version.hasValue(2))
         int extras = seq.size() - (seqStart + 6) - 1;
         if (extras != 0 && isV1)
         {
-            throw new IllegalArgumentException("version 1 certificate contains extra data");
+          addError("version 1 certificate contains extra data");
+          extras = 0; // Ignore the extra data
         }
         
         while (extras > 0)
@@ -147,15 +155,43 @@ else if (!version.hasValue(2))
             case 3:
                 if (isV2)
                 {
-                    throw new IllegalArgumentException("version 2 certificate cannot contain extensions");
+                  addError("version 2 certificate cannot contain extensions");
+                  throw new IllegalArgumentWarningException(errors, this);
+                }
+                try {
+                  extensions = Extensions.getInstance(ASN1Sequence.getInstance(extra, true));
+                } catch (IllegalArgumentWarningException ex) {
+                  extensions = ex.getObject(Extensions.class);
+                  addErrors(ex.getMessages());
                 }
-                extensions = Extensions.getInstance(ASN1Sequence.getInstance(extra, true));
                 break;
             default:
-                throw new IllegalArgumentException("Unknown tag encountered in structure: " + extra.getTagNo());
+              addError("Unknown tag encountered in structure: " + extra.getTagNo());
+              throw new IllegalArgumentWarningException(errors, this);
             }
             extras--;
         }
+        
+        if (errors != null) {
+          throw new IllegalArgumentWarningException(errors, this);
+        }
+    }
+    
+    private void addError(String error) {
+      if (errors == null) {
+        errors = new ArrayList<>();
+      }
+      errors.add(error);
+    }
+    
+    private void addErrors(List<String> errors) {
+      for (String error : errors) {
+        addError(error);
+      }
+    }
+
+    public Collection<String> getErrors() {
+      return errors;
     }
 
     public int getVersionNumber()
@@ -218,6 +254,7 @@ public Extensions getExtensions()
         return extensions;
     }
 
+    @Override
     public ASN1Primitive toASN1Primitive()
     {
         if (Properties.getPropertyValue("org.bouncycastle.x509.allow_non-der_tbscert") != null)
diff --git a/core/src/main/java/org/bouncycastle/crypto/params/RSAKeyParameters.java b/core/src/main/java/org/bouncycastle/crypto/params/RSAKeyParameters.java
@@ -1,12 +1,15 @@
 package org.bouncycastle.crypto.params;
 
-import java.math.BigInteger;
-
 import org.bouncycastle.crypto.CryptoServicesRegistrar;
 import org.bouncycastle.math.Primes;
 import org.bouncycastle.util.BigIntegers;
+import org.bouncycastle.util.IllegalArgumentWarningException;
 import org.bouncycastle.util.Properties;
 
+import java.math.BigInteger;
+import java.util.ArrayList;
+import java.util.List;
+
 public class RSAKeyParameters
     extends AsymmetricKeyParameter
 {
@@ -41,16 +44,20 @@ public RSAKeyParameters(
     {
         super(isPrivate);
 
+        this.modulus = modulus;
+        this.exponent = exponent;
+
         if (!isPrivate)
         {
             if ((exponent.intValue() & 1) == 0)
             {
-                throw new IllegalArgumentException("RSA publicExponent is even");
+                throw new IllegalArgumentWarningException("RSA publicExponent is even", this);
             }
         }
   
-        this.modulus = validated.contains(modulus) ? modulus : validate(modulus, isInternal);
-        this.exponent = exponent;
+        if (!validated.contains(modulus)) {
+            validate(modulus, isInternal);
+        }
     }
 
     private BigInteger validate(BigInteger modulus, boolean isInternal)
@@ -62,44 +69,48 @@ private BigInteger validate(BigInteger modulus, boolean isInternal)
             return modulus;
         }
 
+        List<String> issues = new ArrayList<>();
+
         if ((modulus.intValue() & 1) == 0)
         {
-            throw new IllegalArgumentException("RSA modulus is even");
+            issues.add("RSA modulus is even");
         }
 
         // If you need to set this you need to have a serious word to whoever is generating
         // your keys.
-        if (Properties.isOverrideSet("org.bouncycastle.rsa.allow_unsafe_mod"))
+        if (!Properties.isOverrideSet("org.bouncycastle.rsa.allow_unsafe_mod"))
         {
-            return modulus;
-        }
-
-        int maxBitLength = Properties.asInteger("org.bouncycastle.rsa.max_size", 15360);
+            int maxBitLength = Properties.asInteger("org.bouncycastle.rsa.max_size", 15360);
 
-        int modBitLength = modulus.bitLength();
-        if (maxBitLength < modBitLength)
-        {
-            throw new IllegalArgumentException("modulus value out of range");
-        }
+            int modBitLength = modulus.bitLength();
+            if (maxBitLength < modBitLength)
+            {
+                issues.add("modulus value out of range");
+            }
 
-        if (!modulus.gcd(SMALL_PRIMES_PRODUCT).equals(ONE))
-        {
-            throw new IllegalArgumentException("RSA modulus has a small prime factor");
-        }
+            if (!modulus.gcd(SMALL_PRIMES_PRODUCT).equals(ONE))
+            {
+                issues.add("RSA modulus has a small prime factor");
+            }
 
-        int bits = modulus.bitLength() / 2;
-        int iterations = Properties.asInteger("org.bouncycastle.rsa.max_mr_tests", getMRIterations(bits));
+            int bits = modulus.bitLength() / 2;
+            int iterations = Properties.asInteger("org.bouncycastle.rsa.max_mr_tests", getMRIterations(bits));
 
-        if (iterations > 0)
-        {
-            Primes.MROutput mr = Primes.enhancedMRProbablePrimeTest(modulus, CryptoServicesRegistrar.getSecureRandom(), iterations);
-            if (!mr.isProvablyComposite())
+            if (iterations > 0)
             {
-                throw new IllegalArgumentException("RSA modulus is not composite");
+                Primes.MROutput mr = Primes.enhancedMRProbablePrimeTest(modulus, CryptoServicesRegistrar.getSecureRandom(), iterations);
+                if (!mr.isProvablyComposite())
+                {
+                    issues.add("RSA modulus is not composite");
+                }
+            }
+
+            if (!issues.isEmpty()) {
+              throw new IllegalArgumentWarningException(issues, this);
             }
-        }
 
-        validated.add(modulus);
+            validated.add(modulus);
+        }
         
         return modulus;
     }
diff --git a/core/src/main/java/org/bouncycastle/util/IllegalArgumentWarningException.java b/core/src/main/java/org/bouncycastle/util/IllegalArgumentWarningException.java
@@ -0,0 +1,41 @@
+package org.bouncycastle.util;
+
+import java.util.Collections;
+import java.util.List;
+
+public class IllegalArgumentWarningException extends IllegalArgumentException {
+  
+  private static final long serialVersionUID = 5735291408274180892L;
+  List<String> messages;
+  Object object;
+  
+  public IllegalArgumentWarningException(List<String> messages, Object object, Throwable cause) {
+    super(messages.get(0), cause);
+    this.messages = messages;
+    this.object = object;
+  }
+
+  public IllegalArgumentWarningException(List<String> messages, Object object) {
+    this(messages, object, null);
+  }
+
+  public IllegalArgumentWarningException(String message, Object object) {
+    this(Collections.singletonList(message), object, null);
+  }
+
+  public IllegalArgumentWarningException(Object object, Throwable cause) {
+    this(Collections.singletonList(cause.getMessage()), object, cause);
+  }
+
+  public List<String> getMessages() {
+    return messages;
+  }
+
+  public <T> T getObject(Class<T> clazz) {
+    if (clazz.isInstance(object)) {
+      return (T) object;
+    }
+    throw new IllegalArgumentException(messages.get(0), this);
+  }
+
+}
diff --git a/core/src/main/java/org/bouncycastle/util/test/SimpleTest.java b/core/src/main/java/org/bouncycastle/util/test/SimpleTest.java
@@ -47,7 +47,7 @@ protected void isEquals(
     {
         if (!a.equals(b))
         {
-            throw new TestFailedException(SimpleTestResult.failed(this, "no message"));
+            throw new TestFailedException(SimpleTestResult.failed(this, String.format("isEqual fail: %s != %s", a, b)));
         }
     }
 
@@ -57,7 +57,7 @@ protected void isEquals(
     {
         if (a != b)
         {
-            throw new TestFailedException(SimpleTestResult.failed(this, "no message"));
+            throw new TestFailedException(SimpleTestResult.failed(this, String.format("isEqual fail: %s != %s", a, b)));
         }
     }
 
diff --git a/prov/src/test/java/org/bouncycastle/jce/provider/test/CertPathValidatorTest.java b/prov/src/test/java/org/bouncycastle/jce/provider/test/CertPathValidatorTest.java

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@`
`7`	`7`	`import org.bouncycastle.asn1.ASN1Sequence;`
`8`	`8`	`import org.bouncycastle.asn1.ASN1TaggedObject;`
`9`	`9`	`import org.bouncycastle.asn1.x500.X500Name;`
	`10`	`+import org.bouncycastle.util.IllegalArgumentWarningException;`
`10`	`11`
`11`	`12`	`/**`
`12`	`13`	`* an X509Certificate structure.`
`@@ -53,12 +54,19 @@ private Certificate(`
`53`	`54`	`{`
`54`	`55`	`this.seq = seq;`
`55`	`56`
	`57`	`+ IllegalArgumentWarningException exception = null;`
	`58`	`+`
`56`	`59`	`//`
`57`	`60`	`// correct x509 certficate`
`58`	`61`	`//`
`59`	`62`	`if (seq.size() == 3)`
`60`	`63`	`{`
	`64`	`+ try {`
`61`	`65`	`tbsCert = TBSCertificate.getInstance(seq.getObjectAt(0));`
	`66`	`+ } catch (IllegalArgumentWarningException ex) {`
	`67`	`+ tbsCert = (TBSCertificate) ex.getObject(TBSCertificate.class);`
	`68`	`+ exception = ex;`
	`69`	`+ }`
`62`	`70`	`sigAlgId = AlgorithmIdentifier.getInstance(seq.getObjectAt(1));`
`63`	`71`
`64`	`72`	`sig = ASN1BitString.getInstance(seq.getObjectAt(2));`
`@@ -67,6 +75,10 @@ private Certificate(`
`67`	`75`	`{`
`68`	`76`	`throw new IllegalArgumentException("sequence wrong size for a certificate");`
`69`	`77`	`}`
	`78`	`+`
	`79`	`+ if (exception != null) {`
	`80`	`+ throw new IllegalArgumentWarningException(this, exception);`
	`81`	`+ }`
`70`	`82`	`}`
`71`	`83`
`72`	`84`	`public TBSCertificate getTBSCertificate()`
`@@ -124,6 +136,7 @@ public ASN1BitString getSignature()`
`124`	`136`	`return sig;`
`125`	`137`	`}`
`126`	`138`
	`139`	`+ @Override`
`127`	`140`	`public ASN1Primitive toASN1Primitive()`
`128`	`141`	`{`
`129`	`142`	`return seq;`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ protected void isEquals(`
`47`	`47`	`{`
`48`	`48`	`if (!a.equals(b))`
`49`	`49`	`{`
`50`		`- throw new TestFailedException(SimpleTestResult.failed(this, "no message"));`
	`50`	`+ throw new TestFailedException(SimpleTestResult.failed(this, String.format("isEqual fail: %s != %s", a, b)));`
`51`	`51`	`}`
`52`	`52`	`}`
`53`	`53`
`@@ -57,7 +57,7 @@ protected void isEquals(`
`57`	`57`	`{`
`58`	`58`	`if (a != b)`
`59`	`59`	`{`
`60`		`- throw new TestFailedException(SimpleTestResult.failed(this, "no message"));`
	`60`	`+ throw new TestFailedException(SimpleTestResult.failed(this, String.format("isEqual fail: %s != %s", a, b)));`
`61`	`61`	`}`
`62`	`62`	`}`
`63`	`63`