Resolve an injection vulnerability in SAN creation

Author: alex

Diff for: openssl-sys/src/handwritten/x509.rs

@@ -550,6 +550,13 @@ extern "C" {
     pub fn X509_EXTENSION_get_object(ext: *mut X509_EXTENSION) -> *mut ASN1_OBJECT;
     pub fn X509_EXTENSION_get_data(ext: *mut X509_EXTENSION) -> *mut ASN1_OCTET_STRING;
 }
+
+const_ptr_api! {
+    extern "C" {
+        pub fn i2d_X509_EXTENSION(ext: #[const_ptr_if(ossl300)] X509_EXTENSION, pp: *mut *mut c_uchar) -> c_int;
+    }
+}
+
 const_ptr_api! {
     extern "C" {
         // in X509

Diff for: openssl-sys/src/handwritten/x509v3.rs

@@ -4,6 +4,7 @@ use libc::*;
 pub enum CONF_METHOD {}
 
 extern "C" {
+    pub fn GENERAL_NAME_new() -> *mut GENERAL_NAME;
     pub fn GENERAL_NAME_free(name: *mut GENERAL_NAME);
 }
 

Diff for: openssl/src/x509/extension.rs

@@ -20,7 +20,8 @@ use std::fmt::Write;
 
 use crate::error::ErrorStack;
 use crate::nid::Nid;
-use crate::x509::{X509Extension, X509v3Context};
+use crate::x509::{Asn1Object, GeneralName, Stack, X509Extension, X509v3Context};
+use foreign_types::ForeignType;
 
 /// An extension which indicates whether a certificate is a CA certificate.
 pub struct BasicConstraints {
@@ -463,11 +464,19 @@ impl AuthorityKeyIdentifier {
     }
 }
 
+enum RustGeneralName {
+    Dns(String),
+    Email(String),
+    Uri(String),
+    Ip(String),
+    Rid(String),
+}
+
 /// An extension that allows additional identities to be bound to the subject
 /// of the certificate.
 pub struct SubjectAlternativeName {
     critical: bool,
-    names: Vec<String>,
+    items: Vec<RustGeneralName>,
 }
 
 impl Default for SubjectAlternativeName {
@@ -481,7 +490,7 @@ impl SubjectAlternativeName {
     pub fn new() -> SubjectAlternativeName {
         SubjectAlternativeName {
             critical: false,
-            names: vec![],
+            items: vec![],
         }
     }
 
@@ -493,55 +502,73 @@ impl SubjectAlternativeName {
 
     /// Sets the `email` flag.
     pub fn email(&mut self, email: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("email:{}", email));
+        self.items.push(RustGeneralName::Email(email.to_string()));
         self
     }
 
     /// Sets the `uri` flag.
     pub fn uri(&mut self, uri: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("URI:{}", uri));
+        self.items.push(RustGeneralName::Uri(uri.to_string()));
         self
     }
 
     /// Sets the `dns` flag.
     pub fn dns(&mut self, dns: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("DNS:{}", dns));
+        self.items.push(RustGeneralName::Dns(dns.to_string()));
         self
     }
 
     /// Sets the `rid` flag.
     pub fn rid(&mut self, rid: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("RID:{}", rid));
+        self.items.push(RustGeneralName::Rid(rid.to_string()));
         self
     }
 
     /// Sets the `ip` flag.
     pub fn ip(&mut self, ip: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("IP:{}", ip));
+        self.items.push(RustGeneralName::Ip(ip.to_string()));
         self
     }
 
     /// Sets the `dirName` flag.
-    pub fn dir_name(&mut self, dir_name: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("dirName:{}", dir_name));
-        self
+    ///
+    /// Not currently actually supported, always panics.
+    #[deprecated = "dir_name is deprecated and always panics. Please file a bug if you have a use case for this."]
+    pub fn dir_name(&mut self, _dir_name: &str) -> &mut SubjectAlternativeName {
+        unimplemented!(
+            "This has not yet been adapted for the new internals. File a bug if you need this."
+        );
     }
 
     /// Sets the `otherName` flag.
-    pub fn other_name(&mut self, other_name: &str) -> &mut SubjectAlternativeName {
-        self.names.push(format!("otherName:{}", other_name));
-        self
+    ///
+    /// Not currently actually supported, always panics.
+    #[deprecated = "other_name is deprecated and always panics. Please file a bug if you have a use case for this."]
+    pub fn other_name(&mut self, _other_name: &str) -> &mut SubjectAlternativeName {
+        unimplemented!(
+            "This has not yet been adapted for the new internals. File a bug if you need this."
+        );
     }
 
     /// Return a `SubjectAlternativeName` extension as an `X509Extension`.
-    pub fn build(&self, ctx: &X509v3Context<'_>) -> Result<X509Extension, ErrorStack> {
-        let mut value = String::new();
-        let mut first = true;
-        append(&mut value, &mut first, self.critical, "critical");
-        for name in &self.names {
-            append(&mut value, &mut first, true, name);
+    pub fn build(&self, _ctx: &X509v3Context<'_>) -> Result<X509Extension, ErrorStack> {
+        let mut stack = Stack::new()?;
+        for item in &self.items {
+            let gn = match item {
+                RustGeneralName::Dns(s) => GeneralName::new_dns(s.as_bytes())?,
+                RustGeneralName::Email(s) => GeneralName::new_email(s.as_bytes())?,
+                RustGeneralName::Uri(s) => GeneralName::new_uri(s.as_bytes())?,
+                RustGeneralName::Ip(s) => {
+                    GeneralName::new_ip(s.parse().map_err(|_| ErrorStack::get())?)?
+                }
+                RustGeneralName::Rid(s) => GeneralName::new_rid(Asn1Object::from_str(s)?)?,
+            };
+            stack.push(gn)?;
+        }
+
+        unsafe {
+            X509Extension::new_internal(Nid::SUBJECT_ALT_NAME, self.critical, stack.as_ptr().cast())
         }
-        X509Extension::new_nid(None, Some(ctx), Nid::SUBJECT_ALT_NAME, &value)
     }
 }
 

Diff for: openssl/src/x509/mod.rs

@@ -9,9 +9,9 @@
 
 use cfg_if::cfg_if;
 use foreign_types::{ForeignType, ForeignTypeRef, Opaque};
-use libc::{c_int, c_long, c_uint};
+use libc::{c_int, c_long, c_uint, c_void};
 use std::cmp::{self, Ordering};
-use std::convert::TryFrom;
+use std::convert::{TryFrom, TryInto};
 use std::error::Error;
 use std::ffi::{CStr, CString};
 use std::fmt;
@@ -24,7 +24,8 @@ use std::slice;
 use std::str;
 
 use crate::asn1::{
-    Asn1BitStringRef, Asn1IntegerRef, Asn1ObjectRef, Asn1StringRef, Asn1TimeRef, Asn1Type,
+    Asn1BitStringRef, Asn1IntegerRef, Asn1Object, Asn1ObjectRef, Asn1StringRef, Asn1TimeRef,
+    Asn1Type,
 };
 use crate::bio::MemBioSlice;
 use crate::conf::ConfRef;
@@ -851,6 +852,15 @@ impl X509Extension {
         }
     }
 
+    pub(crate) unsafe fn new_internal(
+        nid: Nid,
+        critical: bool,
+        value: *mut c_void,
+    ) -> Result<X509Extension, ErrorStack> {
+        ffi::init();
+        cvt_p(ffi::X509V3_EXT_i2d(nid.as_raw(), critical as _, value)).map(X509Extension)
+    }
+
     /// Adds an alias for an extension
     ///
     /// # Safety
@@ -863,6 +873,15 @@ impl X509Extension {
     }
 }
 
+impl X509ExtensionRef {
+    to_der! {
+        /// Serializes the Extension to its standard DER encoding.
+        #[corresponds(i2d_X509_EXTENSION)]
+        to_der,
+        ffi::i2d_X509_EXTENSION
+    }
+}
+
 /// A builder used to construct an `X509Name`.
 pub struct X509NameBuilder(X509Name);
 
@@ -1715,6 +1734,75 @@ foreign_type_and_impl_send_sync! {
     pub struct GeneralNameRef;
 }
 
+impl GeneralName {
+    unsafe fn new(
+        type_: c_int,
+        asn1_type: Asn1Type,
+        value: &[u8],
+    ) -> Result<GeneralName, ErrorStack> {
+        ffi::init();
+        let gn = GeneralName::from_ptr(cvt_p(ffi::GENERAL_NAME_new())?);
+        (*gn.as_ptr()).type_ = type_;
+        let s = cvt_p(ffi::ASN1_STRING_type_new(asn1_type.as_raw()))?;
+        ffi::ASN1_STRING_set(s, value.as_ptr().cast(), value.len().try_into().unwrap());
+
+        #[cfg(boringssl)]
+        {
+            (*gn.as_ptr()).d.ptr = s.cast();
+        }
+        #[cfg(not(boringssl))]
+        {
+            (*gn.as_ptr()).d = s.cast();
+        }
+
+        Ok(gn)
+    }
+
+    pub(crate) fn new_email(email: &[u8]) -> Result<GeneralName, ErrorStack> {
+        unsafe { GeneralName::new(ffi::GEN_EMAIL, Asn1Type::IA5STRING, email) }
+    }
+
+    pub(crate) fn new_dns(dns: &[u8]) -> Result<GeneralName, ErrorStack> {
+        unsafe { GeneralName::new(ffi::GEN_DNS, Asn1Type::IA5STRING, dns) }
+    }
+
+    pub(crate) fn new_uri(uri: &[u8]) -> Result<GeneralName, ErrorStack> {
+        unsafe { GeneralName::new(ffi::GEN_URI, Asn1Type::IA5STRING, uri) }
+    }
+
+    pub(crate) fn new_ip(ip: IpAddr) -> Result<GeneralName, ErrorStack> {
+        match ip {
+            IpAddr::V4(addr) => unsafe {
+                GeneralName::new(ffi::GEN_IPADD, Asn1Type::OCTET_STRING, &addr.octets())
+            },
+            IpAddr::V6(addr) => unsafe {
+                GeneralName::new(ffi::GEN_IPADD, Asn1Type::OCTET_STRING, &addr.octets())
+            },
+        }
+    }
+
+    pub(crate) fn new_rid(oid: Asn1Object) -> Result<GeneralName, ErrorStack> {
+        unsafe {
+            ffi::init();
+            let gn = cvt_p(ffi::GENERAL_NAME_new())?;
+            (*gn).type_ = ffi::GEN_RID;
+
+            #[cfg(boringssl)]
+            {
+                (*gn).d.registeredID = oid.as_ptr();
+            }
+            #[cfg(not(boringssl))]
+            {
+                (*gn).d = oid.as_ptr().cast();
+            }
+
+            mem::forget(oid);
+
+            Ok(GeneralName::from_ptr(gn))
+        }
+    }
+}
+
 impl GeneralNameRef {
     fn ia5_string(&self, ffi_type: c_int) -> Option<&str> {
         unsafe {

Diff for: openssl/src/x509/tests.rs

@@ -287,6 +287,44 @@ fn x509_builder() {
     assert_eq!(serial, x509.serial_number().to_bn().unwrap());
 }
 
+#[test]
+fn x509_extension_to_der() {
+    let builder = X509::builder().unwrap();
+
+    for (ext, expected) in [
+        (
+            BasicConstraints::new().critical().ca().build().unwrap(),
+            b"0\x0f\x06\x03U\x1d\x13\x01\x01\xff\x04\x050\x03\x01\x01\xff" as &[u8],
+        ),
+        (
+            SubjectAlternativeName::new()
+                .dns("example.com,DNS:example2.com")
+                .build(&builder.x509v3_context(None, None))
+                .unwrap(),
+            b"0'\x06\x03U\x1d\x11\x04 0\x1e\x82\x1cexample.com,DNS:example2.com",
+        ),
+        (
+            SubjectAlternativeName::new()
+                .rid("1.2.3.4")
+                .uri("https://example.com")
+                .build(&builder.x509v3_context(None, None))
+                .unwrap(),
+            b"0#\x06\x03U\x1d\x11\x04\x1c0\x1a\x88\x03*\x03\x04\x86\x13https://example.com",
+        ),
+        (
+            ExtendedKeyUsage::new()
+                .server_auth()
+                .other("2.999.1")
+                .other("clientAuth")
+                .build()
+                .unwrap(),
+            b"0\x22\x06\x03U\x1d%\x04\x1b0\x19\x06\x08+\x06\x01\x05\x05\x07\x03\x01\x06\x03\x887\x01\x06\x08+\x06\x01\x05\x05\x07\x03\x02",
+        ),
+    ] {
+        assert_eq!(&ext.to_der().unwrap(), expected);
+    }
+}
+
 #[test]
 fn x509_req_builder() {
     let pkey = pkey();