Re: Vote proposal: Private keys can exist independently of public keys

2020-10-08 Thread Nicola Tuveri
An update to keep this thread in sync: we moved the discussion between
me and Richard partially offline and partially on
 (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

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  wrote:
>
> On Wed, 07 Oct 2020 21:25:57 +0200,
> Nicola Tuveri wrote:
> >
> > On Wed, 7 Oct 2020 at 20:45, Richard Levitte  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
> 

Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread SHANE LONTIS
I assume you are just talking about the ec key?

If the public key is not present then that could be seen as an error for 
operations that require the public key. 

Shane

> On 7 Oct 2020, at 9:54 pm, Dr Paul Dale  wrote:
> 
> Would it be feasible to change code that does ->pub_key to call a function 
> that null checks the field and generates the public key if it is absent?
> 
> 
> Pauli
> -- 
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
> Phone +61 7 3031 7217
> Oracle Australia
> 
> 
> 
> 
>> On 7 Oct 2020, at 9:29 pm, Matt Caswell > > 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
> 



Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Richard Levitte
On Wed, 07 Oct 2020 21:25:57 +0200,
Nicola Tuveri wrote:
> 
> On Wed, 7 Oct 2020 at 20:45, Richard Levitte  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/


Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Nicola Tuveri
On Wed, 7 Oct 2020 at 20:45, Richard Levitte  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!
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.

> Regarding avoiding NULL checks, that's actually a separate story,
> evem though it overlaps conveniently.  There was the idea, for a
> while, that we should avoid NULL checks everywhere (unless it's valid
> input), and indeed make it a programmer error if they happened to pass
> NULL where they shouldn't.
> One can see that as an hard assertion, and that has indeed produced
> crashes that uncovered bugs we might otherwise have missed.  The tide
> has changed though, and it seem like the fashion du jour is to check
> NULLs and error with an ERR_R_PASSED_NULL_PARAMETER.  I'm not sure
> that we've made an actual hard decision in either direction, though.
>
> However, I'm not sure where the NULL check problem comes in.
> Operations that don't use the public key parts don't need to look at
> the public key parts, so a NULL check there is simply not necessary.

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.

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.


I would add, although it's feedback for the vote, not the proposal,
that I am not opposed to changing the documented assumption in
principle, but with changing it this late in the development of 3.0.

I am giving feedback for the text proposal to ensure that during the
voting we don't underestimate the high risk of critical bugs coming
with adopting this change at this stage, and that it does call for
vastly extending our current test coverage beyond what we were
currently planning as acceptable.

We all feel quite under pressure due to the time constraints, and I
feel that in general we have privileged working on implementing the
required features (with sufficient level of tests as per our policy)
rather than spending major resources in further developing our test
coverage with exhaustive positive and negative tests for regular and
far-fetched cases, this should probably also have a weight on the
decision of committing to this change at this point.



Nicola


Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Nicola Tuveri
On Wed, 7 Oct 2020 at 20:17, Richard Levitte  wrote:
>
> There's no reason for the EVP layer to check for the presence or the
> absence or the public key, for one very simple reason: it doesn't have
> access to the backend key structure and therefore has no possibility
> to make any such checks.  Such checks should be made in the backend.

The very case that triggered this discussion happens because the
provider backend is doing the tests that libcrypto is not doing, and
upholding the core to the documented assumption.
On import/export the backend is enforcing that privkey alone is never
imported/exported without its public counterpart.


> That part of your proposed vote text that mentions checks in EVP
> doesn't make any sense with that in mind, and I fear that it would
> lead us into a rabbit hole that's not really necessary.  Also, this
> contradicts the intentions of the design for libcrypto vs providers,
> which was that libcrypto (and therefore EVP) would be a rather thin
> layer that simply passes stuff to the providers and gets results back
> (there are things that complicate this a bit, but this is essentially
> what we do), and leave it to the provider to decide what to do and how
> to do it.

Re: contradicting design principles

I would also remark here that pushing quirkiness into providers to
satisfy "weird" requirements that only serve the purpose of dealing
with the legacy promises of libcrypto (or anyway the extra constraints
enforced by libcrypto surface that is not directly facing the
providers) also contradicts designs of libcrypto vs providers:
libcrypto takes care of libcrypto user-facing and provider-facing API,
the provider only should care about the new and well defined
core-provider API.

Re: why test at EVP layer?

In my proposal the test at the EVP (and lower) layers is because that
is part of taking care of libcrypto user-facing API: the long-standing
(locally betrayed) assumption is established for `EVP_PKEY` objects,
so `EVP` is the natural layer to have a first layer of testing to
ensure that pervasively we can guarantee that the current documented
assumption can be safely disregarded because not relevant anymore.
That assumption, for which we have long-standing documentation at the
EVP layer has at some point (even in the past 4 years in my limited
experience) been enforced also on lower levels (it is not particularly
relevant if chronologically it percolated down to the lower levels or
conversely was already there all along undocumented and was put in
writing only when documenting `EVP_PKEY`): that is why I included in
my proposal to also build coverage for this change not only in EVP but
also in the other layers.



I can agree with you that some of this testing at different layers can
appear quite redundant, especially in terms of which "security
critical" or "potentially failing" code is tested in the current
snapshot of the code: but my counter argument is that if we were less
worried about redundant tests and were more rigorous in always
including both positive and negative tests for all the things we put
in our documentation, maybe we would have already had the tests that
asserted that the code reflects our documented assumption (and in
which layers) and we wouldn't have ended up violating our documented
assumption by accident creating something that works in 1.1.1 by
(partial) accident and that we are now considering as a potential
regression in 3.0.


Nicola


Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Richard Levitte
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.

Regarding avoiding NULL checks, that's actually a separate story,
evem though it overlaps conveniently.  There was the idea, for a
while, that we should avoid NULL checks everywhere (unless it's valid
input), and indeed make it a programmer error if they happened to pass
NULL where they shouldn't.
One can see that as an hard assertion, and that has indeed produced
crashes that uncovered bugs we might otherwise have missed.  The tide
has changed though, and it seem like the fashion du jour is to check
NULLs and error with an ERR_R_PASSED_NULL_PARAMETER.  I'm not sure
that we've made an actual hard decision in either direction, though.

However, I'm not sure where the NULL check problem comes in.
Operations that don't use the public key parts don't need to look at
the public key parts, so a NULL check there is simply not necessary.

Cheers,
Richard

On Wed, 07 Oct 2020 19:20:45 +0200,
Nicola Tuveri wrote:
> 
> Re; "enforcement"
> 
> My impression is that there is no such enforcement, because the
> project has (had?) a stance on "avoid NULL checks" and "consider
> things that break our documented assumptions as programmer errors".
> 
> The natural result of these two principles may very well be the
> deliberate reason why "enforcement" cannot be found and we might
> perceive documentation written in 2006 as not current.
> 
> I can say that in the past 4 years within PR I authored or
> co-authored, even before becoming a Committer, I can recall reviews
> requesting to amend the code to avoid NULL checks on the public key
> component because it is our convention that an EVP_PKEY with key
> material is either a public key or a key pair.
> 
> 
> Nicola
> 
> On Wed, 7 Oct 2020 at 19:52, Richard Levitte  wrote:
> >
> > 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 

Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Dmitry Belyavsky
On Wed, Oct 7, 2020 at 8:29 PM Kurt Roeckx  wrote:

> On Wed, Oct 07, 2020 at 12:29:10PM +0100, Matt Caswell 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 to me that we have various ways forward:
> 1) Enforce that a private key requires the public key
> 2) Don't enforce it, just give an error when you try to use the
>public key and it's not available.
> 3) Make it work for the same cases that worked with 1.1.1
> 4) Generate the public key from the private key
> 5) Have some kind of transition where we do 2), 3)
>or 4) in 3.0, but will move to 1) at some later point.
> 6) do 2), but enforce it in the fips provider
>
> I don't know if we do any any kind of consistency checks on the key
> now when it's loaded. But 2) would then imply that the check is
> skipped instead of returning an error.
>

4) maybe not applicable when a private key is on the hardware token.

-- 
SY, Dmitry Belyavsky


Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Kurt Roeckx
On Wed, Oct 07, 2020 at 12:29:10PM +0100, Matt Caswell 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 to me that we have various ways forward:
1) Enforce that a private key requires the public key
2) Don't enforce it, just give an error when you try to use the
   public key and it's not available.
3) Make it work for the same cases that worked with 1.1.1
4) Generate the public key from the private key
5) Have some kind of transition where we do 2), 3)
   or 4) in 3.0, but will move to 1) at some later point.
6) do 2), but enforce it in the fips provider

I don't know if we do any any kind of consistency checks on the key
now when it's loaded. But 2) would then imply that the check is
skipped instead of returning an error.


Kurt



Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Nicola Tuveri
Re; "enforcement"

My impression is that there is no such enforcement, because the
project has (had?) a stance on "avoid NULL checks" and "consider
things that break our documented assumptions as programmer errors".

The natural result of these two principles may very well be the
deliberate reason why "enforcement" cannot be found and we might
perceive documentation written in 2006 as not current.

I can say that in the past 4 years within PR I authored or
co-authored, even before becoming a Committer, I can recall reviews
requesting to amend the code to avoid NULL checks on the public key
component because it is our convention that an EVP_PKEY with key
material is either a public key or a key pair.


Nicola

On Wed, 7 Oct 2020 at 19:52, Richard Levitte  wrote:
>
> 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 

Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Richard Levitte
There's no reason for the EVP layer to check for the presence or the
absence or the public key, for one very simple reason: it doesn't have
access to the backend key structure and therefore has no possibility
to make any such checks.  Such checks should be made in the backend.

That part of your proposed vote text that mentions checks in EVP
doesn't make any sense with that in mind, and I fear that it would
lead us into a rabbit hole that's not really necessary.  Also, this
contradicts the intentions of the design for libcrypto vs providers,
which was that libcrypto (and therefore EVP) would be a rather thin
layer that simply passes stuff to the providers and gets results back
(there are things that complicate this a bit, but this is essentially
what we do), and leave it to the provider to decide what to do and how
to do it.

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  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 

Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Richard Levitte
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 

Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Matt Caswell
I would also be ok with this vote text formulation. Lets see what others
say.

Matt


On 07/10/2020 16:34, 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  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
> 


Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Nicola Tuveri
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  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


Re: Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Dr Paul Dale
Would it be feasible to change code that does ->pub_key to call a function that 
null checks the field and generates the public key if it is absent?


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On 7 Oct 2020, at 9:29 pm, Matt Caswell  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



Vote proposal: Private keys can exist independently of public keys

2020-10-07 Thread Matt Caswell
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