Re: [openssl-project] Removal of NULL checks

2018-08-09 Thread Viktor Dukhovni
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

2018-08-09 Thread Paul Dale
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

2018-08-09 Thread Viktor Dukhovni
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

2018-08-09 Thread Richard Levitte
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

2018-08-09 Thread Richard Levitte
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

2018-08-09 Thread Viktor Dukhovni
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

2018-08-09 Thread Richard Levitte
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

2018-08-09 Thread Salz, Rich
>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

2018-08-09 Thread Viktor Dukhovni



> 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] Removal of NULL checks

2018-08-09 Thread Salz, Rich
>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

2018-08-09 Thread Andy Polyakov
> 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] Removal of NULL checks

2018-08-08 Thread Paul Dale
I'm firmly in the don't remove them camp too.


Pauli
-- 
Oracle
Dr Paul Dale | Cryptographer | Network Security & Encryption 
Phone +61 7 3031 7217
Oracle Australia


-Original Message-
From: Viktor Dukhovni [mailto:openssl-us...@dukhovni.org] 
Sent: Wednesday, 8 August 2018 11:52 PM
To: openssl-project@openssl.org
Subject: Re: [openssl-project] Removal of NULL checks



> On Aug 8, 2018, at 6:19 AM, Tim Hudson  wrote:
> 
> However in the context of removing such checks - that we should not be 
> doing - the behaviour of the APIs in this area should not be changed

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.

-- 
Viktor.

___
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

2018-08-08 Thread Kurt Roeckx
On Wed, Aug 08, 2018 at 08:19:24PM +1000, Tim Hudson wrote:
> We don't have a formal policy of no NULL checks - we just have a few
> members that think we should have such a policy but it has never been voted
> on as we had sufficiently varying views for a consensus approach to not be
> possible.
> 
> Personally I'm in favour of high-level APIs having NULL checks as it (in my
> experience) leads to less bogus error crash reports from users and simpler
> handling in error cleanup logic.
> Otherwise you end up writing a whole pile of if(x!=NULL) FOO_free(x); etc

I think at least Rich would not add checks for NULL in functions
that don't expect to be called with NULL, but on the other hand
moves the NULL check into the free() calls. But in that case, we
also clearly document that it can be called with NULL.

So it's at least not general that there shouldn't be a NULL check.


Kurt

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project


Re: [openssl-project] Removal of NULL checks

2018-08-08 Thread Viktor Dukhovni



> On Aug 8, 2018, at 6:19 AM, Tim Hudson  wrote:
> 
> However in the context of removing such checks - that we should not be doing 
> - the behaviour of the APIs in this area should not be changed

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.

-- 
Viktor.

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project


Re: [openssl-project] Removal of NULL checks

2018-08-08 Thread Tim Hudson
We don't have a formal policy of no NULL checks - we just have a few
members that think we should have such a policy but it has never been voted
on as we had sufficiently varying views for a consensus approach to not be
possible.

Personally I'm in favour of high-level APIs having NULL checks as it (in my
experience) leads to less bogus error crash reports from users and simpler
handling in error cleanup logic.
Otherwise you end up writing a whole pile of if(x!=NULL) FOO_free(x); etc

But it is a style issue.

However in the context of removing such checks - that we should *not* be
doing - the behaviour of the APIs in this area should not be changed
outside of a major version increment - and even then I would not make the
change either because it introduces changes which in themselves serve no
real purpose - i.e. a style change can result in making a program that used
to work fine crash in bad ways - and that we shouldn't be doing even across
major version increments in my view.

Tim.


On Wed, Aug 8, 2018 at 8:08 PM, Matt Caswell  wrote:

> We've had a policy for a while of not requiring NULL checks in
> functions. However there is a difference between not adding them for new
> functions and actively removing them for old ones.
>
> See https://github.com/openssl/openssl/pull/6893
>
> In this case the removal of a NULL check in the stack code had the
> unintended consequence of a crash in a no-comp build. Is it wise to be
> actively removing existing NULL checks like this? It does have an impact
> on the behaviour of a function (even if that behaviour is undocumented
> and not encouraged). The concern I have is for our API/ABI compatibility
> guarantee. If we make changes like this then 1.1.1 may no longer be a
> drop in replacement for 1.1.0 for some apps.
>
> Matt
>
> ___
> 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

[openssl-project] Removal of NULL checks

2018-08-08 Thread Matt Caswell
We've had a policy for a while of not requiring NULL checks in
functions. However there is a difference between not adding them for new
functions and actively removing them for old ones.

See https://github.com/openssl/openssl/pull/6893

In this case the removal of a NULL check in the stack code had the
unintended consequence of a crash in a no-comp build. Is it wise to be
actively removing existing NULL checks like this? It does have an impact
on the behaviour of a function (even if that behaviour is undocumented
and not encouraged). The concern I have is for our API/ABI compatibility
guarantee. If we make changes like this then 1.1.1 may no longer be a
drop in replacement for 1.1.0 for some apps.

Matt

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project