Vote proposal: Private keys can exist independently of public keys

Thu Oct 8 14:06:57 UTC 2020

An update to keep this thread in sync: we moved the discussion between
me and Richard partially offline and partially on
<https://github.com/openssl/openssl/issues/12612> (the issue that
triggered this vote proposal).

Part of the discussion was on our positions and rationales regarding
the actual vote and not the vote proposal, so they did not belong
here, but I feel like it would be better to include in this thread a
point I made in the issue discussion in support of amending the vote
text to be more explicit regarding the amount of work required to
change the "keypair assumption" in a way that is safe for all of our
users.

The full details are at
<https://github.com/openssl/openssl/issues/12612#issuecomment-705500620>
but the relevant part that I think is worth including in this thread
is the following:

> Regardless of the fact that, in this particular instance, arguments
> are checked and we don't fail with a segfault, my point in the
> discussion about the vote proposal is exactly that to propose to
> change the underlying assumption first we should build test coverage
> to catch the segfaults where they could happen and fix them!
>
> Currently our test coverage for "incomplete keys" is 0% or very close
> to it, because we rely greatly on data driven tests (and PEM in
> particular), and while we do exercise encodings that omit the optional
> public key components, this still means we are not covering the
> codepaths that a user could hit if using the API to intentionally
> violate the "keypair assumption" as in the description of this issue.

To be fair, when I say close to 0%, I am aware we have tests specific
to provider-native keys, that feed the raw data, and are offering some
level of assurance for this test. What I feel is gravely undertested
to undertake this change in the timeframe of 3.0 is exhaustive unit
testing of our wide API and integration/system tests to ensure that by
removing this assumption we do not impact established patterns in the
existing user codebase.

Yes, as #12612 shows also being strict about this introduces breaking
changes for the users that used the API to programmatically create
patterns that work in 1.1.1 even if they defied the documented
"keypair assumption", but I see a big difference in breaking leaning
towards hardening and breaking leaning towards potential run-time
exceptions!

I am not saying we should never change the "keypair assumption", but
that on voting to change this assumption in the 3.0 timeframe, we
should remind ourselves of the current poor coverage on some details,
the risk that could emerge out of this change and the amount of work
required to reach exhaustive testing to be able to quantify these
risks and the work required to make all the existing codebase robust
to this change.

-- Nicola

On Thu, 8 Oct 2020 at 00:10, Richard Levitte <levitte at openssl.org> wrote:
>
> On Wed, 07 Oct 2020 21:25:57 +0200,
> Nicola Tuveri wrote:
> >
> > On Wed, 7 Oct 2020 at 20:45, Richard Levitte <levitte at openssl.org> wrote:
> > >
> > > I'm as culpable as anyone re pushing the "convention" that an EVP_PKEY
> > > with a private key should have a public key.  I was incorrect.
> >
> > Sure, my example was not about pointing fingers!
>
> I didn't try to imply that you did, just wanted to own up to my part
> in it.
>
> > It's just to give recent data points on how the documentation written
> > in 2006 is probably not something as stale as one can think, because
> > until recently we were leveraging that assumption at least in some
> > sections of the lower layers.
>
> Were we?  Not in the operations...  however, this picked my curiousity
> and got me to look at this from a different angle.  See, it seems that
> what we're mostly disputing is the behaviour of the keymgmt import
> function, which is related to loading keys from file, among others.
>
> When loading private keys from PEM files in 1.1.1, we do rely entirely
> on the related i2d functions, which do implement the ASN.1 structures
> quite failthfully, so our basis of deciding what's ok or not is at
> influenced by associated standardised ASN.1 key formats.
>
> I did just now look up the ASN.1 key format for ECC keys, and it seems
> that our EC keymgmt ends up violating that standard.  I've commented
> further on that in the issue:
> https://github.com/openssl/openssl/issues/12612#issuecomment-705162303
>
> > I think the lack of NULL checks is intertwined with this problem as
> > long as in some code that does access the public key component we
> > ensured we dereference without making the NULL check because of the
> > assumption.
>
> Looking again at #12612, I don't quite see where the conclusion that
> we're missning NULL checks comes from.  EVP_DigestSignInit() fails,
> quite correctly, because it's been given a key that couldn't be
> (automatically) exported to a provider, because the keymgmt import
> refused it for the lack of a public key.  Had the import accepted this
> key material, the signature operation would have worked with no issues
> whatsoever as far as I can see, because it doesn't use the public key.
> And the code we use for the actual computation does a NULL check on
> the private key.
>
> I can't see that the issue was a lack of NULL check, rather the
> contrary in this case.
>
> > The problem I see with spot checking rather than writing a
> > comprehensive suite of tests, is that given our wide API surface and
> > the compromises taken in the transition from non-opaque stack
> > allocated objects in 1.0.2 to opaque objects in 1.1.1+, we might have
> > non-obvious places where dereferencing the public key without checking
> > for NULL can bite us.
>
> The current ECC private keys embedded in test/recipes/30-test_evp_data/evppkey_ecc.txt
> (both in 1.1.1 and 3.0) currently all have a public key present.
> It should be possible to change some of them to have the public key
> removed, or add test stanza with private keys that don't have the
> public part.
>
> I agree that we should add such stanzas, and probably test them on
> 1.1.1 as well as on 3.0, to ensure that we don't have the dormant bug
> that you fear that we might have.
>
> Mind you, there's another possible answer for ECC keys.  If you look
> at crypto/dh/dh_ameth.c and crypto/dsa/dsa_ameth.c, and look up
> dh_priv_decode() and dsa_priv_decode()...  well, you only need to read
> the comment, really.
> See, a PKCS#8 structure with a DH or DSA key only has the private
> key.  No public key whatsoever, so DSA and DH keys can be said to be
> in the same situation as ECC keys...  it's just that our legacy code
> to decode such PKCS#8 structures will recalculate the public key from
> the private key and the parameters.
> I'm not terribly keen on that idea, and specially not on the idea to
> possibly have to reproduce it in the provided decoders, which means
> that we're putting that pressure on other provider authors as well.
> However, if that suites everyone better, we have a precedent.
>
> Cheers,
> Richard
>
> --
> Richard Levitte         levitte at openssl.org
> OpenSSL Project         http://www.openssl.org/~levitte/