Generate error on unknown critical extension during path validation

Previously unknown critical extensions were rejected during
X509_Certificate constructor, which inhibited inspecting other
parts of such a certificate. Refactored the certificate extensions
code so that the path validation routine performs this check only.
Additionally, added an interface for extensions to inspect the path
during path validation. TODOs were added in places where existing path
validation code can use the new interface.

Fixes GH #449.

9 files changed, 145 insertions, 19 deletions

diff --git a/src/lib/cert/x509/cert_status.h b/src/lib/cert/x509/cert_status.h
index 6e8635237..52b65fb57 100644
--- a/src/lib/cert/x509/cert_status.h
+++ b/src/lib/cert/x509/cert_status.h
@@ -47,6 +47,8 @@ enum class Certificate_Status_Code {
 
    CERT_NAME_NOMATCH,
 
+   UNKNOWN_CRITICAL_EXTENSION,
+
    // Hard failures
    CERT_IS_REVOKED = 5000,
    CRL_BAD_SIGNATURE,
diff --git a/src/lib/cert/x509/crl_ent.cpp b/src/lib/cert/x509/crl_ent.cpp
index d6923f714..7074f0609 100644
--- a/src/lib/cert/x509/crl_ent.cpp
+++ b/src/lib/cert/x509/crl_ent.cpp
@@ -6,6 +6,7 @@
 */
 
 #include <botan/crl_ent.h>
+#include <botan/x509cert.h>
 #include <botan/x509_ext.h>
 #include <botan/der_enc.h>
 #include <botan/ber_dec.h>
diff --git a/src/lib/cert/x509/crl_ent.h b/src/lib/cert/x509/crl_ent.h
index 11ab34365..4be508812 100644
--- a/src/lib/cert/x509/crl_ent.h
+++ b/src/lib/cert/x509/crl_ent.h
@@ -8,11 +8,12 @@
 #ifndef BOTAN_CRL_ENTRY_H__
 #define BOTAN_CRL_ENTRY_H__
 
-#include <botan/x509cert.h>
 #include <botan/asn1_time.h>
 
 namespace Botan {
 
+class X509_Certificate;
+
 /**
 * X.509v2 CRL Reason Code.
 */
diff --git a/src/lib/cert/x509/x509_crl.h b/src/lib/cert/x509/x509_crl.h
index 29057e944..dab4d5153 100644
--- a/src/lib/cert/x509/x509_crl.h
+++ b/src/lib/cert/x509/x509_crl.h
@@ -9,7 +9,9 @@
 #define BOTAN_X509_CRL_H__
 
 #include <botan/x509_obj.h>
+#include <botan/x509_dn.h>
 #include <botan/crl_ent.h>
+#include <botan/datastor.h>
 #include <vector>
 
 namespace Botan {
diff --git a/src/lib/cert/x509/x509_ext.cpp b/src/lib/cert/x509/x509_ext.cpp
index 47fd909eb..0125719cc 100644
--- a/src/lib/cert/x509/x509_ext.cpp
+++ b/src/lib/cert/x509/x509_ext.cpp
@@ -55,7 +55,7 @@ Extensions::Extensions(const Extensions& extensions) : ASN1_Object()
 * Extensions Assignment Operator
 */
 Extensions& Extensions::operator=(const Extensions& other)
-   {   
+   {
    m_extensions.clear();
 
    for(size_t i = 0; i != other.m_extensions.size(); ++i)
@@ -63,6 +63,7 @@ Extensions& Extensions::operator=(const Extensions& other)
          std::make_pair(std::unique_ptr<Certificate_Extension>(other.m_extensions[i].first->copy()),
                         other.m_extensions[i].second));
 
+   m_extensions_raw = other.m_extensions_raw;
    m_throw_on_unknown_critical = other.m_throw_on_unknown_critical;
 
    return (*this);
@@ -82,6 +83,15 @@ void Extensions::add(Certificate_Extension* extn, bool critical)
    m_extensions_raw.emplace(extn->oid_of(), std::make_pair(extn->encode_inner(), critical));
    }
 
+std::vector<std::pair<std::unique_ptr<Certificate_Extension>, bool>> Extensions::extensions() const
+   {
+   std::vector<std::pair<std::unique_ptr<Certificate_Extension>, bool>> exts;
+   for(auto& ext : m_extensions)
+      {
+      exts.push_back(std::make_pair(std::unique_ptr<Certificate_Extension>(ext.first->copy()), ext.second));
+      }
+   return exts;
+   }
 
 std::map<OID, std::pair<std::vector<byte>, bool>> Extensions::extensions_raw() const
    {
@@ -174,6 +184,11 @@ void Extensions::contents_to(Data_Store& subject_info,
       }
    }
 
+bool Extensions::is_known_extension(const OID& oid)
+   {
+   return get_extension(oid) != nullptr;
+   }
+
 
 namespace Cert_Extension {
 
diff --git a/src/lib/cert/x509/x509_ext.h b/src/lib/cert/x509/x509_ext.h
index ac456b998..2dfc71509 100644
--- a/src/lib/cert/x509/x509_ext.h
+++ b/src/lib/cert/x509/x509_ext.h
@@ -10,11 +10,17 @@
 
 #include <botan/asn1_obj.h>
 #include <botan/asn1_oid.h>
+#include <botan/asn1_alt_name.h>
+#include <botan/cert_status.h>
 #include <botan/datastor.h>
+#include <botan/name_constraint.h>
+#include <botan/key_constraint.h>
 #include <botan/crl_ent.h>
 
 namespace Botan {
 
+class X509_Certificate;
+
 /**
 * X.509 Certificate Extension
 */
@@ -46,6 +52,19 @@ class BOTAN_DLL Certificate_Extension
       */
       virtual std::string oid_name() const = 0;
 
+      /*
+      * Callback visited during path validation
+      *
+      * If an error occurs during validation of this extension,
+      * an appropriate status code shall be added to status.
+      *
+      * @param current_cert Certificate that contains this extension
+      * @param status Certificate validation status codes for current_cert
+      * @param cert_path Certificate path which is currently validated
+      */
+      virtual void validate(const X509_Certificate& current_cert, std::set<Certificate_Status_Code>& status,
+            const std::vector<X509_Certificate>& cert_path) = 0;
+
       virtual ~Certificate_Extension() {}
    protected:
       friend class Extensions;
@@ -67,8 +86,12 @@ class BOTAN_DLL Extensions : public ASN1_Object
 
       void add(Certificate_Extension* extn, bool critical = false);
 
+      std::vector<std::pair<std::unique_ptr<Certificate_Extension>, bool>> extensions() const;
+
       std::map<OID, std::pair<std::vector<byte>, bool>> extensions_raw() const;
 
+      static bool is_known_extension(const OID& oid);
+
       Extensions& operator=(const Extensions&);
 
       Extensions(const Extensions&);
@@ -101,6 +124,9 @@ class BOTAN_DLL Basic_Constraints final : public Certificate_Extension
 
       bool get_is_ca() const { return m_is_ca; }
       size_t get_path_limit() const;
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.BasicConstraints"; }
@@ -124,6 +150,9 @@ class BOTAN_DLL Key_Usage final : public Certificate_Extension
       explicit Key_Usage(Key_Constraints c = NO_CONSTRAINTS) : m_constraints(c) {}
 
       Key_Constraints get_constraints() const { return m_constraints; }
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override { return "X509v3.KeyUsage"; }
 
@@ -149,6 +178,9 @@ class BOTAN_DLL Subject_Key_ID final : public Certificate_Extension
       explicit Subject_Key_ID(const std::vector<byte>&);
 
       std::vector<byte> get_key_id() const { return m_key_id; }
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.SubjectKeyIdentifier"; }
@@ -174,6 +206,9 @@ class BOTAN_DLL Authority_Key_ID final : public Certificate_Extension
       explicit Authority_Key_ID(const std::vector<byte>& k) : m_key_id(k) {}
 
       std::vector<byte> get_key_id() const { return m_key_id; }
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.AuthorityKeyIdentifier"; }
@@ -198,6 +233,9 @@ class BOTAN_DLL Alternative_Name : public Certificate_Extension
       Alternative_Name(const AlternativeName&, const std::string& oid_name);
 
       Alternative_Name(const std::string&, const std::string&);
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override { return m_oid_name_str; }
 
@@ -247,6 +285,9 @@ class BOTAN_DLL Extended_Key_Usage final : public Certificate_Extension
       explicit Extended_Key_Usage(const std::vector<OID>& o) : m_oids(o) {}
 
       std::vector<OID> get_oids() const { return m_oids; }
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.ExtendedKeyUsage"; }
@@ -270,6 +311,9 @@ class BOTAN_DLL Name_Constraints : public Certificate_Extension
 
       Name_Constraints() {}
       Name_Constraints(const NameConstraints &nc) : m_name_constraints(nc) {}
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.NameConstraints"; }
@@ -295,6 +339,9 @@ class BOTAN_DLL Certificate_Policies final : public Certificate_Extension
       explicit Certificate_Policies(const std::vector<OID>& o) : m_oids(o) {}
 
       std::vector<OID> get_oids() const { return m_oids; }
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.CertificatePolicies"; }
@@ -318,6 +365,8 @@ class BOTAN_DLL Authority_Information_Access final : public Certificate_Extensio
       explicit Authority_Information_Access(const std::string& ocsp) :
          m_ocsp_responder(ocsp) {}
 
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "PKIX.AuthorityInformationAccess"; }
@@ -344,6 +393,9 @@ class BOTAN_DLL CRL_Number final : public Certificate_Extension
       CRL_Number(size_t n) : m_has_value(true), m_crl_number(n) {}
 
       size_t get_crl_number() const;
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override { return "X509v3.CRLNumber"; }
 
@@ -368,6 +420,9 @@ class BOTAN_DLL CRL_ReasonCode final : public Certificate_Extension
       explicit CRL_ReasonCode(CRL_Code r = UNSPECIFIED) : m_reason(r) {}
 
       CRL_Code get_reason() const { return m_reason; }
+
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override { return "X509v3.ReasonCode"; }
 
@@ -407,6 +462,8 @@ class BOTAN_DLL CRL_Distribution_Points final : public Certificate_Extension
       std::vector<Distribution_Point> distribution_points() const
          { return m_distribution_points; }
 
+      void validate(const X509_Certificate& /* current_cert */, std::set<Certificate_Status_Code>& /* status */,
+                  const std::vector<X509_Certificate>& /* cert_path */) {}
    private:
       std::string oid_name() const override
          { return "X509v3.CRLDistributionPoints"; }
diff --git a/src/lib/cert/x509/x509cert.cpp b/src/lib/cert/x509/x509cert.cpp
index 959cddb53..f68956859 100644
--- a/src/lib/cert/x509/x509cert.cpp
+++ b/src/lib/cert/x509/x509cert.cpp
@@ -42,9 +42,10 @@ std::vector<std::string> lookup_oids(const std::vector<std::string>& in)
 * X509_Certificate Constructor
 */
 X509_Certificate::X509_Certificate(DataSource& in) :
-   X509_Object(in, "CERTIFICATE/X509 CERTIFICATE")
+   X509_Object(in, "CERTIFICATE/X509 CERTIFICATE"),
+   m_self_signed(false),
+   m_v3_extensions(false)
    {
-   m_self_signed = false;
    do_decode();
    }
 
@@ -52,9 +53,10 @@ X509_Certificate::X509_Certificate(DataSource& in) :
 * X509_Certificate Constructor
 */
 X509_Certificate::X509_Certificate(const std::string& in) :
-   X509_Object(in, "CERTIFICATE/X509 CERTIFICATE")
+   X509_Object(in, "CERTIFICATE/X509 CERTIFICATE"),
+   m_self_signed(false),
+   m_v3_extensions(false)
    {
-   m_self_signed = false;
    do_decode();
    }
 
@@ -62,12 +64,39 @@ X509_Certificate::X509_Certificate(const std::string& in) :
 * X509_Certificate Constructor
 */
 X509_Certificate::X509_Certificate(const std::vector<byte>& in) :
-   X509_Object(in, "CERTIFICATE/X509 CERTIFICATE")
+   X509_Object(in, "CERTIFICATE/X509 CERTIFICATE"),
+   m_self_signed(false),
+   m_v3_extensions(false)
    {
-   m_self_signed = false;
    do_decode();
    }
 
+X509_Certificate::X509_Certificate(const X509_Certificate& other) :
+   X509_Object(other)
+   {
+   m_subject = other.m_subject;
+   m_issuer = other.m_issuer;
+   m_self_signed = other.m_self_signed;
+   m_v3_extensions = other.m_v3_extensions;
+   }
+
+X509_Certificate& X509_Certificate::operator=(const X509_Certificate& other)
+   {
+   if(&other == this)
+      {
+      return *this;
+      }
+   else
+      {
+      m_subject = other.m_subject;
+      m_issuer = other.m_issuer;
+      m_self_signed = other.m_self_signed;
+      m_v3_extensions = other.m_v3_extensions;
+      }
+   return *this;
+   }
+
+
 /*
 * Decode the TBSCertificate data
 */
@@ -120,12 +149,8 @@ void X509_Certificate::force_decode()
    if(v3_exts_data.type_tag == 3 &&
       v3_exts_data.class_tag == ASN1_Tag(CONSTRUCTED | CONTEXT_SPECIFIC))
       {
-      Extensions extensions;
-
-      BER_Decoder(v3_exts_data.value).decode(extensions).verify_end();
-
-      m_v3_extensions = extensions.extensions_raw();
-      extensions.contents_to(m_subject, m_issuer);
+      BER_Decoder(v3_exts_data.value).decode(m_v3_extensions).verify_end();
+      m_v3_extensions.contents_to(m_subject, m_issuer);
       }
    else if(v3_exts_data.type_tag != NO_OBJECT)
       throw BER_Bad_Tag("Unknown tag in X.509 cert",
@@ -332,7 +357,7 @@ std::vector<std::string> X509_Certificate::policies() const
    return lookup_oids(m_subject.get("X509v3.CertificatePolicies"));
    }
 
-std::map<OID, std::pair<std::vector<byte>, bool>> X509_Certificate::v3_extensions() const
+Extensions X509_Certificate::v3_extensions() const
    {
    return m_v3_extensions;
    }
diff --git a/src/lib/cert/x509/x509cert.h b/src/lib/cert/x509/x509cert.h
index 54d82b1b4..c521cf7ca 100644
--- a/src/lib/cert/x509/x509cert.h
+++ b/src/lib/cert/x509/x509cert.h
@@ -11,11 +11,13 @@
 #include <botan/x509_obj.h>
 #include <botan/x509_dn.h>
 #include <botan/x509_key.h>
+#include <botan/x509_ext.h>
 #include <botan/asn1_alt_name.h>
 #include <botan/datastor.h>
 #include <botan/key_constraint.h>
 #include <botan/name_constraint.h>
 #include <map>
+#include <memory>
 
 namespace Botan {
 
@@ -191,10 +193,10 @@ class BOTAN_DLL X509_Certificate final : public X509_Object
       std::vector<std::string> policies() const;
 
       /**
-      * Get all extensions of this certificate indexed by oid.
-      * @return extension values and critical flag
+      * Get all extensions of this certificate.
+      * @return certificate extensions
       */
-      std::map<OID, std::pair<std::vector<byte>, bool>> v3_extensions() const;
+      Extensions v3_extensions() const;
 
       /**
       * Return the listed address of an OCSP responder, or empty if not set
@@ -250,6 +252,10 @@ class BOTAN_DLL X509_Certificate final : public X509_Object
 
       explicit X509_Certificate(const std::vector<byte>& in);
 
+      X509_Certificate(const X509_Certificate& other);
+
+      X509_Certificate& operator=(const X509_Certificate& other);
+
    private:
       void force_decode() override;
       friend class X509_CA;
@@ -259,7 +265,7 @@ class BOTAN_DLL X509_Certificate final : public X509_Object
 
       Data_Store m_subject, m_issuer;
       bool m_self_signed;
-      std::map<OID, std::pair<std::vector<byte>, bool>> m_v3_extensions;
+      Extensions m_v3_extensions;
    };
 
 /**
diff --git a/src/lib/cert/x509/x509path.cpp b/src/lib/cert/x509/x509path.cpp
index dd9df6f51..dfd45f951 100644
--- a/src/lib/cert/x509/x509path.cpp
+++ b/src/lib/cert/x509/x509path.cpp
@@ -113,10 +113,12 @@ check_chain(const std::vector<X509_Certificate>& cert_path,
 
       // Check issuer constraints
 
+      // TODO: put into Certificate_Extensions::Basic_Constraints::validate()
       // Don't require CA bit set on self-signed end entity cert
       if(!issuer.is_CA_cert() && !self_signed_ee_cert)
          status.insert(Certificate_Status_Code::CA_CERT_NOT_FOR_CERT_ISSUER);
 
+      // TODO: put into Certificate_Extensions::Basic_Constraints::validate()
       if(issuer.path_limit() < i)
          status.insert(Certificate_Status_Code::CERT_CHAIN_TOO_LONG);
 
@@ -142,6 +144,7 @@ check_chain(const std::vector<X509_Certificate>& cert_path,
             status.insert(Certificate_Status_Code::UNTRUSTED_HASH);
          }
 
+      // TODO: put into Certificate_Extensions::Name_Constraints::validate()
       const NameConstraints& name_constr = issuer.name_constraints();
 
       if(!name_constr.permitted().empty() || !name_constr.excluded().empty())
@@ -197,6 +200,18 @@ check_chain(const std::vector<X509_Certificate>& cert_path,
                }
             }
          }
+
+         // Check cert extensions
+         Extensions extensions = subject.v3_extensions();
+         for (auto& extension : extensions.extensions())
+            {
+            if(!Extensions::is_known_extension(extension.first->oid_of()) && extension.second)
+               {
+               status.insert(Certificate_Status_Code::UNKNOWN_CRITICAL_EXTENSION);
+               continue;
+               }
+            extension.first->validate(cert_path[i], status, cert_path);
+            }
       }
 
    for(size_t i = 0; i != cert_path.size() - 1; ++i)
@@ -472,6 +487,8 @@ const char* Path_Validation_Result::status_string(Certificate_Status_Code code)
          return "Certificate does not match provided name";
       case Certificate_Status_Code::NAME_CONSTRAINT_ERROR:
          return "Certificate does not pass name constraint";
+      case Certificate_Status_Code::UNKNOWN_CRITICAL_EXTENSION:
+         return "Unknown critical extension encountered";
 
       case Certificate_Status_Code::CERT_IS_REVOKED:
          return "Certificate is revoked";