From 482575bff434f58b80ffea34a9610d0ff265ac1f Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Tue, 21 Mar 2023 20:20:26 -0400 Subject: [PATCH] Resolve an injection vulnerability in SAN creation --- openssl-sys/src/handwritten/x509.rs | 7 ++ openssl-sys/src/handwritten/x509v3.rs | 1 + openssl/src/x509/extension.rs | 69 ++++++++++++++------ openssl/src/x509/mod.rs | 94 ++++++++++++++++++++++++++- openssl/src/x509/tests.rs | 38 +++++++++++ 5 files changed, 185 insertions(+), 24 deletions(-) diff --git a/openssl-sys/src/handwritten/x509.rs b/openssl-sys/src/handwritten/x509.rs index 8762e5f98d..abda4110cf 100644 --- a/openssl-sys/src/handwritten/x509.rs +++ b/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 --git a/openssl-sys/src/handwritten/x509v3.rs b/openssl-sys/src/handwritten/x509v3.rs index d0923e32b2..4f661ca5ec 100644 --- a/openssl-sys/src/handwritten/x509v3.rs +++ b/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 --git a/openssl/src/x509/extension.rs b/openssl/src/x509/extension.rs index ebbea1c885..21d8faac35 100644 --- a/openssl/src/x509/extension.rs +++ b/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, + items: Vec, } 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 { - 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 { + 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 --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 4f08bbc667..3d8f236fd5 100644 --- a/openssl/src/x509/mod.rs +++ b/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 { + 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 { + 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 { + unsafe { GeneralName::new(ffi::GEN_EMAIL, Asn1Type::IA5STRING, email) } + } + + pub(crate) fn new_dns(dns: &[u8]) -> Result { + unsafe { GeneralName::new(ffi::GEN_DNS, Asn1Type::IA5STRING, dns) } + } + + pub(crate) fn new_uri(uri: &[u8]) -> Result { + unsafe { GeneralName::new(ffi::GEN_URI, Asn1Type::IA5STRING, uri) } + } + + pub(crate) fn new_ip(ip: IpAddr) -> Result { + 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 { + 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 --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 5c563a2192..41a9bc4d61 100644 --- a/openssl/src/x509/tests.rs +++ b/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();