New X509D2iError to improve error checking for X509_ext_get_d2i()

openssl/src/error.rs

@@ -23,6 +23,7 @@ use std::convert::TryInto;
 use std::error;
 use std::ffi::CStr;
 use std::fmt;
+use std::fmt::Formatter;
 use std::io;
 use std::ptr;
 use std::str;
@@ -399,6 +400,49 @@ cfg_if! {
     }
 }
 
+pub enum X509D2iError {
+    InternalOpenSSLError(ErrorStack),
+    ExtensionNotFoundError,
+    ExtensionAmbiguousError,
+}
+
+impl X509D2iError {
+    pub fn internal_openssl_error(error: ErrorStack) -> Self {
+        Self::InternalOpenSSLError(error)
+    }
+
+    pub fn extension_not_found_error() -> Self {
+        Self::ExtensionNotFoundError
+    }
+
+    pub fn extension_ambiguous_error() -> Self {
+        Self::ExtensionAmbiguousError
+    }
+
+    fn format(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::InternalOpenSSLError(stack) =>
+                write!(fmt, "Error: Could not get X509 extension; {}", stack),
+            Self::ExtensionNotFoundError =>
+                write!(fmt, "Error: Could not get X509 extension; Reason: Could not find any matching extension."),
+            Self::ExtensionAmbiguousError =>
+                write!(fmt, "Error: Could not get X509 extension; Reason: Tried to read an extension, but found multiple."),
+        }
+    }
+}
+
+impl fmt::Debug for X509D2iError {
+    fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
+        self.format(fmt)
+    }
+}
+
+impl fmt::Display for X509D2iError {
+    fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
+        self.format(fmt)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     #[cfg(not(ossl310))]

openssl/src/ssl/connector.rs

@@ -417,7 +417,7 @@ cfg_if! {
             use std::str;
             use once_cell::sync::OnceCell;
 
-            use crate::error::ErrorStack;
+            use crate::error::{ErrorStack, X509D2iError};
             use crate::ex_data::Index;
             use crate::nid::Nid;
             use crate::ssl::Ssl;
@@ -460,8 +460,9 @@ cfg_if! {
 
             fn verify_hostname(domain: &str, cert: &X509Ref) -> bool {
                 match cert.subject_alt_names() {
-                    Some(names) => verify_subject_alt_names(domain, names),
-                    None => verify_subject_name(domain, &cert.subject_name()),
+                    Ok(names) => verify_subject_alt_names(domain, names),
+                    Err(X509D2iError::ExtensionNotFoundError) => verify_subject_name(domain, &cert.subject_name()),
+                    Err(e) => panic!("Error when fetching alt names from certificate: {}", e),
                 }
             }
 

openssl/src/x509/mod.rs

@@ -29,7 +29,7 @@ use crate::asn1::{
 };
 use crate::bio::MemBioSlice;
 use crate::conf::ConfRef;
-use crate::error::ErrorStack;
+use crate::error::{ErrorStack, X509D2iError};
 use crate::ex_data::Index;
 use crate::hash::{DigestBytes, MessageDigest};
 use crate::nid::Nid;
@@ -419,63 +419,85 @@ impl X509Ref {
             ffi::X509_issuer_name_hash(self.as_ptr()) as u32
         }
     }
+    fn create_d2i_result<T>(critical: i32, out: Option<T>) -> Result<T, X509D2iError> {
+        match (critical, out) {
+            (0 | 1, Some(out)) => Ok(out),
+            // -1 means the extension wasn't found, -2 means multiple were found.
+            (-1, _) => Err(X509D2iError::extension_not_found_error()),
+            (-2, _) => Err(X509D2iError::extension_ambiguous_error()),
+            // A critical value of 0 or 1 suggests success, but a null pointer
+            // was returned so something went wrong.
+            (0 | 1, None) => Err(X509D2iError::internal_openssl_error(ErrorStack::get())),
+            (c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical),
+        }
+    }
 
     /// Returns this certificate's subject alternative name entries, if they exist.
     #[corresponds(X509_get_ext_d2i)]
-    pub fn subject_alt_names(&self) -> Option<Stack<GeneralName>> {
-        unsafe {
+    pub fn subject_alt_names(&self) -> Result<Stack<GeneralName>, X509D2iError> {
+        let mut critical = -1;
+        let out = unsafe {
             let stack = ffi::X509_get_ext_d2i(
                 self.as_ptr(),
                 ffi::NID_subject_alt_name,
-                ptr::null_mut(),
+                &mut critical as *mut _,
                 ptr::null_mut(),
             );
             Stack::from_ptr_opt(stack as *mut _)
-        }
+        };
+
+        Self::create_d2i_result(critical, out)
     }
 
     /// Returns this certificate's CRL distribution points, if they exist.
     #[corresponds(X509_get_ext_d2i)]
-    pub fn crl_distribution_points(&self) -> Option<Stack<DistPoint>> {
-        unsafe {
+    pub fn crl_distribution_points(&self) -> Result<Stack<DistPoint>, X509D2iError> {
+        let mut critical = -1;
+        let out = unsafe {
             let stack = ffi::X509_get_ext_d2i(
                 self.as_ptr(),
                 ffi::NID_crl_distribution_points,
-                ptr::null_mut(),
+                &mut critical as *mut _,
                 ptr::null_mut(),
             );
             Stack::from_ptr_opt(stack as *mut _)
-        }
+        };
+
+        Self::create_d2i_result(critical, out)
     }
 
     /// Returns this certificate's issuer alternative name entries, if they exist.
     #[corresponds(X509_get_ext_d2i)]
-    pub fn issuer_alt_names(&self) -> Option<Stack<GeneralName>> {
-        unsafe {
+    pub fn issuer_alt_names(&self) -> Result<Stack<GeneralName>, X509D2iError> {
+        let mut critical = -1;
+        let out = unsafe {
             let stack = ffi::X509_get_ext_d2i(
                 self.as_ptr(),
                 ffi::NID_issuer_alt_name,
-                ptr::null_mut(),
+                &mut critical as *mut _,
                 ptr::null_mut(),
             );
             Stack::from_ptr_opt(stack as *mut _)
-        }
+        };
+        Self::create_d2i_result(critical, out)
     }
 
     /// Returns this certificate's [`authority information access`] entries, if they exist.
     ///
     /// [`authority information access`]: https://tools.ietf.org/html/rfc5280#section-4.2.2.1
     #[corresponds(X509_get_ext_d2i)]
-    pub fn authority_info(&self) -> Option<Stack<AccessDescription>> {
-        unsafe {
+    pub fn authority_info(&self) -> Result<Stack<AccessDescription>, X509D2iError> {
+        let mut critical = -1;
+        let out = unsafe {
             let stack = ffi::X509_get_ext_d2i(
                 self.as_ptr(),
                 ffi::NID_info_access,
-                ptr::null_mut(),
+                &mut critical as *mut _,
                 ptr::null_mut(),
             );
             Stack::from_ptr_opt(stack as *mut _)
-        }
+        };
+        Self::create_d2i_result(critical, out)
     }
 
     /// Retrieves the path length extension from a certificate, if it exists.
@@ -814,9 +836,15 @@ impl fmt::Debug for X509 {
         debug_struct.field("signature_algorithm", &self.signature_algorithm().object());
         debug_struct.field("issuer", &self.issuer_name());
         debug_struct.field("subject", &self.subject_name());
-        if let Some(subject_alt_names) = &self.subject_alt_names() {
-            debug_struct.field("subject_alt_names", subject_alt_names);
-        }
+        match &self.subject_alt_names() {
+            Ok(subject_alt_names) => {
+                debug_struct.field("subject_alt_names", subject_alt_names);
+            }
+            Err(X509D2iError::ExtensionNotFoundError) => {
+                // found nothing, but this is ok
+            }
+            Err(e) => panic!("{}", e),
+        };
         debug_struct.field("not_before", &self.not_before());
         debug_struct.field("not_after", &self.not_after());
 
@@ -1711,7 +1739,7 @@ impl X509RevokedRef {
     ///
     /// This returns None if the extension is not present or occurs multiple times.
     #[corresponds(X509_REVOKED_get_ext_d2i)]
-    pub fn extension<T: ExtensionType>(&self) -> Result<Option<(bool, T::Output)>, ErrorStack> {
+    pub fn extension<T: ExtensionType>(&self) -> Result<(bool, T::Output), X509D2iError> {
         let mut critical = -1;
         let out = unsafe {
             // SAFETY: self.as_ptr() is a valid pointer to an X509_REVOKED.
@@ -1726,13 +1754,14 @@ impl X509RevokedRef {
             T::Output::from_ptr_opt(ext as *mut _)
         };
         match (critical, out) {
-            (0, Some(out)) => Ok(Some((false, out))),
-            (1, Some(out)) => Ok(Some((true, out))),
+            (0, Some(out)) => Ok((false, out)),
+            (1, Some(out)) => Ok((true, out)),
             // -1 means the extension wasn't found, -2 means multiple were found.
-            (-1 | -2, _) => Ok(None),
+            (-1, _) => Err(X509D2iError::extension_not_found_error()),
+            (-2, _) => Err(X509D2iError::extension_ambiguous_error()),
             // A critical value of 0 or 1 suggests success, but a null pointer
             // was returned so something went wrong.
-            (0 | 1, None) => Err(ErrorStack::get()),
+            (0 | 1, None) => Err(X509D2iError::internal_openssl_error(ErrorStack::get())),
             (c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical),
         }
     }
@@ -1947,7 +1976,7 @@ impl X509CrlRef {
     ///
     /// This returns None if the extension is not present or occurs multiple times.
     #[corresponds(X509_CRL_get_ext_d2i)]
-    pub fn extension<T: ExtensionType>(&self) -> Result<Option<(bool, T::Output)>, ErrorStack> {
+    pub fn extension<T: ExtensionType>(&self) -> Result<(bool, T::Output), X509D2iError> {
         let mut critical = -1;
         let out = unsafe {
             // SAFETY: self.as_ptr() is a valid pointer to an X509_CRL.
@@ -1962,13 +1991,14 @@ impl X509CrlRef {
             T::Output::from_ptr_opt(ext as *mut _)
         };
         match (critical, out) {
-            (0, Some(out)) => Ok(Some((false, out))),
-            (1, Some(out)) => Ok(Some((true, out))),
+            (0, Some(out)) => Ok((false, out)),
+            (1, Some(out)) => Ok((true, out)),
             // -1 means the extension wasn't found, -2 means multiple were found.
-            (-1 | -2, _) => Ok(None),
+            (-1, _) => Err(X509D2iError::extension_not_found_error()),
+            (-2, _) => Err(X509D2iError::extension_ambiguous_error()),
             // A critical value of 0 or 1 suggests success, but a null pointer
             // was returned so something went wrong.
-            (0 | 1, None) => Err(ErrorStack::get()),
+            (0 | 1, None) => Err(X509D2iError::internal_openssl_error(ErrorStack::get())),
             (c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical),
         }
     }

openssl/src/x509/tests.rs

@@ -28,6 +28,7 @@ use crate::x509::{
     CrlStatus, X509Crl, X509Extension, X509Name, X509Req, X509StoreContext, X509VerifyResult, X509,
 };
 
+use crate::error::X509D2iError;
 #[cfg(ossl110)]
 use foreign_types::ForeignType;
 use hex::{self, FromHex};
@@ -280,7 +281,11 @@ fn test_aia_ca_issuer() {
     // Without AIA
     let cert = include_bytes!("../../test/cert.pem");
     let cert = X509::from_pem(cert).unwrap();
-    assert!(cert.authority_info().is_none());
+    match cert.authority_info() {
+        Ok(_) => panic!("Should not find dist point"),
+        Err(X509D2iError::ExtensionNotFoundError) => { /* ok */ }
+        Err(e) => panic!("Wrong error: {}", e),
+    }
 }
 
 #[test]
@@ -701,10 +706,7 @@ fn test_crl_entry_extensions() {
     let crl = include_bytes!("../../test/entry_extensions.crl");
     let crl = X509Crl::from_pem(crl).unwrap();
 
-    let (critical, access_info) = crl
-        .extension::<AuthorityInformationAccess>()
-        .unwrap()
-        .expect("Authority Information Access extension should be present");
+    let (critical, access_info) = crl.extension::<AuthorityInformationAccess>().unwrap();
     assert!(
         !critical,
         "Authority Information Access extension is not critical"
@@ -724,7 +726,6 @@ fn test_crl_entry_extensions() {
 
     let (critical, issuer) = entry
         .extension::<CertificateIssuer>()
-        .unwrap()
         .expect("Certificate issuer extension should be present");
     assert!(critical, "Certificate issuer extension is critical");
     assert_eq!(issuer.len(), 1, "Certificate issuer should have one entry");
@@ -740,7 +741,6 @@ fn test_crl_entry_extensions() {
     #[allow(unused_variables)]
     let (critical, reason_code) = entry
         .extension::<ReasonCode>()
-        .unwrap()
         .expect("Reason code extension should be present");
     assert!(!critical, "Reason code extension is not critical");
     #[cfg(ossl110)]
@@ -1175,7 +1175,11 @@ fn test_dist_point() {
 fn test_dist_point_null() {
     let cert = include_bytes!("../../test/cert.pem");
     let cert = X509::from_pem(cert).unwrap();
-    assert!(cert.crl_distribution_points().is_none());
+    match cert.crl_distribution_points() {
+        Ok(_) => panic!("Should not find dist point"),
+        Err(X509D2iError::ExtensionNotFoundError) => { /* ok */ }
+        Err(e) => panic!("Wrong error: {}", e),
+    }
 }
 
 #[test]