Vote proposal: Private keys can exist independently of public keys

Wed Oct 7 16:52:48 UTC 2020

As far as I can tell, the existing "enforcement" is in the algorithm
implementations.  I had a look through the following files in OpenSSL
1.1.1:

crypto/rsa/rsa_ossl.c
crypto/dsa/dsa_ossl.c
crypto/dh/dh_key.c
crypto/ec/ecdsa_ossl.c
crypto/ec/ecdh_ossl.c

I first had a look at the key computing functions in dh_key.c and
ecdh_ossl.c, because I was told that the public key is looked at
there.  This turns out to be somewhat false; they do look at a public
key, the public key of the peer key that they get passed, which isn't
quite the same.  However, when it comes to the DH or EC_KEY they work
with, only the private half is looked at,

Looking at dsa_ossl.c and ecdsa_ossl.c, I can see that the signing
functions do NOT look a |pub_key|, and the verification functions do
NOT look at |priv_key|.  This seems perfectly normal to me.

Similarly, the signing functions in rsa_ossl.c do NOT seem to look at
|d|, and the verification functions do NOT seem to look at |e|.

I took a moment to search through the corresponding *meth.c files to
see if I could quickly grep for some kind of check, and found none.
Mind you, that was a *quick* grep, someone more thorough might want to
confirm or contradict.

So, having looked through that code, my sense is that the "enforcement"
that's talked about is non-existent, or at least very unclear, and I
suspect that if there is some sort of "enforcement" at that level,
it's more accidental than by design...

I'm not sure what to make of the documentation from 2006.  Considering
the level of involvement there was from diverse people (2006 wasn't
exactly the most eventful time), there's a possibility that it was a
precaution ("don't touch that can of worms!") than a firm decision.

Cheers,
Richard

On Wed, 07 Oct 2020 17:34:25 +0200,
Nicola Tuveri wrote:
> 
> I believe the current formulation is slightly concealing part of the problem.
> 
> The way I see it, the intention if the vote passes is to do more than stated:
> 1. change the long-documented assumption
> 2. fix "regressions" in 3.0 limited to things that worked in a certain
> way in 1.1.1 (and maybe before), _despite_ the documented assumption
> 3. test all existing functions that access the public key component of
> `EVP_PKEY` (or lower level) objects to ensure they don't rely on the
> long-documented assumption
> 4. fix all the places that intentionally relied on the long-documented
> assumption and that were required to avoid "useless NULL checks"
> 
> I believe that to change a widely and longed-enforced assumption like
> this, we can't rely on a single case or a list of single cases that
> worked _despite_ the documented assumption as proof that things can
> (and should) work a certain way or another, and that before taking the
> decision this late in the development process the results of thorough
> testing are a prerequisite to assess the extent of code changes
> required by changing the assumption and the potential for instability
> and disruption that this can cause for our direct and indirect users
> if segfaults slip through our currently limited coverage as a
> consequence of this change.
> 
> I feel that we are currently underestimating the potential impact of
> this change, and currently motivated by a handful of isolated cases in
> which under a very specific set of conditions things aligned in a way
> that allowed certain usage patterns to succeed despite the documented
> assumption.
> I also feel that the "burden of the proof" of improving the test
> coverage to exhaustively cover all kinds of keys (both in their legacy
> form and in provider-native form), under all possible operations at
> different abstraction levels (e.g. limiting this example to
> sign/verify as the operation, testing should not be limited to
> `EVP_DigestSign/Verify()`, but also through
> `EVP_DigestSign/Verify{Init,Update,Final}()`,
> `EVP_PKEY_sign/verify*()`, `(EC)DSA_(do_)sign/verify()`, etc.,
> external testing with ENGINEs that might rely on the long-documented
> assumption), should fall on who is proposing this change, rather than
> committing that we will be able to increase our coverage to prevent
> unforeseen consequences of changing the assumption, before we can
> decide to commit to alter the assumption.
> 
> So, to better capture the extent of work required to apply the change
> and the risks associated with it I would rephrase the text vote as
> follows:
> 
> > We should change the 3.0 code to explicitly allow private components
> > to exist in keys without the public components also being present,
> > ensuring that, in `EVP` and in the lower abstraction layers, both in
> > legacy and in provider-native codepaths, _every_ instance in which any
> > public key component is dereferenced it is preceded by a NULL pointer
> > check or guaranteed non-NULL from any caller.
> > To change the long documented assumption, we commit to improve test
> > coverage of all public functions directly or indirectly triggering
> > potential access to public key components, to prevent the risk of user
> > facing crashes.
> 
> 
> Nicola
> 
> 
> 
> 
> On Wed, 7 Oct 2020 at 14:29, Matt Caswell <matt at openssl.org> wrote:
> >
> > Issue #12612 exposes a problem with how we handle keys that contain
> > private components but not public components.
> >
> > There is a widespread assumption in the code that keys with private
> > components must have public components. There is text in our public
> > documentation that states this (and that text dates back to 2006).
> >
> > OTOH, the code has not always enforced this. Issue #12612 describes a
> > scenario where this has not historically been enforced, and it now is in
> > the current 3.0 code causing a regression.
> >
> > There are differences of opinion on how this should be handled. Some
> > have the opinion that we should change the model so that we explicitly
> > allow private keys to exists without the public components. Others feel
> > that we should continue with the old model.
> >
> > It seems we need a vote to decide this. Here is my proposed vote text:
> >
> > We should change the 3.0 code to explicitly allow private components to
> > exist in keys without the public components also being present.
> >
> > Feedback please on the proposed vote text.
> >
> > Matt
> 
-- 
Richard Levitte         levitte at openssl.org
OpenSSL Project         http://www.openssl.org/~levitte/