From f8e225e6a404b260ea617a400cb848ab47fc0564 Mon Sep 17 00:00:00 2001 From: Joshua Nitschke Date: Thu, 12 Nov 2020 09:26:23 -0800 Subject: [PATCH 1/7] Add additional function so that x509 name with specific type can be added Originally added in https://github.com/sfackler/rust-openssl/pull/1371 --- boring/src/asn1.rs | 73 ++++++++++++++++++++++++++++++++++++++++++ boring/src/x509/mod.rs | 57 ++++++++++++++++++++++++++++++++- 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/boring/src/asn1.rs b/boring/src/asn1.rs index e9b3d43d..6eb2199f 100644 --- a/boring/src/asn1.rs +++ b/boring/src/asn1.rs @@ -78,6 +78,79 @@ impl fmt::Display for Asn1GeneralizedTimeRef { } } +/// The type of an ASN.1 value. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct Asn1Type(c_int); + +#[allow(missing_docs)] // no need to document the constants +impl Asn1Type { + pub const EOC: Asn1Type = Asn1Type(ffi::V_ASN1_EOC); + + pub const BOOLEAN: Asn1Type = Asn1Type(ffi::V_ASN1_BOOLEAN); + + pub const INTEGER: Asn1Type = Asn1Type(ffi::V_ASN1_INTEGER); + + pub const BIT_STRING: Asn1Type = Asn1Type(ffi::V_ASN1_BIT_STRING); + + pub const OCTET_STRING: Asn1Type = Asn1Type(ffi::V_ASN1_OCTET_STRING); + + pub const NULL: Asn1Type = Asn1Type(ffi::V_ASN1_NULL); + + pub const OBJECT: Asn1Type = Asn1Type(ffi::V_ASN1_OBJECT); + + pub const OBJECT_DESCRIPTOR: Asn1Type = Asn1Type(ffi::V_ASN1_OBJECT_DESCRIPTOR); + + pub const EXTERNAL: Asn1Type = Asn1Type(ffi::V_ASN1_EXTERNAL); + + pub const REAL: Asn1Type = Asn1Type(ffi::V_ASN1_REAL); + + pub const ENUMERATED: Asn1Type = Asn1Type(ffi::V_ASN1_ENUMERATED); + + pub const UTF8STRING: Asn1Type = Asn1Type(ffi::V_ASN1_UTF8STRING); + + pub const SEQUENCE: Asn1Type = Asn1Type(ffi::V_ASN1_SEQUENCE); + + pub const SET: Asn1Type = Asn1Type(ffi::V_ASN1_SET); + + pub const NUMERICSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_NUMERICSTRING); + + pub const PRINTABLESTRING: Asn1Type = Asn1Type(ffi::V_ASN1_PRINTABLESTRING); + + pub const T61STRING: Asn1Type = Asn1Type(ffi::V_ASN1_T61STRING); + + pub const TELETEXSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_TELETEXSTRING); + + pub const VIDEOTEXSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_VIDEOTEXSTRING); + + pub const IA5STRING: Asn1Type = Asn1Type(ffi::V_ASN1_IA5STRING); + + pub const UTCTIME: Asn1Type = Asn1Type(ffi::V_ASN1_UTCTIME); + + pub const GENERALIZEDTIME: Asn1Type = Asn1Type(ffi::V_ASN1_GENERALIZEDTIME); + + pub const GRAPHICSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_GRAPHICSTRING); + + pub const ISO64STRING: Asn1Type = Asn1Type(ffi::V_ASN1_ISO64STRING); + + pub const VISIBLESTRING: Asn1Type = Asn1Type(ffi::V_ASN1_VISIBLESTRING); + + pub const GENERALSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_GENERALSTRING); + + pub const UNIVERSALSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_UNIVERSALSTRING); + + pub const BMPSTRING: Asn1Type = Asn1Type(ffi::V_ASN1_BMPSTRING); + + /// Constructs an `Asn1Type` from a raw OpenSSL value. + pub fn from_raw(value: c_int) -> Self { + Asn1Type(value) + } + + /// Returns the raw OpenSSL value represented by this type. + pub fn as_raw(&self) -> c_int { + self.0 + } +} + /// Difference between two ASN1 times. /// /// This `struct` is created by the [`diff`] method on [`Asn1TimeRef`]. See its diff --git a/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index 08f10208..8b4a97bf 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -21,7 +21,9 @@ use std::ptr; use std::slice; use std::str; -use crate::asn1::{Asn1BitStringRef, Asn1IntegerRef, Asn1ObjectRef, Asn1StringRef, Asn1TimeRef}; +use crate::asn1::{ + Asn1BitStringRef, Asn1IntegerRef, Asn1ObjectRef, Asn1StringRef, Asn1TimeRef, Asn1Type, +}; use crate::bio::MemBioSlice; use crate::conf::ConfRef; use crate::error::ErrorStack; @@ -826,6 +828,33 @@ impl X509NameBuilder { } } + /// Add a field entry by str with a specific type. + /// + /// This corresponds to [`X509_NAME_add_entry_by_txt`]. + /// + /// [`X509_NAME_add_entry_by_txt`]: https://www.openssl.org/docs/man1.1.0/crypto/X509_NAME_add_entry_by_txt.html + pub fn append_entry_by_text_with_type( + &mut self, + field: &str, + value: &str, + ty: Asn1Type, + ) -> Result<(), ErrorStack> { + unsafe { + let field = CString::new(field).unwrap(); + assert!(value.len() <= c_int::max_value() as usize); + cvt(ffi::X509_NAME_add_entry_by_txt( + self.0.as_ptr(), + field.as_ptr() as *mut _, + ty.as_raw(), + value.as_ptr(), + value.len() as c_int, + -1, + 0, + )) + .map(|_| ()) + } + } + /// Add a field entry by NID. /// /// This corresponds to [`X509_NAME_add_entry_by_NID`]. @@ -847,6 +876,32 @@ impl X509NameBuilder { } } + /// Add a field entry by NID with a specific type. + /// + /// This corresponds to [`X509_NAME_add_entry_by_NID`]. + /// + /// [`X509_NAME_add_entry_by_NID`]: https://www.openssl.org/docs/man1.1.0/crypto/X509_NAME_add_entry_by_NID.html + pub fn append_entry_by_nid_with_type( + &mut self, + field: Nid, + value: &str, + ty: Asn1Type, + ) -> Result<(), ErrorStack> { + unsafe { + assert!(value.len() <= c_int::max_value() as usize); + cvt(ffi::X509_NAME_add_entry_by_NID( + self.0.as_ptr(), + field.as_raw(), + ty.as_raw(), + value.as_ptr() as *mut _, + value.len() as c_int, + -1, + 0, + )) + .map(|_| ()) + } + } + /// Return an `X509Name`. pub fn build(self) -> X509Name { self.0 From ae0cd6b98e4b2eab5ab337cebc7df69326e7811f Mon Sep 17 00:00:00 2001 From: Rob Shearman Date: Mon, 4 Oct 2021 13:01:11 +0100 Subject: [PATCH 2/7] Add X509Name to/from DER methods Since X509Name is more complex than a single value (it's a a sequence of entries) it's useful to be able to serialise/deserialise to/from flat data, and DER is a natural form for this. So add a {i2d,d2i}_X509_NAME -sys functions, and to_der/from_der wrappers in X509NameRef and X509Name respectively. Originally added in https://github.com/sfackler/rust-openssl/pull/1534 --- boring/src/x509/mod.rs | 22 ++++++++++++++++++++++ boring/src/x509/tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index 8b4a97bf..3d5bef8c 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -929,6 +929,18 @@ impl X509Name { let file = CString::new(file.as_ref().as_os_str().to_str().unwrap()).unwrap(); unsafe { cvt_p(ffi::SSL_load_client_CA_file(file.as_ptr())).map(|p| Stack::from_ptr(p)) } } + + from_der! { + /// Deserializes a DER-encoded X509 name structure. + /// + /// This corresponds to [`d2i_X509_NAME`]. + /// + /// [`d2i_X509_NAME`]: https://www.openssl.org/docs/manmaster/man3/d2i_X509_NAME.html + from_der, + X509Name, + ffi::d2i_X509_NAME, + ::libc::c_long + } } impl Stackable for X509Name { @@ -953,6 +965,16 @@ impl X509NameRef { loc: -1, } } + + to_der! { + /// Serializes the certificate into a DER-encoded X509 name structure. + /// + /// This corresponds to [`i2d_X509_NAME`]. + /// + /// [`i2d_X509_NAME`]: https://www.openssl.org/docs/man1.1.0/crypto/i2d_X509_NAME.html + to_der, + ffi::i2d_X509_NAME + } } impl fmt::Debug for X509NameRef { diff --git a/boring/src/x509/tests.rs b/boring/src/x509/tests.rs index 3d942aaa..a4f2ba79 100644 --- a/boring/src/x509/tests.rs +++ b/boring/src/x509/tests.rs @@ -379,3 +379,26 @@ fn test_verify_fails() { .init(&store, &cert, &chain, |c| c.verify_cert()) .unwrap()); } + +#[test] +fn test_save_subject_der() { + let cert = include_bytes!("../../test/cert.pem"); + let cert = X509::from_pem(cert).unwrap(); + + let der = cert.subject_name().to_der().unwrap(); + println!("der: {:?}", der); + assert!(!der.is_empty()); +} + +#[test] +fn test_load_subject_der() { + // The subject from ../../test/cert.pem + const SUBJECT_DER: &[u8] = &[ + 48, 90, 49, 11, 48, 9, 6, 3, 85, 4, 6, 19, 2, 65, 85, 49, 19, 48, 17, 6, 3, 85, 4, 8, 12, + 10, 83, 111, 109, 101, 45, 83, 116, 97, 116, 101, 49, 33, 48, 31, 6, 3, 85, 4, 10, 12, 24, + 73, 110, 116, 101, 114, 110, 101, 116, 32, 87, 105, 100, 103, 105, 116, 115, 32, 80, 116, + 121, 32, 76, 116, 100, 49, 19, 48, 17, 6, 3, 85, 4, 3, 12, 10, 102, 111, 111, 98, 97, 114, + 46, 99, 111, 109, + ]; + X509Name::from_der(SUBJECT_DER).unwrap(); +} From 1eea7c527162d3ad6baf3599579e2b00c9aa48a2 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Tue, 21 Mar 2023 20:20:26 -0400 Subject: [PATCH 3/7] Resolve an injection vulnerability in SAN creation --- boring/src/x509/extension.rs | 69 +++++++++++++++++++++---------- boring/src/x509/mod.rs | 78 +++++++++++++++++++++++++++++++++++- boring/src/x509/tests.rs | 38 ++++++++++++++++++ 3 files changed, 162 insertions(+), 23 deletions(-) diff --git a/boring/src/x509/extension.rs b/boring/src/x509/extension.rs index 8bc33a44..0f1a4abb 100644 --- a/boring/src/x509/extension.rs +++ b/boring/src/x509/extension.rs @@ -19,7 +19,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 { @@ -456,11 +457,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 { @@ -474,7 +483,7 @@ impl SubjectAlternativeName { pub fn new() -> SubjectAlternativeName { SubjectAlternativeName { critical: false, - names: vec![], + items: vec![], } } @@ -486,55 +495,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/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index 3d5bef8c..aedbf803 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -9,20 +9,22 @@ use crate::ffi; use foreign_types::{ForeignType, ForeignTypeRef}; -use libc::{c_int, c_long}; +use libc::{c_int, c_long, c_void}; use std::convert::TryInto; use std::error::Error; use std::ffi::{CStr, CString}; use std::fmt; use std::marker::PhantomData; use std::mem; +use std::net::IpAddr; use std::path::Path; use std::ptr; 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; @@ -792,6 +794,24 @@ impl X509Extension { .map(|p| X509Extension::from_ptr(p)) } } + + 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(|p| X509Extension::from_ptr(p)) + } +} + +impl X509ExtensionRef { + to_der! { + /// Serializes the Extension to its standard DER encoding. + to_der, + ffi::i2d_X509_EXTENSION + } } /// A builder used to construct an `X509Name`. @@ -1369,6 +1389,60 @@ foreign_type_and_impl_send_sync! { pub struct GeneralName; } +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()); + + (*gn.as_ptr()).d.ptr = 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; + (*gn).d.registeredID = oid.as_ptr(); + + mem::forget(oid); + + Ok(GeneralName::from_ptr(gn)) + } + } +} + impl GeneralNameRef { fn ia5_string(&self, ffi_type: c_int) -> Option<&str> { unsafe { diff --git a/boring/src/x509/tests.rs b/boring/src/x509/tests.rs index a4f2ba79..bda7508b 100644 --- a/boring/src/x509/tests.rs +++ b/boring/src/x509/tests.rs @@ -250,6 +250,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(); From 0f2800102724f4788d0a2612487987c96fefaabb Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Tue, 21 Mar 2023 20:31:02 -0400 Subject: [PATCH 4/7] Resolve an injection vulnerability in EKU creation --- boring/src/asn1.rs | 5 ++ boring/src/x509/extension.rs | 89 +++++++++--------------------------- boring/src/x509/tests.rs | 8 ++++ 3 files changed, 34 insertions(+), 68 deletions(-) diff --git a/boring/src/asn1.rs b/boring/src/asn1.rs index 6eb2199f..02e601fc 100644 --- a/boring/src/asn1.rs +++ b/boring/src/asn1.rs @@ -38,6 +38,7 @@ use crate::bio::MemBio; use crate::bn::{BigNum, BigNumRef}; use crate::error::ErrorStack; use crate::nid::Nid; +use crate::stack::Stackable; use crate::string::OpensslString; use crate::{cvt, cvt_p}; @@ -555,6 +556,10 @@ foreign_type_and_impl_send_sync! { pub struct Asn1Object; } +impl Stackable for Asn1Object { + type StackType = ffi::stack_st_ASN1_OBJECT; +} + impl Asn1Object { /// Constructs an ASN.1 Object Identifier from a string representation of /// the OID. diff --git a/boring/src/x509/extension.rs b/boring/src/x509/extension.rs index 0f1a4abb..639d28bd 100644 --- a/boring/src/x509/extension.rs +++ b/boring/src/x509/extension.rs @@ -17,9 +17,10 @@ //! ``` use std::fmt::Write; +use crate::asn1::Asn1Object; use crate::error::ErrorStack; use crate::nid::Nid; -use crate::x509::{Asn1Object, GeneralName, Stack, X509Extension, X509v3Context}; +use crate::x509::{GeneralName, Stack, X509Extension, X509v3Context}; use foreign_types::ForeignType; /// An extension which indicates whether a certificate is a CA certificate. @@ -222,18 +223,7 @@ impl KeyUsage { /// for which the certificate public key can be used for. pub struct ExtendedKeyUsage { critical: bool, - server_auth: bool, - client_auth: bool, - code_signing: bool, - email_protection: bool, - time_stamping: bool, - ms_code_ind: bool, - ms_code_com: bool, - ms_ctl_sign: bool, - ms_sgc: bool, - ms_efs: bool, - ns_sgc: bool, - other: Vec, + items: Vec, } impl Default for ExtendedKeyUsage { @@ -247,18 +237,7 @@ impl ExtendedKeyUsage { pub fn new() -> ExtendedKeyUsage { ExtendedKeyUsage { critical: false, - server_auth: false, - client_auth: false, - code_signing: false, - email_protection: false, - time_stamping: false, - ms_code_ind: false, - ms_code_com: false, - ms_ctl_sign: false, - ms_sgc: false, - ms_efs: false, - ns_sgc: false, - other: vec![], + items: vec![], } } @@ -270,95 +249,69 @@ impl ExtendedKeyUsage { /// Sets the `serverAuth` flag to `true`. pub fn server_auth(&mut self) -> &mut ExtendedKeyUsage { - self.server_auth = true; - self + self.other("serverAuth") } /// Sets the `clientAuth` flag to `true`. pub fn client_auth(&mut self) -> &mut ExtendedKeyUsage { - self.client_auth = true; - self + self.other("clientAuth") } /// Sets the `codeSigning` flag to `true`. pub fn code_signing(&mut self) -> &mut ExtendedKeyUsage { - self.code_signing = true; - self + self.other("codeSigning") } /// Sets the `timeStamping` flag to `true`. pub fn time_stamping(&mut self) -> &mut ExtendedKeyUsage { - self.time_stamping = true; - self + self.other("timeStamping") } /// Sets the `msCodeInd` flag to `true`. pub fn ms_code_ind(&mut self) -> &mut ExtendedKeyUsage { - self.ms_code_ind = true; - self + self.other("msCodeInd") } /// Sets the `msCodeCom` flag to `true`. pub fn ms_code_com(&mut self) -> &mut ExtendedKeyUsage { - self.ms_code_com = true; - self + self.other("msCodeCom") } /// Sets the `msCTLSign` flag to `true`. pub fn ms_ctl_sign(&mut self) -> &mut ExtendedKeyUsage { - self.ms_ctl_sign = true; - self + self.other("msCTLSign") } /// Sets the `msSGC` flag to `true`. pub fn ms_sgc(&mut self) -> &mut ExtendedKeyUsage { - self.ms_sgc = true; - self + self.other("msSGC") } /// Sets the `msEFS` flag to `true`. pub fn ms_efs(&mut self) -> &mut ExtendedKeyUsage { - self.ms_efs = true; - self + self.other("msEFS") } /// Sets the `nsSGC` flag to `true`. pub fn ns_sgc(&mut self) -> &mut ExtendedKeyUsage { - self.ns_sgc = true; - self + self.other("nsSGC") } /// Sets a flag not already defined. pub fn other(&mut self, other: &str) -> &mut ExtendedKeyUsage { - self.other.push(other.to_owned()); + self.items.push(other.to_string()); self } /// Return the `ExtendedKeyUsage` extension as an `X509Extension`. pub fn build(&self) -> Result { - let mut value = String::new(); - let mut first = true; - append(&mut value, &mut first, self.critical, "critical"); - append(&mut value, &mut first, self.server_auth, "serverAuth"); - append(&mut value, &mut first, self.client_auth, "clientAuth"); - append(&mut value, &mut first, self.code_signing, "codeSigning"); - append( - &mut value, - &mut first, - self.email_protection, - "emailProtection", - ); - append(&mut value, &mut first, self.time_stamping, "timeStamping"); - append(&mut value, &mut first, self.ms_code_ind, "msCodeInd"); - append(&mut value, &mut first, self.ms_code_com, "msCodeCom"); - append(&mut value, &mut first, self.ms_ctl_sign, "msCTLSign"); - append(&mut value, &mut first, self.ms_sgc, "msSGC"); - append(&mut value, &mut first, self.ms_efs, "msEFS"); - append(&mut value, &mut first, self.ns_sgc, "nsSGC"); - for other in &self.other { - append(&mut value, &mut first, true, other); + let mut stack = Stack::new()?; + for item in &self.items { + stack.push(Asn1Object::from_str(item)?)?; + } + unsafe { + X509Extension::new_internal(Nid::EXT_KEY_USAGE, self.critical, stack.as_ptr().cast()) } - X509Extension::new_nid(None, None, Nid::EXT_KEY_USAGE, &value) } } diff --git a/boring/src/x509/tests.rs b/boring/src/x509/tests.rs index bda7508b..88ad78f4 100644 --- a/boring/src/x509/tests.rs +++ b/boring/src/x509/tests.rs @@ -288,6 +288,14 @@ fn x509_extension_to_der() { } } +#[test] +fn eku_invalid_other() { + assert!(ExtendedKeyUsage::new() + .other("1.1.1.1.1,2.2.2.2.2") + .build() + .is_err()); +} + #[test] fn x509_req_builder() { let pkey = pkey(); From c80e3a3ec56d5543b905bb3d3f4b4ead52d0e71a Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Tue, 21 Mar 2023 20:44:15 -0400 Subject: [PATCH 5/7] Always provide an X509V3Context in X509Extension::new because OpenSSL requires it for some extensions (and segfaults without) --- boring/src/x509/mod.rs | 40 ++++++++++++++++++++++++++++++++++++---- boring/src/x509/tests.rs | 10 +++++++++- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index aedbf803..b48655c7 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -757,14 +757,30 @@ impl X509Extension { ) -> Result { let name = CString::new(name).unwrap(); let value = CString::new(value).unwrap(); + let mut ctx; unsafe { ffi::init(); let conf = conf.map_or(ptr::null_mut(), ConfRef::as_ptr); - let context = context.map_or(ptr::null_mut(), X509v3Context::as_ptr); + let context_ptr = match context { + Some(c) => c.as_ptr(), + None => { + ctx = mem::zeroed(); + + ffi::X509V3_set_ctx( + &mut ctx, + ptr::null_mut(), + ptr::null_mut(), + ptr::null_mut(), + ptr::null_mut(), + 0, + ); + &mut ctx + } + }; let name = name.as_ptr() as *mut _; let value = value.as_ptr() as *mut _; - cvt_p(ffi::X509V3_EXT_nconf(conf, context, name, value)) + cvt_p(ffi::X509V3_EXT_nconf(conf, context_ptr, name, value)) .map(|p| X509Extension::from_ptr(p)) } } @@ -783,14 +799,30 @@ impl X509Extension { value: &str, ) -> Result { let value = CString::new(value).unwrap(); + let mut ctx; unsafe { ffi::init(); let conf = conf.map_or(ptr::null_mut(), ConfRef::as_ptr); - let context = context.map_or(ptr::null_mut(), X509v3Context::as_ptr); + let context_ptr = match context { + Some(c) => c.as_ptr(), + None => { + ctx = mem::zeroed(); + + ffi::X509V3_set_ctx( + &mut ctx, + ptr::null_mut(), + ptr::null_mut(), + ptr::null_mut(), + ptr::null_mut(), + 0, + ); + &mut ctx + } + }; let name = name.as_raw(); let value = value.as_ptr() as *mut _; - cvt_p(ffi::X509V3_EXT_nconf_nid(conf, context, name, value)) + cvt_p(ffi::X509V3_EXT_nconf_nid(conf, context_ptr, name, value)) .map(|p| X509Extension::from_ptr(p)) } } diff --git a/boring/src/x509/tests.rs b/boring/src/x509/tests.rs index 88ad78f4..966e7b12 100644 --- a/boring/src/x509/tests.rs +++ b/boring/src/x509/tests.rs @@ -12,7 +12,7 @@ use crate::x509::extension::{ SubjectKeyIdentifier, }; use crate::x509::store::X509StoreBuilder; -use crate::x509::{X509Name, X509Req, X509StoreContext, X509VerifyResult, X509}; +use crate::x509::{X509Extension, X509Name, X509Req, X509StoreContext, X509VerifyResult, X509}; fn pkey() -> PKey { let rsa = Rsa::generate(2048).unwrap(); @@ -250,6 +250,14 @@ fn x509_builder() { assert_eq!(serial, x509.serial_number().to_bn().unwrap()); } +#[test] +fn x509_extension_new() { + assert!(X509Extension::new(None, None, "crlDistributionPoints", "section").is_err()); + assert!(X509Extension::new(None, None, "proxyCertInfo", "").is_err()); + assert!(X509Extension::new(None, None, "certificatePolicies", "").is_err()); + assert!(X509Extension::new(None, None, "subjectAltName", "dirName:section").is_err()); +} + #[test] fn x509_extension_to_der() { let builder = X509::builder().unwrap(); From 90dfe2f91261f7181e093e3a659cdea41d5c5cfe Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Tue, 21 Mar 2023 20:45:35 -0400 Subject: [PATCH 6/7] Document the horror show --- boring/src/x509/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index b48655c7..e58c9efd 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -748,6 +748,9 @@ impl X509Extension { /// Some extension types, such as `subjectAlternativeName`, require an `X509v3Context` to be /// provided. /// + /// DO NOT CALL THIS WITH UNTRUSTED `value`: `value` is an OpenSSL + /// mini-language that can read arbitrary files. + /// /// See the extension module for builder types which will construct certain common extensions. pub fn new( conf: Option<&ConfRef>, @@ -791,6 +794,9 @@ impl X509Extension { /// Some extension types, such as `nid::SUBJECT_ALTERNATIVE_NAME`, require an `X509v3Context` to /// be provided. /// + /// DO NOT CALL THIS WITH UNTRUSTED `value`: `value` is an OpenSSL + /// mini-language that can read arbitrary files. + /// /// See the extension module for builder types which will construct certain common extensions. pub fn new_nid( conf: Option<&ConfRef>, From b36b1705b3148348a0361e5681d7377d5c45bd68 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Tue, 21 Mar 2023 20:49:48 -0400 Subject: [PATCH 7/7] Fix race condition with X509Name creation --- boring/src/x509/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/boring/src/x509/mod.rs b/boring/src/x509/mod.rs index e58c9efd..4f45d132 100644 --- a/boring/src/x509/mod.rs +++ b/boring/src/x509/mod.rs @@ -962,7 +962,10 @@ impl X509NameBuilder { /// Return an `X509Name`. pub fn build(self) -> X509Name { - self.0 + // Round-trip through bytes because OpenSSL is not const correct and + // names in a "modified" state compute various things lazily. This can + // lead to data-races because OpenSSL doesn't have locks or anything. + X509Name::from_der(&self.0.to_der().unwrap()).unwrap() } }