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

Ambiguous use of SSL_CERT_DIR #18068

Closed
hlandau opened this issue Apr 8, 2022 · 6 comments
Closed

Ambiguous use of SSL_CERT_DIR #18068

hlandau opened this issue Apr 8, 2022 · 6 comments
Assignees
Labels
branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug

Comments

@hlandau
Copy link
Member

hlandau commented Apr 8, 2022

Currently, the SSL_CERT_DIR environment variable is used to specify the default location both for X509_LOOKUP_hash_dir() as well as X509_LOOKUP_store().

However, X509_LOOKUP_hash_dir() interprets the value as a path delimiter-separated list of paths, whereas X509_LOOKUP_store() interprets this value as a URI.

This means that if you specify e.g. file:///etc/ssl/certs, and setup both path and store lookups (for example by calling X509_STORE_set_default_paths), at first a directory lookup is attempted in the directory ./file relative to the current working directory, followed by the directory ///etc/ssl/certs. Then the file store provider will subsequently handle it as a supported URL.

This seems bizarre and was probably not intended. It may also be a slight security concern; e.g. if an environment sets SSL_CERT_DIR=somestoreprovider://foo, if an attacker can cause someone to use OpenSSL in a directory containing a directory named somestoreprovider under the attacker's control, the attacker could add certificates they control to the trusted list.

I suggest we resolve this by:

  • adding an environment variable SSL_CERT_URI preferred for all future usage
  • if SSL_CERT_URI is set, the store lookup method will not check SSL_CERT_DIR
  • if SSL_CERT_DIR looks like an URL (^([a-zA-Z0-9-+]{2,})://), do not have X509_LOOKUP_hash_dir() process it.

The third point is a potential compatibility change, but would require someone to be using SSL_CERT_DIR to specify a relative path followed by an absolute path which is specified via redundant leading slashes. This is theoretically possible but the combination seems very unlikely.

My intention would then be to have SSL_CERT_URI default to capi:// on Windows and be empty otherwise. The new CAPI store (re #18020) will probably also be behind a compile time option.

@hlandau hlandau added the triaged: bug The issue/pr is/fixes a bug label Apr 8, 2022
@hlandau hlandau self-assigned this Apr 8, 2022
@t8m
Copy link
Member

t8m commented Apr 11, 2022

This looks like a good plan. However the third thing would be something to backport as a bug fix and we should IMO backport it.
So please do that as a separate PR.

@t8m t8m added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch labels Apr 11, 2022
@levitte
Copy link
Member

levitte commented Apr 11, 2022

Two notes:

  • SSL_CERT_STORE or SSL_CERT_STORE_URI rather than SSL_CERT_URI, as the latter can be read as a single cert URI, which isn't necessarily the case.
  • capi: isn't a very good scheme name for the Windows cert stores, as doesn't have much to do with CAPI per se, except for being input data... this becomes clear when you realise that the exact same stores are used for CNG (the next generation crypto API), but you wouldn't change the store scheme to cng: all of a sudden 😉

@t8m
Copy link
Member

t8m commented Apr 11, 2022

There is one more thing. The :// as an indicator for URIs is not perfect as URIs do not necessarily have to include the double forward slash // at the beginning. For example the pkcs11 URIs do not have the double slash.

@t8m
Copy link
Member

t8m commented Apr 11, 2022

Perhaps just anything that has ^([a-zA-Z0-9-+]{2,}): should be interpreted as URI. IMO relative paths with multiple paths in SSL_CERT_DIR should be quite uncommon.

@levitte
Copy link
Member

levitte commented May 19, 2022

Perhaps just anything that has ^([a-zA-Z0-9-+]{2,}): should be interpreted as URI. IMO relative paths with multiple paths in SSL_CERT_DIR should be quite uncommon.

foo:bar/cookie is perfectly legitimate as a multi-directory path, at least in Unix $PATH terms...

@hlandau
Copy link
Member Author

hlandau commented May 19, 2022

Two notes:

  • SSL_CERT_STORE or SSL_CERT_STORE_URI rather than SSL_CERT_URI, as the latter can be read as a single cert URI, which isn't necessarily the case.

The existing code expects SSL_CERT_DIR to contain a list of directories or a single URI. In other words, you can't specify multiple URIs. That is, X509_LOOKUP_store only supports a single URI, whereas X509_LOOKUP_hash_dir supports multiple directories.

Do we want a PR to allow the new SSL_CERT_URI to specify multiple stores? How should they be delimited — spaces?

  • capi: isn't a very good scheme name for the Windows cert stores, as doesn't have much to do with CAPI per se, except for being input data... this becomes clear when you realise that the exact same stores are used for CNG (the next generation crypto API), but you wouldn't change the store scheme to cng: all of a sudden

Makes sense. Any suggestions for a better name? wincrypt?

hlandau added a commit to hlandau/openssl that referenced this issue Jun 29, 2022
hlandau added a commit to hlandau/openssl that referenced this issue Jul 25, 2022
hlandau added a commit to hlandau/openssl that referenced this issue Aug 19, 2022
hlandau added a commit to hlandau/openssl that referenced this issue Aug 19, 2022
hlandau added a commit to hlandau/openssl that referenced this issue Sep 8, 2022
hlandau added a commit to hlandau/openssl that referenced this issue Sep 13, 2022
sftcd pushed a commit to sftcd/openssl that referenced this issue Sep 24, 2022
Fixes openssl#18068.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18070)
beldmit pushed a commit to beldmit/openssl that referenced this issue Dec 26, 2022
Fixes openssl#18068.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18070)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants