On Wed, 07 Oct 2020 21:25:57 +0200,
Nicola Tuveri wrote:
> 
> On Wed, 7 Oct 2020 at 20:45, Richard Levitte <levi...@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         levi...@openssl.org
OpenSSL Project         http://www.openssl.org/~levitte/

Reply via email to