Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error checking for X509_ext_get_d2i() #2226

Open
botovq opened this issue Apr 24, 2024 · 1 comment · May be fixed by #2240
Open

Improve error checking for X509_ext_get_d2i() #2226

botovq opened this issue Apr 24, 2024 · 1 comment · May be fixed by #2240

Comments

@botovq
Copy link
Contributor

botovq commented Apr 24, 2024

Quoting the BoringSSL documentation, which is very explicit about it:

// WARNING: This function is difficult to use correctly. Callers should pass a
// non-NULL |out_critical| and check both the return value and |*out_critical|
// to handle errors. If the return value is NULL and |*out_critical| is not -1,
// there was an error. Otherwise, the function succeeded and but may return NULL
// for a missing extension. Callers should pass NULL to |out_idx| so that
// duplicate extensions are handled correctly.

The internal callers of X509_CRL_get_ext_d2i() and X509_REVOKED_get_ext_d2i() handle this closer to correctly, although you can't rely on the library setting an error code if NULL is returned and critical != -1, and I think -2 (multiple extensions of type nid found) should be an error.

pub fn extension<T: ExtensionType>(&self) -> Result<Option<(bool, T::Output)>, ErrorStack> {
let mut critical = -1;
let out = unsafe {
// SAFETY: self.as_ptr() is a valid pointer to an X509_CRL.
let ext = ffi::X509_CRL_get_ext_d2i(
self.as_ptr(),
T::NID.as_raw(),
&mut critical as *mut _,
ptr::null_mut(),
);
// SAFETY: Extensions's contract promises that the type returned by
// OpenSSL here is T::Output.
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))),
// -1 means the extension wasn't found, -2 means multiple were found.
(-1 | -2, _) => Ok(None),
// 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()),
(c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical),
}
}

@HolyShitMan
Copy link

I try to address this issue. Pull request is coming soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants