Repository: randombit/botan
Pull request #4032 (2024-04-25 21:27:19)

Fix a bug in SAN handling when creating certs and PKCS 10 requests
If the Subject Alternative Name extension was set in opts.extensions, then we would ignore any values set in other fields (eg opts.dns)

coveralls: 
[![Coverage Status](https://coveralls.io/builds/67144727/badge)](https://coveralls.io/builds/67144727)

coverage: 92.023% (-0.006%) from 92.029%
when pulling **5774c4d12ec8bd9579ad17f990bdf6c28daf087e on jack/pkcs10-ext**
into **e50f4f61b08115c4616136ec87df718b75284721 on master**.


Patch:
diff --git a/src/lib/x509/x509self.cpp b/src/lib/x509/x509self.cpp
index 605dca2a4..65ea6c320 100644
--- a/src/lib/x509/x509self.cpp
+++ b/src/lib/x509/x509self.cpp
@@ -21,25 +21,48 @@ namespace {
 /*
 * Load information from the X509_Cert_Options
 */
-void load_info(const X509_Cert_Options& opts, X509_DN& subject_dn, AlternativeName& subject_alt) {
+X509_DN load_dn_info(const X509_Cert_Options& opts) {
+   X509_DN subject_dn;
+
    subject_dn.add_attribute("X520.CommonName", opts.common_name);
    subject_dn.add_attribute("X520.Country", opts.country);
    subject_dn.add_attribute("X520.State", opts.state);
    subject_dn.add_attribute("X520.Locality", opts.locality);
    subject_dn.add_attribute("X520.Organization", opts.organization);
    subject_dn.add_attribute("X520.OrganizationalUnit", opts.org_unit);
+   subject_dn.add_attribute("X520.SerialNumber", opts.serial_number);
+
    for(const auto& extra_ou : opts.more_org_units) {
       subject_dn.add_attribute("X520.OrganizationalUnit", extra_ou);
    }
 
-   subject_dn.add_attribute("X520.SerialNumber", opts.serial_number);
-   subject_alt = AlternativeName(opts.email, opts.uri, opts.dns, opts.ip);
+   return subject_dn;
+}
+
+auto create_alt_name_ext(const X509_Cert_Options& opts, Extensions& extensions) {
+   AlternativeName subject_alt;
+
+   /*
+   If the extension was already created in opts.extension we need to
+   merge the values provied in opts with the values set in the extension.
+   */
+   if(auto ext = extensions.get_extension_object_as<Cert_Extension::Subject_Alternative_Name>()) {
+      subject_alt = ext->get_alt_name();
+   }
+
+   subject_alt.add_attribute("DNS", opts.dns);
+   subject_alt.add_attribute("URI", opts.uri);
+   subject_alt.add_attribute("RFC822", opts.email);
+   subject_alt.add_attribute("IP", opts.ip);
    subject_alt.add_othername(OID::from_string("PKIX.XMPPAddr"), opts.xmpp, ASN1_Type::Utf8String);
 
    for(const auto& dns : opts.more_dns) {
       subject_alt.add_attribute("DNS", dns);
    }
+
+   return std::make_unique<Cert_Extension::Subject_Alternative_Name>(subject_alt);
 }
+
 }  // namespace
 
 namespace X509 {
@@ -56,9 +79,7 @@ X509_Certificate create_self_signed_cert(const X509_Cert_Options& opts,
    const AlgorithmIdentifier sig_algo = signer->algorithm_identifier();
    BOTAN_ASSERT_NOMSG(sig_algo.oid().has_value());
 
-   X509_DN subject_dn;
-   AlternativeName subject_alt;
-   load_info(opts, subject_dn, subject_alt);
+   const auto subject_dn = load_dn_info(opts);
 
    Extensions extensions = opts.extensions;
 
@@ -79,7 +100,7 @@ X509_Certificate create_self_signed_cert(const X509_Cert_Options& opts,
    extensions.add_new(std::make_unique<Cert_Extension::Authority_Key_ID>(skid->get_key_id()));
    extensions.add_new(std::move(skid));
 
-   extensions.add_new(std::make_unique<Cert_Extension::Subject_Alternative_Name>(subject_alt));
+   extensions.replace(create_alt_name_ext(opts, extensions));
 
    extensions.add_new(std::make_unique<Cert_Extension::Extended_Key_Usage>(opts.ex_constraints));
 
@@ -93,9 +114,7 @@ PKCS10_Request create_cert_req(const X509_Cert_Options& opts,
                                const Private_Key& key,
                                std::string_view hash_fn,
                                RandomNumberGenerator& rng) {
-   X509_DN subject_dn;
-   AlternativeName subject_alt;
-   load_info(opts, subject_dn, subject_alt);
+   const auto subject_dn = load_dn_info(opts);
 
    const auto constraints = opts.is_CA ? Key_Constraints::ca_constraints() : opts.constraints;
 
@@ -111,8 +130,9 @@ PKCS10_Request create_cert_req(const X509_Cert_Options& opts,
       extensions.add_new(std::make_unique<Cert_Extension::Key_Usage>(constraints));
    }
 
-   extensions.add_new(std::make_unique<Cert_Extension::Extended_Key_Usage>(opts.ex_constraints));
-   extensions.add_new(std::make_unique<Cert_Extension::Subject_Alternative_Name>(subject_alt));
+   extensions.replace(create_alt_name_ext(opts, extensions));
+
+   create_alt_name_ext(opts, extensions);
 
    return PKCS10_Request::create(key, subject_dn, extensions, hash_fn, rng, opts.padding_scheme, opts.challenge);
 }


Test patch:
diff --git a/src/tests/unit_x509.cpp b/src/tests/unit_x509.cpp
index 0f166c296..95d9aec16 100644
--- a/src/tests/unit_x509.cpp
+++ b/src/tests/unit_x509.cpp
@@ -818,12 +818,14 @@ Test::Result test_pkcs10_ext(const Botan::Private_Key& key,
 
    Botan::X509_Cert_Options opts;
 
+   opts.dns = "main.example.org";
+   opts.more_dns.push_back("more1.example.org");
+   opts.more_dns.push_back("more2.example.org");
+
    opts.padding_scheme = sig_padding;
 
    Botan::AlternativeName alt_name;
-   alt_name.add_attribute("DNS", "example.org");
-   alt_name.add_attribute("DNS", "example.com");
-   alt_name.add_attribute("DNS", "example.net");
+   alt_name.add_attribute("DNS", "bonus.example.org");
 
    opts.extensions.add(std::make_unique<Botan::Cert_Extension::Subject_Alternative_Name>(alt_name));
 
@@ -831,14 +833,17 @@ Test::Result test_pkcs10_ext(const Botan::Private_Key& key,
 
    std::vector<std::string> alt_dns_names = req.subject_alt_name().get_attribute("DNS");
 
-   result.test_eq("Expected number of DNS names", alt_dns_names.size(), 3);
+   result.test_eq("Expected number of DNS names", alt_dns_names.size(), 4);
 
    // The order is not guaranteed so sort before comparing
    std::sort(alt_dns_names.begin(), alt_dns_names.end());
 
-   result.test_eq("Expected DNS name 1", alt_dns_names.at(0), "example.com");
-   result.test_eq("Expected DNS name 2", alt_dns_names.at(1), "example.net");
-   result.test_eq("Expected DNS name 3", alt_dns_names.at(2), "example.org");
+   if(alt_dns_names.size() == 4) {
+      result.test_eq("Expected DNS name 1", alt_dns_names.at(0), "bonus.example.org");
+      result.test_eq("Expected DNS name 2", alt_dns_names.at(1), "main.example.org");
+      result.test_eq("Expected DNS name 3", alt_dns_names.at(2), "more1.example.org");
+      result.test_eq("Expected DNS name 3", alt_dns_names.at(3), "more2.example.org");
+   }
 
    return result;
 }