PKIX: explicit validation of version number and extension repeats

3 files changed, 75 insertions, 17 deletions

diff --git a/crypto/src/asn1/x509/TBSCertificateStructure.cs b/crypto/src/asn1/x509/TBSCertificateStructure.cs
index fc7c39ba2..9df078539 100644
--- a/crypto/src/asn1/x509/TBSCertificateStructure.cs
+++ b/crypto/src/asn1/x509/TBSCertificateStructure.cs
@@ -1,6 +1,7 @@
 using System;
 
 using Org.BouncyCastle.Asn1.Pkcs;
+using Org.BouncyCastle.Math;
 
 namespace Org.BouncyCastle.Asn1.X509
 {
@@ -78,6 +79,22 @@ namespace Org.BouncyCastle.Asn1.X509
 				version = new DerInteger(0);
 			}
 
+            bool isV1 = false;
+            bool isV2 = false;
+
+            if (version.Value.Equals(BigInteger.Zero))
+            {
+                isV1 = true;
+            }
+            else if (version.Value.Equals(BigInteger.One))
+            {
+                isV2 = true;
+            }
+            else if (!version.Value.Equals(BigInteger.Two))
+            {
+                throw new ArgumentException("version number not recognised");
+            }
+
 			serialNumber = DerInteger.GetInstance(seq[seqStart + 1]);
 
 			signature = AlgorithmIdentifier.GetInstance(seq[seqStart + 2]);
@@ -98,22 +115,36 @@ namespace Org.BouncyCastle.Asn1.X509
 			//
 			subjectPublicKeyInfo = SubjectPublicKeyInfo.GetInstance(seq[seqStart + 6]);
 
-			for (int extras = seq.Count - (seqStart + 6) - 1; extras > 0; extras--)
+            int extras = seq.Count - (seqStart + 6) - 1;
+            if (extras != 0 && isV1)
+                throw new ArgumentException("version 1 certificate contains extra data");
+
+            while (extras > 0)
 			{
 				DerTaggedObject extra = (DerTaggedObject) seq[seqStart + 6 + extras];
 
 				switch (extra.TagNo)
 				{
-					case 1:
-						issuerUniqueID = DerBitString.GetInstance(extra, false);
-						break;
-					case 2:
-						subjectUniqueID = DerBitString.GetInstance(extra, false);
-						break;
-					case 3:
-						extensions = X509Extensions.GetInstance(extra);
-						break;
-				}
+				case 1:
+                {
+					issuerUniqueID = DerBitString.GetInstance(extra, false);
+					break;
+                }
+                case 2:
+                {
+                    subjectUniqueID = DerBitString.GetInstance(extra, false);
+                    break;
+                }
+				case 3:
+                {
+                    if (isV2)
+                        throw new ArgumentException("version 2 certificate cannot contain extensions");
+
+                    extensions = X509Extensions.GetInstance(extra);
+					break;
+                }
+                }
+                extras--;
 			}
 		}
 
diff --git a/crypto/src/asn1/x509/X509Extensions.cs b/crypto/src/asn1/x509/X509Extensions.cs
index 049d728bb..d1b9fa39a 100644
--- a/crypto/src/asn1/x509/X509Extensions.cs
+++ b/crypto/src/asn1/x509/X509Extensions.cs
@@ -224,7 +224,10 @@ namespace Org.BouncyCastle.Asn1.X509
 
 				Asn1OctetString octets = Asn1OctetString.GetInstance(s[s.Count - 1].ToAsn1Object());
 
-				extensions.Add(oid, new X509Extension(isCritical, octets));
+                if (extensions.Contains(oid))
+                    throw new ArgumentException("repeated extension found: " + oid);
+
+                extensions.Add(oid, new X509Extension(isCritical, octets));
 				ordering.Add(oid);
 			}
         }
diff --git a/crypto/src/pkix/PkixCertPathValidator.cs b/crypto/src/pkix/PkixCertPathValidator.cs
index fcfa63837..1d7c00d7d 100644
--- a/crypto/src/pkix/PkixCertPathValidator.cs
+++ b/crypto/src/pkix/PkixCertPathValidator.cs
@@ -3,6 +3,7 @@ using System.Collections;
 using Org.BouncyCastle.Asn1;
 using Org.BouncyCastle.Asn1.X509;
 using Org.BouncyCastle.Crypto;
+using Org.BouncyCastle.Security.Certificates;
 using Org.BouncyCastle.Utilities;
 using Org.BouncyCastle.Utilities.Collections;
 using Org.BouncyCastle.X509;
@@ -81,16 +82,18 @@ namespace Org.BouncyCastle.Pkix
                 trust = PkixCertPathValidatorUtilities.FindTrustAnchor(
 					(X509Certificate)certs[certs.Count - 1],
 					paramsPkix.GetTrustAnchors());
+
+                if (trust == null)
+                    throw new PkixCertPathValidatorException("Trust anchor for certification path not found.", null, certPath, -1);
+
+                CheckCertificate(trust.TrustedCert);
             }
             catch (Exception e)
             {
-                throw new PkixCertPathValidatorException(e.Message, e, certPath, certs.Count - 1);
+                throw new PkixCertPathValidatorException(e.Message, e.InnerException, certPath, certs.Count - 1);
             }
 
-            if (trust == null)
-                throw new PkixCertPathValidatorException("Trust anchor for certification path not found.", null, certPath, -1);
-
-			//
+            //
             // (e), (f), (g) are part of the paramsPkix object.
             //
             IEnumerator certIter;
@@ -253,6 +256,15 @@ namespace Org.BouncyCastle.Pkix
                 //
                 cert = (X509Certificate)certs[index];
 
+                try
+                {
+                    CheckCertificate(cert);
+                }
+                catch (Exception e)
+                {
+                    throw new PkixCertPathValidatorException(e.Message, e.InnerException, certPath, index);
+                }
+
                 //
                 // 6.1.3
                 //
@@ -416,5 +428,17 @@ namespace Org.BouncyCastle.Pkix
 
 			throw new PkixCertPathValidatorException("Path processing failed on policy.", null, certPath, index);
         }
+
+        internal static void CheckCertificate(X509Certificate cert)
+        {
+            try
+            {
+                TbsCertificateStructure.GetInstance(cert.CertificateStructure.TbsCertificate);
+            }
+            catch (CertificateEncodingException e)
+            {
+                throw new Exception("unable to process TBSCertificate", e);
+            }
+        }
     }
 }