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

Fix a series of security issues #1854

Merged
merged 5 commits into from Mar 24, 2023
Merged

Fix a series of security issues #1854

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
482575b
Resolve an injection vulnerability in SAN creation
alex Mar 22, 2023
332311b
Resolve an injection vulnerability in EKU creation
alex Mar 22, 2023
78aa9aa
Always provide an X509V3Context in X509Extension::new because OpenSSL…
alex Mar 22, 2023
a752805
Document the horror show
alex Mar 22, 2023
6ced4f3
Fix race condition with X509Name creation
alex Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 36 additions & 4 deletions openssl/src/x509/mod.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, if we want to avoid null pointer and initialize the ctx, why not using MaybeUninit instead of mem::zeroed based on the description in documentation:

This has the same effect as MaybeUninit::zeroed().assume_init(). It is useful for FFI sometimes, but should generally be avoided.

Even though ctx is not a reference here, it is still better to use MaybeUninit, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably using MaybeUnit consistently would be better, yeah. Would you like to submit a PR to do so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to do it, but I am still a bit confused about the root cause.

If I understand correctly, null ptr dereference happens because context was sent to C function ffi::X509V3_EXT_nconf as a null mutable pointer. However, based on the codebase of openssl crate, it is a common behavior to pass null_ptr directly to the C function.

I believe there could be another null pointer deref issue hidden in the crate. Is there any suggestion to avoid such issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null-pointer derefs are inside of OpenSSL, it wasn't a crash in Rust itself. The problem is that some extensions require a context value, but others allow null just fine.

There's test cases for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think my description was not precise enough. "dereference" part was in OpenSSL(C), and "null-pointer" part was led by None => null_mut in Rust itself.

However, what I am confused is that: Why is it Rust's responsibility to fix the bug? null-pointer derefs happened in C code, then X509V3_EXT_nconf should check whether context value is null for some extensions, isn't it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a safe Rust function. It should not be possible to cause a null pointer deref from calling it.

Expand Up @@ -816,14 +816,30 @@ impl X509Extension {
) -> Result<X509Extension, ErrorStack> {
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)).map(X509Extension)
cvt_p(ffi::X509V3_EXT_nconf(conf, context_ptr, name, value)).map(X509Extension)
}
}

Expand All @@ -841,14 +857,30 @@ impl X509Extension {
value: &str,
) -> Result<X509Extension, ErrorStack> {
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)).map(X509Extension)
cvt_p(ffi::X509V3_EXT_nconf_nid(conf, context_ptr, name, value)).map(X509Extension)
}
}

Expand Down
10 changes: 9 additions & 1 deletion openssl/src/x509/tests.rs
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ use crate::x509::X509PurposeId;
#[cfg(any(ossl102, libressl261))]
use crate::x509::X509PurposeRef;
use crate::x509::{
CrlStatus, X509Crl, X509Name, X509Req, X509StoreContext, X509VerifyResult, X509,
CrlStatus, X509Crl, X509Extension, X509Name, X509Req, X509StoreContext, X509VerifyResult, X509,
};
use hex::{self, FromHex};
#[cfg(any(ossl102, libressl261))]
Expand Down Expand Up @@ -287,6 +287,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();
Expand Down