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

openssl-sys: Provide information about the default certificate paths #2260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hvenev
Copy link
Contributor

@hvenev hvenev commented Jul 7, 2024

The goal of these functions is to contain the logic implemented by the openssl-probe crate. The openssl-sys crate knows whether we are linked against the system OpenSSL library, and if so, it can use it to get the correct certificate paths.

If we are using the vendored version, we keep the old insecure behavior where we randomly guess paths and hope one works.

The goal of these functions is to contain the logic implemented by the
`openssl-probe` crate. The `openssl-sys` crate knows whether we are
linked against the system OpenSSL library, and if so, it can use it to
get the correct certificate paths.

If we are using the vendored version, we keep the old insecure behavior
where we randomly guess paths and hope one works.
@sfackler
Copy link
Owner

sfackler commented Jul 7, 2024

The openssl-sys crate knows whether we are linked against the system OpenSSL library, and if so, it can use it to get the correct certificate paths.

It does not. It only knows if we are linked to the vendored copy or not.

@hvenev
Copy link
Contributor Author

hvenev commented Jul 7, 2024

The non-vendored copy is the one explicitly provided by whoever builds the (downstream) crate.

Perhaps another feature probe-insecure can be used to enable the insecure probing behavior when the non-vendred OpenSSL library is not the system one, e.g., when attempting to create a statically linked "portable" binary.

@hvenev
Copy link
Contributor Author

hvenev commented Jul 7, 2024

Or perhaps the search path could be a compile-time option, just like OPENSSL_DIR / OPENSSL_LIB_DIR / OPENSSL_INCLUDE_DIR?

In any case, my goal is to make sure that downstream users like native-tls cannot (by default) end up looking at what on Android appears to be the data directory of some app, but may on other systems be any unspecified "data" directory that may or may not be world-writable.

@sfackler
Copy link
Owner

sfackler commented Jul 7, 2024

I don't see how this change would affect the behavior of native-tls.

@hvenev
Copy link
Contributor Author

hvenev commented Jul 7, 2024

This change by itself wouldn't. A subsequent change to have openssl-probe use openssl_sys::probe::* would then fix it. I should have been clearer about that.

The default (not(vendored)) is to use the system OpenSSL library, in which case we wouldn't end up looking under /data.

@sfackler
Copy link
Owner

sfackler commented Jul 7, 2024

Fundamentally, I am not interested in maintaining the big-list-of-paths in openssl-sys.

Again, not(vendored) does not guarantee "the system OpenSSL library".

@hvenev
Copy link
Contributor Author

hvenev commented Jul 7, 2024

(sorry about the deleted message, used the wrong account)

Fundamentally, I am not interested in maintaining the big-list-of-paths in openssl-sys.

Again, not(vendored) does not guarantee "the system OpenSSL library".

Yes, that's true. It probably makes more sense to be able to provide a list at build time, and to keep the default list in openssl-probe as it is right now. However, I think we should at least aim for the following:

  • The default should be to try to use the system OpenSSL library.
  • If we know we are using the system OpenSSL library, downstream crates should be expected not to probe random paths for things that look like certificates.
  • It should be possible for downstream crates to know whether they can rely on openssl-sys to know where its certificates are.

One thing I guess we know for sure is that when cfg(vendored), we are not using the system OpenSSL. How well can we guess during build time when cfg(not(vendored))?

@sfackler
Copy link
Owner

sfackler commented Jul 7, 2024

How well can we guess during build time when cfg(not(vendored))?

You can't. When dynamically linking the build-time library may not even be the same as the run-time library, beyond ABI compatibility.

@hvenev
Copy link
Contributor Author

hvenev commented Jul 7, 2024

How well can we guess during build time when cfg(not(vendored))?

You can't. When dynamically linking the build-time library may not even be the same as the run-time library, beyond ABI compatibility.

Isn't the point of dynamic linking that the user provides the OpenSSL library? In that case I think it's better if the user makes sure that the OpenSSL library they provide knows where its certificates are.

@sfackler
Copy link
Owner

sfackler commented Jul 7, 2024

That is a thing that you can do with dynamic linking, but I wouldn't say that it's the "point" of it. You can absolutely ship a dynamically linked libssl with your executable if you want.

@hvenev
Copy link
Contributor Author

hvenev commented Jul 7, 2024

You can absolutely ship a dynamically linked libssl with your executable if you want.

"You" here being the person building the (downstream) crate, hoping to provide a "portable" install tarball. In that case the probing behavior can be specified during build time.

@hvenev
Copy link
Contributor Author

hvenev commented Jul 8, 2024

So, how about the following in openssl_sys::probe

pub fn default_cert_file() -> Option<PathBuf> { ... }
pub fn default_cert_dir() -> Option<PathBuf> { ... }

where no probing is done. Instead:

  1. If at build time OPENSSL_CERT_FILE was specified, default_cert_file() returns that (empty means None). Similarly, OPENSSL_CERT_DIR and default_cert_dir().
  2. Otherwise, if the vendored OpenSSL is used, None is returned.
  3. Otherwise, if OpenSSL is linked statically (or some other heuristic, or print a warning, or fail the build?), None is returned.
  4. Otherwise, X509_get_default_cert_file / X509_get_default_cert_dir are called and their results are returned.

Then openssl_probe would do the following:

  1. Check if the appropriate environment variable is set. If so, do nothing.
  2. Otherwise, call openssl_sys::probe::*. If the result is Some(p), update the environment variable, return p.
  3. Otherwise, fall back to the current insecure haphazard probing.

This way the behavior is controlled at build time and the default isn't a security footgun. With a good heuristic in step 3 we can avoid most breakage. What do you think?

@hvenev
Copy link
Contributor Author

hvenev commented Aug 31, 2024

Should I open an issue in the users of rust-openssl/openssl-probe instead? Some of them have an actual security vulnerability where they sometimes look under /data for certificates, and there is no reason to believe that /data is trustworthy -- it could be a world-writable directory on a multi-user system, for example.

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.

2 participants