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

Fix a series of security issues #1854

Merged
merged 5 commits into from Mar 24, 2023
Merged

Conversation

alex
Copy link
Collaborator

@alex alex commented Mar 24, 2023

See individual commits for details

@alex alex force-pushed the davids-openssl-of-horrors branch from 3cb72dc to 50c7e76 Compare March 24, 2023 01:12
@alex alex force-pushed the davids-openssl-of-horrors branch from 50c7e76 to 6ced4f3 Compare March 24, 2023 01:15
@alex alex merged commit 5efceaa into sfackler:master Mar 24, 2023
51 checks passed
@alex alex deleted the davids-openssl-of-horrors branch March 24, 2023 01:34
This was referenced Mar 24, 2023
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.

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 this pull request may close these issues.

None yet

3 participants