Re: [openssl-project] Removal of NULL checks
On Thu, Aug 09, 2018 at 02:23:07PM -0700, Paul Dale wrote: > > Real code often doesn't check return values. Even ours. :( > > Could we consider adding a lot more __owur tags to functions to encourage > this? > > As an API change it would have to wait for a major release. This is sometimes a good idea, for sufficiently important functions. This sort of change generates compiler warnings, and is easily addressed without breaking compatibility with older library versions. We should not overuse __owur in marginal cases. -- Viktor. ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
Rich wrote: > Real code often doesn't check return values. Even ours. :( Could we consider adding a lot more __owur tags to functions to encourage this? As an API change it would have to wait for a major release. Pauli -- Oracle Dr Paul Dale | Cryptographer | Network Security & Encryption Phone +61 7 3031 7217 Oracle Australia -Original Message- From: Salz, Rich [mailto:rs...@akamai.com] Sent: Thursday, 9 August 2018 11:49 PM To: openssl-project@openssl.org Subject: Re: [openssl-project] Removal of NULL checks >it's NULL. Now imagine that this stack was some kind of deny list. If >entry producer didn't pay attention to error when creating stack and >putting things into it, consumer would be led to belief that there is >nothing to deny and let through the requests that are supposed to be >denied. This is another reason why I am opposed to NULL checks. >Oh! Triggering factor was quite a number of unchecked pushes in apps. Real code often doesn't check return values. Even ours. :( ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
On Thu, Aug 09, 2018 at 07:12:18PM +0200, Richard Levitte wrote:
> viktor> X509 *x;
> viktor> STACK_OF(X509) *s;
> viktor>
> viktor> ...
> viktor> /* Allocate 's' and initialize with x as first element */
> viktor> if (sk_X509_push(s = sk_X509_new(NULL), x) < 0) {
> viktor> /* error */
> viktor> }
>
> I would regard that code incorrectly written, because it doesn't check
> the value returned from sk_X509_new(NULL) (i.e. it doesn't properly
> check for possible errors). Correctly written code would be written
> like this:
It is correctly written *given* the existing NULL checks, and the
fact that our API is under-documented.
> However, if we actually want people to be able not to check if the
> stack they wanted to allocate actually got allocated, the correct
> course of action would be to make that a defined behaviour, i.e. fix
> the docs accordingly.
Yes, we should document the existing behaviour in preference to
changing it. Changing the behaviour of existing functions should
require a compelling reason to do that.
--
Viktor.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
In message <20180809165255.gg14...@straasha.imrryr.org> on Thu, 9 Aug 2018
12:52:56 -0400, Viktor Dukhovni said:
viktor> On Thu, Aug 09, 2018 at 06:40:14PM +0200, Richard Levitte wrote:
viktor> > In message <20180809162245.gd14...@straasha.imrryr.org> on Thu, 9 Aug
2018 12:22:45 -0400, Viktor Dukhovni said:
viktor> >
viktor> > openssl-users> It needs to be possible to recompile and run without
auditing code.
viktor> > openssl-users> The worst kind of incompatibilities are those that are
not reported
viktor> > openssl-users> by the compiler, and are only found at runtime,
possibly under unusual
viktor> > openssl-users> conditions.
viktor> >
viktor> > So in this particular case, such as unchecked calls of sk_ functions,
viktor> > including sk_TYPE_new(), just to discover later that "oops, the
viktor> > elements we thought we inserted aren't there"? ;-)
viktor>
viktor> The NULL checks were returning an error, not silently failing to
viktor> add the element.
I said *unchecked* calls of sk_ functions.
viktor> > Either way, sk == NULL will not be reported by the compiler, will only
viktor> > be found at runtime, possibly under unusual conditions.
viktor>
viktor> Code that tested for the error and avoided a crash would absent
viktor> NULL checks crash (in unexpected cases). The existing code should
viktor> be assumed to be correctly written for the current library behaviour.
viktor> What happens to already broken code is of little interest.
Code that is correctly written should check the value returned from
sk_TYPE_new(), no?
viktor> > The only
viktor> > difference is exactly how the user gets to find out in runtime; 1)
viktor> > mysterious failures because the stack that should contain n elements
viktor> > is really empty and unfillable, or 2) an immediate crash.
viktor>
viktor> This is simply wrong I'm afraid. The NULL checks in question
viktor> returned an *error* (rather than crash) that the application should
viktor> check. Applications that are not doing their own NULL check and
viktor> expect us to do it for them, can check for return value. This
viktor> makes possible more concise code:
viktor>
viktor> X509 *x;
viktor> STACK_OF(X509) *s;
viktor>
viktor> ...
viktor> /* Allocate 's' and initialize with x as first element */
viktor> if (sk_X509_push(s = sk_X509_new(NULL), x) < 0) {
viktor> /* error */
viktor> }
I would regard that code incorrectly written, because it doesn't check
the value returned from sk_X509_new(NULL) (i.e. it doesn't properly
check for possible errors). Correctly written code would be written
like this:
X509 *x;
STACK_OF(X509) *s;
...
/* Allocate 's' and initialize with x as first element */
if ((s = sk_X509_new(NULL)) == NULL
|| sk_X509_push(s, x) < 0) {
/* error */
}
However, if we actually want people to be able not to check if the
stack they wanted to allocate actually got allocated, the correct
course of action would be to make that a defined behaviour, i.e. fix
the docs accordingly.
I find it weird, however, that in this particular case, we would
expect users not to check if a stack was actually allocated, when such
checks is actually correct behaviour, for example on stuff returned by
OPENSSL_malloc(), EVP_PKEY_CTX_new() etc etc etc.
Cheers,
Richard
--
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
On Thu, Aug 09, 2018 at 06:40:14PM +0200, Richard Levitte wrote:
> In message <20180809162245.gd14...@straasha.imrryr.org> on Thu, 9 Aug 2018
> 12:22:45 -0400, Viktor Dukhovni said:
>
> openssl-users> It needs to be possible to recompile and run without auditing
> code.
> openssl-users> The worst kind of incompatibilities are those that are not
> reported
> openssl-users> by the compiler, and are only found at runtime, possibly under
> unusual
> openssl-users> conditions.
>
> So in this particular case, such as unchecked calls of sk_ functions,
> including sk_TYPE_new(), just to discover later that "oops, the
> elements we thought we inserted aren't there"? ;-)
The NULL checks were returning an error, not silently failing to
add the element.
> Either way, sk == NULL will not be reported by the compiler, will only
> be found at runtime, possibly under unusual conditions.
Code that tested for the error and avoided a crash would absent
NULL checks crash (in unexpected cases). The existing code should
be assumed to be correctly written for the current library behaviour.
What happens to already broken code is of little interest.
> The only
> difference is exactly how the user gets to find out in runtime; 1)
> mysterious failures because the stack that should contain n elements
> is really empty and unfillable, or 2) an immediate crash.
This is simply wrong I'm afraid. The NULL checks in question
returned an *error* (rather than crash) that the application should
check. Applications that are not doing their own NULL check and
expect us to do it for them, can check for return value. This
makes possible more concise code:
X509 *x;
STACK_OF(X509) *s;
...
/* Allocate 's' and initialize with x as first element */
if (sk_X509_push(s = sk_X509_new(NULL), x) < 0) {
/* error */
}
/* s is stack holding x */
...
--
Viktor.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
In message <20180809162245.gd14...@straasha.imrryr.org> on Thu, 9 Aug 2018 12:22:45 -0400, Viktor Dukhovni said: openssl-users> It needs to be possible to recompile and run without auditing code. openssl-users> The worst kind of incompatibilities are those that are not reported openssl-users> by the compiler, and are only found at runtime, possibly under unusual openssl-users> conditions. So in this particular case, such as unchecked calls of sk_ functions, including sk_TYPE_new(), just to discover later that "oops, the elements we thought we inserted aren't there"? ;-) Either way, sk == NULL will not be reported by the compiler, will only be found at runtime, possibly under unusual conditions. The only difference is exactly how the user gets to find out in runtime; 1) mysterious failures because the stack that should contain n elements is really empty and unfillable, or 2) an immediate crash. Either way, the application authors will have to learn to check their stack pointers. The real difference is how much they will have to scratch their heads to figure out what went wrong. Cheers, Richard -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/ ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
On Thu, Aug 09, 2018 at 05:53:11PM +0200, Richard Levitte wrote: > I think we need to be a bit more nuanced in our views. Bug fixes are > potentially behaviour changes (for example, I recently got through a > PR that added a stricter check of EVP_PKEY_asn1_new() input; see #6880 > (*)). We went too far too quickly in the transition from 1.0.2 to 1.1.0, e.g. by needlessly renaming some functions without providing the legacy names (even as deprecated aliases) and by not adding to 1.0.2 the new accessors required for compatibility with 1.1.0. Mostly that could have been done (and could still be done) via new macros in headers that add 1.1.0 accessors to 1.0.2: #if OPENSSL_API_COMPAT >= 0x1010UL #define EVP_MD_CTX_new() EVP_MD_CTX_create() ... #endif As a result many applications that need to support both 1.0.2 and 1.1.0 (whichever is available) had to waste effort to create the requisite #ifdefs, wrapper functions, ... If we keep doing that, everyone will be using LibreSSL or another alternative. We must not casually change APIs. Especially because of our documentation deficit, which results in users learning about our interfaces via experimentation or reading the source. If we must change an interface, and *can* do it by introducing a new function (that we adequately document), that must be the way forward. And *furthermore*, we can't remove the deprecated interface until the new function has been in multiple stable releases. Indeed to promote adoption, such new functions (when simple enough) should be considered for inclusion in the extant stable releases, making it easy to migrate from old to new. > "But this is how it has worked so far!" Yeah? Still undefined behaviour. Blaming the user for changes in undefined behaviour does not get us more happy users. > I think we're doing ourselves a disservice if we get too stuck by > behaviour that can only be reasonably derived by reading the source > code rather than the docs. I think we're doing our users and ourselves a disservice if we're too casual about API changes. We can only get away with major incompatibilities like those between 1.0.2 and 1.1.0 once. If we keep doing that, we'll lose the application base. > So there's a choice, and if we accept that NULL is valid input the the > safestack functions, we should document it. If not, then sk == NULL > is still mostly undefined, and crashes are therefore as expected as > anything else. If the functions previously returned an error, they must continue to do that barring overwhelming reasons to make a change. > However, caution isn't a bad thing. I think that as part of a minor > version upgrade, removing existing NULL checks may be a bit rad. > However, I'd say that for the next major version, we're free to change > an undefined behaviour to something more well defined, as we > see fit. No, we need a greater emphasis on backwards compatibility, and introduce API changes more slowly, over multiple releases that carry old and new APIs, and we must not change the behaviour of existing functions without renaming them, except when the current behaviour is clearly a bug. It needs to be possible to recompile and run without auditing code. The worst kind of incompatibilities are those that are not reported by the compiler, and are only found at runtime, possibly under unusual conditions. -- Viktor. ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
In message <8acbebbf-f4a2-469f-bb5b-3564a24dc...@dukhovni.org> on Thu, 9 Aug 2018 11:02:16 -0400, Viktor Dukhovni said: openssl-users> openssl-users> openssl-users> > On Aug 9, 2018, at 9:49 AM, Salz, Rich wrote: openssl-users> > openssl-users> > This is another reason why I am opposed to NULL checks. openssl-users> openssl-users> Whether one's for them, or against them, removing a check openssl-users> from a function that would formerly return an error and openssl-users> making it crash is a substantial API change. We must openssl-users> avoid API changes whenever we can. We can introduce openssl-users> new functions and gradually deprecate the old over openssl-users> a number (at least 3 IMHO) major release cycles, but openssl-users> what we MUST NOT do is just change the API of an openssl-users> existing function. I think we need to be a bit more nuanced in our views. Bug fixes are potentially behaviour changes (for example, I recently got through a PR that added a stricter check of EVP_PKEY_asn1_new() input; see #6880 (*)). Also, we might want to look at our own documentation to see what can be reasonably expected. doc/man3/DEFINE_STACK_OF.pod defines all the stack functions. Just scanning for NULL shows that sk == NULL is defined for sk_TYPE_num() and sk_TYPE_set(). That possibility isn't mentioned for any other of the safestack functions, and the behaviour could therefore be seen as undefined. "But this is how it has worked so far!" Yeah? Still undefined behaviour. I think we're doing ourselves a disservice if we get too stuck by behaviour that can only be reasonably derived by reading the source code rather than the docs. So there's a choice, and if we accept that NULL is valid input the the safestack functions, we should document it. If not, then sk == NULL is still mostly undefined, and crashes are therefore as expected as anything else. However, caution isn't a bad thing. I think that as part of a minor version upgrade, removing existing NULL checks may be a bit rad. However, I'd say that for the next major version, we're free to change an undefined behaviour to something more well defined, as we see fit. Cheers, Richard -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/ ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
>Whether one's for them, or against them, removing a check from a function that would formerly return an error and making it crash is a substantial API change. I don't disagree. ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
> On Aug 9, 2018, at 9:49 AM, Salz, Rich wrote: > > This is another reason why I am opposed to NULL checks. Whether one's for them, or against them, removing a check from a function that would formerly return an error and making it crash is a substantial API change. We must avoid API changes whenever we can. We can introduce new functions and gradually deprecate the old over a number (at least 3 IMHO) major release cycles, but what we MUST NOT do is just change the API of an existing function. -- Viktor. ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Reuse of PSKs between TLSv1.2 and TLSv1.3
>"We should remove the TLSv1.2 to TLSv1.3 PSK compatibility mechanism as discussed in issue 6490. If TLSv1.2 PSKs are configured (and not TLSv1.3 PSKs) then we should disable TLSv1.3." This seems reasonable to me, albeit a bit wordy since you could delete the first sentence. :) ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
>it's NULL. Now imagine that this stack was some kind of deny list. If >entry producer didn't pay attention to error when creating stack and >putting things into it, consumer would be led to belief that there is >nothing to deny and let through the requests that are supposed to be >denied. This is another reason why I am opposed to NULL checks. >Oh! Triggering factor was quite a number of unchecked pushes in apps. Real code often doesn't check return values. Even ours. :( ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Removal of NULL checks
> Should not be changed period. Even across major release boundaries. > This is not an ABI compatibility issue, it is a source compatibility > issue, and should avoided all the time. If we want to write a *new* > function that skips the NULL checks it gets a new name. While I admittedly crossed a line, I would actually argue against drawing the line as categorically as above. In sense that NULL checks *can* be excessive, and thing is that such checks can jeopardize application security. And in such case it wouldn't be totally inappropriate to remove them, excessive checks, *without* renaming function as suggested above. It would be kind of equivalent of changing one undefined behaviour pattern for another undefined one. Or rather for more "honest" undefined behaviour pattern:-) As for jeopardizing application security, taking this case as an example. What worked so far? Create stack without paying attention to result[!], dump things into stack even if it's NULL, pull nothing if it's NULL. Now imagine that this stack was some kind of deny list. If entry producer didn't pay attention to error when creating stack and putting things into it, consumer would be led to belief that there is nothing to deny and let through the requests that are supposed to be denied. This is kind of not-quite-right example in the context, because it implies that *all* NULL checks should have been removed, thus making *every* caller to pay attention, including consumer. While I left checks in some of the consume operations reckoning that at least those who will be putting things into NULL stack will have to pay attention and will cancel whole operation hopefully without getting to pull-nothing step. So that those entry producers who are not *currently* paying attention would actually be caught. Which actually would be a positive thing! Oh! Triggering factor was quite a number of unchecked pushes in apps. Yes, you can sense contradiction in sense that one can't "fix" those unchecked pushes with removal of NULL checks in questions. But they were just something that made me look at stack.c and wonder above questions. ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
Re: [openssl-project] Reuse of PSKs between TLSv1.2 and TLSv1.3
On 08/08/18 11:28, Matt Caswell wrote: > For the full background to this issue see: > > https://github.com/openssl/openssl/issues/6490 > > TL;DR summary: > > The TLSv1.2 and TLSv1.3 PSK mechanisms are quite different to each > other. OpenSSL (along with at least GnuTLS maybe others) has implemented > an upgrade path which enables the reuse of a TLSv1.2 PSK in TLSv1.3. > This is not prohibited by the spec. David Benjamin has raised concerns > about this due to key separation. Everything else in TLSv1.3 is provably > secure - but this is not. The spec has been updated to add some words of > warning about this. > > > There seems to be two schools of thought on what to do about this: > > 1) We should seek to avoid this risk. As a fix we should disable TLSv1.3 > if TLSv1.2 PSKs have been configured. We expect that at some later time > the IETF will come up with a better answer and when that happens we can > implement it then. A PR to do the removal is here: > https://github.com/openssl/openssl/pull/6836 > > 2) This is a theoretical risk - there might not actually be a problem at > all, its just that we can't prove it. OTOH not upgrading to TLSv1.3 is > definitely a bad thing, so we should just leave things as they are and > accept the theoretical risk. > > > I'll admit that I've been flip-flopping between the two approaches to > this and there doesn't seem to be a clear consensus forming. How should > we take this forward? Does it require an OMC vote? Ok...no discussion... I think perhaps a vote is the only way forward then. Does this vote text seem reasonable? "We should remove the TLSv1.2 to TLSv1.3 PSK compatibility mechanism as discussed in issue 6490. If TLSv1.2 PSKs are configured (and not TLSv1.3 PSKs) then we should disable TLSv1.3." If the vote fails then we will, by default, stick with the status quo (which is effectively option 2). If it passes then that is option 1. Matt ___ openssl-project mailing list openssl-project@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-project
