sfackler / rust-openssl

OpenSSL bindings for Rust
1.4k stars 752 forks source link

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

Open hvenev opened 4 months ago

hvenev commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

hvenev commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

(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:

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 2 months ago

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.