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

pkey: track whether pkey is private key or not #527

Closed
wants to merge 7 commits into from

Conversation

rhenium
Copy link
Member

@rhenium rhenium commented Jul 26, 2022

There are multiple places where it's necessary to know whether a pkey
is a private key, a public key, or just key parameters. Unfortunately,
OpenSSL doesn't expose an API for this purpose (even though it has one
for its internal use).

Currently, we drill down into the backing object, such as RSA, and see
if the corresponding fields are set or not to determine it. This doesn't
work on OpenSSL 3.0 because of the architecture changes.

Let's manually track this information in an instance variable for now.
This has been partly done for ENGINE-backed pkeys. Now all pkeys get
this flag.

PKeys are immutable on OpenSSL 3.0, so it just needs to be stored once
on initialization. On OpenSSL 1.1 or before (including LibreSSL), it
must be updated whenever a modification is made to the object.

This comes with a slight behavior change. PKey returned by following
method will be explicitly marked as "public", even if it happens to
point at an EVP_PKEY struct containing private key components. I expect
the effect is minimum since these methods explicitly say "public key".

  • OpenSSL::X509::Certificate#public_key
  • OpenSSL::X509::Request#public_key
  • OpenSSL::Netscape::SPKI#public_key

@no6v
Copy link
Contributor

no6v commented Aug 19, 2022

I recently found that OpenSSL::PKey::EC.new("prime256v1").to_der dumps core, and have been searching the way to check the pkey is public? or not briefly.
This PR almostly implement that way, but the above code still dumps core.
I'm not sure such instance makes any sense, but would you consider to fix this segv? Thanks.

@rhenium
Copy link
Member Author

rhenium commented Aug 20, 2022

I recently found that OpenSSL::PKey::EC.new("prime256v1").to_der dumps core, and have been searching the way to check the pkey is public? or not briefly.
This PR almostly implement that way, but the above code still dumps core.
I'm not sure such instance makes any sense, but would you consider to fix this segv? Thanks.

ruby/openssl should not cause a segfault. This seems like a regression introduced by #328, which went into v3.0.

I hoped it would be checked by OpenSSL, like for all other EVP_PKEY types than EC, but it wasn't the case then.

@rhenium
Copy link
Member Author

rhenium commented Aug 31, 2022

This is currently blocked by failing CI checks on Ubuntu 22.04. It appears to be a bug that has been fixed by OpenSSL 3.0.4: openssl/openssl@9f3626f, specifically the diff in providers/implementations/encode_decode/decode_der2key.c. But Ubuntu's OpenSSL package is based on 3.0.2 and has yet to incorporate the patch.

I will need to figure out how I can request the bug fix backported, or come up with a workaround (if there is any).

With providers, pkeys are immutable and we should avoid using low-level
types such as RSA or EC_KEY.

Use this special macro instead of version numbers to make the intention
clear, and also to make it easier to update when LibreSSL gains
OpenSSL 3.0 providers support.
These don't work and make no sense on OpenSSL 3.0, since PKey instances
are immutable once initialized.
Avoid using DH_new() or d2i_DHparams_bio().

The path calling DH_new() creates an empty pkey. This is totally useless
on OpenSSL 3.0 because pkey instances are immutable.

Calling d2i_DHparams_bio() is not needed on OpenSSL 3.0. This format
should be handled by ossl_pkey_read_generic() already. Besides, it
creates a "legacy" pkey instance which prevents EVP_PKEY_todata() from
working properly.
DER encoding may be ambiguous because it doesn't contain information
about the key type or format. For example, DHParameters and
RSAPublicKey look identical on the DER encoding, which is a SEQUENCE
with two INTEGER inside.
There are multiple places where it's necessary to know whether a pkey
is a private key, a public key, or just key parameters. This is due to
two reasons:

 1. It's currently a responsibility of the caller to give a properly
    populated pkey instance to _some_ OpenSSL functions. For example,
    calling EVP_PKEY_sign() with an RSA pkey instance without the
    necessary components is known to cause a segfault.

 2. OpenSSL::PKey::{RSA,DSA,EC}#to_der behaves differently depending on
    it: they use the X.509 SubjectPublicKeyInfo structure instead of
    private key structures if the receiver pkey is a public key.

Currently, whenever this is necessary, we check the backing object, such
as RSA, and see if the fields are set or not. This approach won't always
work on OpenSSL 3.0 because of the architecture change. Unfortunately,
OpenSSL doesn't expose an API for this purpose (even though it has one
for its internal use).

As a workaround, let's manually track this information in an instance
variable. This has been partly done for ENGINE-backed pkeys. Now all
pkeys get this flag. This is straightforward on OpenSSL 3.0 as pkeys
are immutable once instantiated. On OpenSSL 1.1 or before (and current
versions of LibreSSL), it must be updated whenever a modification is
made to the object.

This commit comes with a slight behavior change. Pkeys returned by
following method will be explicitly marked as "public", even if it
happens to point at an EVP_PKEY struct containing private key
components. I expect the effect is minimum since these methods
explicitly say "public key".

 - OpenSSL::X509::Certificate#public_key
 - OpenSSL::X509::Request#public_key
 - OpenSSL::Netscape::SPKI#public_key
tenderlove pushed a commit to Shopify/ruby that referenced this pull request Oct 27, 2022
…e exporting

i2d_PUBKEY_bio() against an EC_KEY without the public key component
trggers a null dereference.

This is a regression introduced by commit ruby/openssl@56f0d34d63fb ("pkey:
refactor #export/#to_pem and #to_der", 2017-06-14).

Fixes ruby/openssl#527 (comment)
Fixes ruby/openssl#369 (comment)

ruby/openssl@f6ee0fa4de
@rhenium rhenium force-pushed the ky/pkey-track-private branch from 8e1c6e2 to dfbfc02 Compare December 18, 2022 10:11
@rhenium
Copy link
Member Author

rhenium commented Dec 18, 2022

After some thought, this approach to fully track the state of a key may not work with the current OpenSSL 3.0.x API. We plan to provide a wrapper for EVP_PKEY_fromdata() to construct a key from a set of individual parameters (see #555) but it's seemingly not possible to tell whether the resulting key is a private key or a public key.

For now, I think we still have to continue to check against the low-level structure (and ignore the deprecation warnings, #576).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants