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.

Sign up for GitHub

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

Jump to bottom

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.

rust-openssl/openssl/src/x509/mod.rs

Lines 1950 to 1974 in 83b3186

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),
}
}

@botovq botovq mentioned this issue May 15, 2024
Open
@HolyShitMan
Copy link

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

@HolyShitMan HolyShitMan linked a pull request May 17, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

New X509D2iError to improve error checking for X509_ext_get_d2i() HolyShitMan/rust-openssl
2 participants
@botovq @HolyShitMan