Re: Asserts considered harmful (or GMP spills its sensitive information)

[Niels Möller]

t...@gmplib.org (Torbjörn Granlund) writes:

> Let's move on.  No bug to be found here.

Just FYI: There was a bug in Nettle's test code, a line

  assert (mpz_invert(key->d, pub->e, phi));

Obviously not working with -DNDEBUG. Fix in commit
https://git.lysator.liu.se/nettle/nettle/commit/73d3c6d5586cc0fd81eab081078144d621de07b4

Regards,
/Niels

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Marco Bodrato]

Ciao,

I'm not sending to oss-security... They does not seem to be interested.

Il Gio, 3 Gennaio 2019 9:42 pm, Jeffrey Walton ha scritto:
> On Thu, Jan 3, 2019 at 2:55 PM Marco Bodrato 
> wrote:

>> This absolutely is NOT a "small example", it requires to build two
>> entire libraries!

> Well, if you can let us know how to reduce it further then we would be
> delighted to hear it.

I did, and I sent my analysis elsewhere.

Unfortunately, the topic here is not "delighting users" :-D

It is GMP bugs! And since your not "small" example does not show a GMP bug
(a behaviour of the library in contrast with the one expected reading
documentation), it would be off topic here.

> I thought it did a good job because it did not muck with your system,

You fired a bug for the wrong library...
The job could be done better, don't you agree? :-D

>> Can we suggest you to read the GMP manual on how to build the library?
>> GMP works fine on many ARM configurations we test and there are lots of

> Here's what I witness on a BananaPi and a couple of other boards. Can
[...]
> bananapi:~$ ./test-gmp.sh

What's "./test-gmp.sh"? It is not a script we provide. If that script does
not work, please report the failure to the author of that script. :-)

I'd suggest:
$ ./configure && make && make check

Then please read https://gmplib.org/manual/Installing-GMP.html if you need
more options.

>> On GMP side, we can only specify even more explicitly in the
>> documentation of that function the need for non-zero sized arguments.

> Returning a failure from mpn_sec_powm would be a most welcomed
> improvement.

Functions in the mpn_ layer are low-level functions. If a developer decide
to use those functions, she/he have the responsibility to correctly use
them.
Otherwise, the developers can decide to use the mpz_ layer or even more
complex wrappers.

Wish-lists of "welcomed improvements" is off topic on this list.

On GMP side, the bug report is closed.


Ĝis,
m

-- 
http://bodrato.it/papers/

___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Torbjörn Granlund]

Jeffrey Walton  writes:

  Here's what I witness on a BananaPi and a couple of other boards. Can
  you provide info on the ARM boards you are using? I have about 8 of
  them for testing, and I may be able to duplicate your [successful]
  result.

Marco and others have told you to read the GMP manual.  People have
explained what you do wrong and it is clear that you know very well why
your CFLAGS messing breaks things.  Yet, you insist on spreading the lie
that GMP "does not build".

  Returning a failure from mpn_sec_powm would be a most welcomed
  improvement.

You have repeated this several times already.

The GMP API is what it is.  If you don't like it, well, we're so sorry.

  It would be a welcomed improvement if GMP does it in
  other places, too. Crashing is least welcomed behavior for many uses
  cases, including those where availability and confidentiality is a
  concern.

You have repeated this several times, and people have patiently replied
and explained how to handle this safely.

  Gracefully handling failure serves several purposes. First, returning
  failure is what developers expect to happen.

Really?  Did you talk to them?

  If a program uses a function incorrectly then it is expected to
  fail. Developers are usually good about checking return values at call
  sites.

I have yet to find one program which checks all return values.

  Second, when GMP crashes it is setting a policy for the application.

Any API sets policies.

We've had enough of your nagging and aggressiveness and your threats in
private email.  Your messages to the GMP lists will henceforth be
automatically discarded.

-- 
Torbjörn
Please encrypt, key id 0xC8601622
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Jeffrey Walton]

Thanks Marco. Comments inline.

On Thu, Jan 3, 2019 at 2:55 PM Marco Bodrato  wrote:
>
> Il Lun, 31 Dicembre 2018 7:03 pm, Jeffrey Walton ha scritto:
> [...skipping opinions...]
>
> > Here's a small example of triggering an assert using the Nettle
> > library.
>
> This absolutely is NOT a "small example", it requires to build two entire
> libraries!

Well, if you can let us know how to reduce it further then we would be
delighted to hear it.

I thought it did a good job because it did not muck with your system,
and it used independent data provided by someone familiar with the
library. That is, I did not craft a sneaky test case to make a point.
And a 'rm -rf /tmp/gmp-test' is all that's needed to remove it.

> > ARM A-32 does not work at the moment due to GMP build errors.
>
> Can we suggest you to read the GMP manual on how to build the library?
> GMP works fine on many ARM configurations we test and there are lots of
> projects out there (eg. many GNU/Linux distributions) that builds GMP for
> different ARM processors.

Here's what I witness on a BananaPi and a couple of other boards. Can
you provide info on the ARM boards you are using? I have about 8 of
them for testing, and I may be able to duplicate your [successful]
result.

bananapi:~$ ./test-gmp.sh
--2019-01-03 15:07:11--  https://ftp.gnu.org/gnu/gmp/gmp-6.1.2.tar.bz2
...

gcc -std=gnu99 -c -DHAVE_CONFIG_H -I. -I.. -D__GMP_WITHIN_GMP -I..
-DOPERATION_lshift -I/tmp/gmp-test/include -DNDEBUG -g2 -O2
-march=native -fPIC -Wa,--noexecstack tmp-lshift.s -fPIC -DPIC -o
.libs/lshift.o
tmp-lshift.s: Assembler messages:
tmp-lshift.s:106: Error: selected processor does not support ARM mode
`vdup.32 d6,r3'
tmp-lshift.s:108: Error: selected processor does not support ARM mode
`vdup.32 d7,r3'
tmp-lshift.s:114: Error: selected processor does not support ARM mode
`vshl.u64 d18,d19,d7'
tmp-lshift.s:120: Error: selected processor does not support ARM mode
`vshl.u64 d4,d19,d6'
...

> > In the case below Nettle is using benign data and not maliciously
> > crafted data.
>
> I'm sorry, but your analysis was incorrect.
>
> I agree, Nettle is not using "maliciously crafted data", but I do not
> agree when you say that it "is using benign data".
>
> With your build options, Nettle calls the GMP function mpn_sec_powm with
> an invalid parameter: ebn = 0.
>
> Because of an error in the Nettle library you built, GMP receives "non
> benign data". To avoid further memory corruptions, GMP aborts.
>
> Thanks to this behaviour of GMP, you was able to catch the incorrect built
> of the library using it. ;-)
>
> Using mpn_sec_powm with an exponent of zero bits is obviously a nonsense,
> and in general the documentation of GMP clearly says that arguments of
> size zero are not supported.
>
> On GMP side, we can only specify even more explicitly in the documentation
> of that function the need for non-zero sized arguments.

Returning a failure from mpn_sec_powm would be a most welcomed
improvement. It would be a welcomed improvement if GMP does it in
other places, too. Crashing is least welcomed behavior for many uses
cases, including those where availability and confidentiality is a
concern.

Gracefully handling failure serves several purposes. First, returning
failure is what developers expect to happen. If a program uses a
function incorrectly then it is expected to fail. Developers are
usually good about checking return values at call sites.

Second, when GMP crashes it is setting a policy for the application.
This is ass-backwards - the application sets its own policies, not
libraries. Only the application knows the requirements to  dictate
runtime behaviors.

Related, even GMP calling exit(-1) rather than abort() is GMP setting
policies. GMP does not know what the policies and requirements are, so
it is not in a position to dictate behavior.

Third, it improves Availability in CIA. A crashed service or app does
not service requests, so there is no availability.

Fourth, it ensures Confidentiality in CIA. A core dump with sensitive
information leaks information and violates security policies. Crashing
results in sensitive information leave's the app's security boundary,
is written to the file system and is sent to platform provider .

Finally, both returning a failure, exiting, and crashing preserves
Integrity in CIA. However, there are too many tangential problems with
exiting or crashing.

Earlier I said, "A core dump with sensitive information leaks
information and violates security policies". I've worked in US DoD, US
Financial and US gov on security architecture teams. I've read a lot
of security policies and helped write a few. I can unequivocally say
no organization would allow sensitive information to leave the
security boundary without proper controls, including crash dumps
(crash dumps are just another egress point or dataflow). That
application or library would be rejected and sent into risk
acceptance.

And for completeness, I personally adore asserts. I use them 

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Marco Bodrato]

Ciao,

Il Lun, 31 Dicembre 2018 7:03 pm, Jeffrey Walton ha scritto:
[...skipping opinions...]

> Here's a small example of triggering an assert using the Nettle
> library.

This absolutely is NOT a "small example", it requires to build two entire
libraries!
Anyway we analysed it, see below.

> ARM A-32 does not work at the moment due to GMP build errors.

Can we suggest you to read the GMP manual on how to build the library?
GMP works fine on many ARM configurations we test and there are lots of
projects out there (eg. many GNU/Linux distributions) that builds GMP for
different ARM processors.

> In the case below Nettle is using benign data and not maliciously
> crafted data.

I'm sorry, but your analysis was incorrect.

I agree, Nettle is not using "maliciously crafted data", but I do not
agree when you say that it "is using benign data".

With your build options, Nettle calls the GMP function mpn_sec_powm with
an invalid parameter: ebn = 0.

Because of an error in the Nettle library you built, GMP receives "non
benign data". To avoid further memory corruptions, GMP aborts.

Thanks to this behaviour of GMP, you was able to catch the incorrect built
of the library using it. ;-)

Using mpn_sec_powm with an exponent of zero bits is obviously a nonsense,
and in general the documentation of GMP clearly says that arguments of
size zero are not supported.

On GMP side, we can only specify even more explicitly in the documentation
of that function the need for non-zero sized arguments.

Ĝis,
m

-- 
http://bodrato.it/papers/

___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Torbjörn Granlund]

  The assert that Jeffrey has hit is in sec_powm.c, 

ASSERT_ALWAYS (enb >= windowsize);

  As far as I can see, "enb" is the input argument to the win_size function,
  and "windowsize" is the return value. I'm waiting for more information,
  since it works fine in my build. Possible explanations I see are

A reasonable assumption is that this user has modified the sources to
cause this bug.  The motive would be to support his auxesis about how
insecure GMP is.

Let's move on.  No bug to be found here.

-- 
Torbjörn
Please encrypt, key id 0xC8601622
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Niels Möller]

Vincent Lefevre  writes:

> If you
> don't like that, you can write a wrapper library that will sanitize
> all the inputs and implement error processing (e.g. where the return
> value contains an error code and the result, if any), and call this
> library instead of GMP.

Regarding invalid inputs, in the GMP sources, validity checks on
function inputs generally use the ASSERT macro, which is disabled by
default. Non-assert validity checks with a return value are used only
when the check is non-trivial, e.g., for the mpz_invert function which
requires arguments to be co-prime. All easy validity checks (null
pointers, divide by zero, and the like) are left as the responsibility
of the application.

In a few places, GMP sources use ASSERT_ALWAYS. This is for internal
consistency checks, or when deveolopers believe a condition is
arithmetically impossible, but really would like to get a bug report if
that belief turns out to be wrong.

The assert that Jeffrey has hit is in sec_powm.c, 

  ASSERT_ALWAYS (enb >= windowsize);

As far as I can see, "enb" is the input argument to the win_size function,
and "windowsize" is the return value. I'm waiting for more information,
since it works fine in my build. Possible explanations I see are

1. Invalid configuration of POWM_SEC_TABLE (used by the win_size function).

2. Some general memory-overwrite problem, due to too small scratch
   space or something like that.

I interpret this ASSERT_ALWAYS as a way to check that POWM_SEC_TABLE is
sane.

Regards,
/Niels

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Vincent Lefevre]

On 2018-12-31 14:38:17 -0500, Jeffrey Walton wrote:
> On Mon, Dec 31, 2018 at 2:16 PM Vincent Lefevre  wrote:
> >
> > On 2018-12-31 13:03:27 -0500, Jeffrey Walton wrote:
> > > The GMP library uses asserts to crash a program at runtime when
> > > presented with data it did not expect. The library also ignores user
> > > requests to remove asserts using Posix's -DNDEBUG. Posix asserts are a
> > > deugging aide intended for developement, and using them in production
> > > software ranges from questionable to insecure.
> >
> > That's much better than letting the program run erratically, with
> > possible memory corruption and/or sensitive information leakage
> > to unauthorized users. You'd better fix bugs in your program.
> 
> To play devil's advocate for this particular example, GMP could have
> validated the parameters and refused to process the data. That is, the
> function could have returned failure and avoided the potential
> information leak.

Unfortunately, this is not always possible, while keeping the original
interface. Moreover, changing the interface can make the library
slower, which could be an issue for GMP (the goal is to be as fast
as possible, just like the C language was designed, where contrary
to other languages, there's the notion of undefined behavior). If you
don't like that, you can write a wrapper library that will sanitize
all the inputs and implement error processing (e.g. where the return
value contains an error code and the result, if any), and call this
library instead of GMP.

Said that, developers who forget to check whether they correctly
follow the API conditions also forget to check failures. Thus this
ends up with a similar issue (a crash).

Moreover, some asserts may come from the detection of an inconsistent
state. In this case, it is better to abort. Otherwise letting the
program continue may have worse consequences.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Niels Möller]

Jeffrey Walton  writes:

> The GMP library uses asserts to crash a program at runtime when
> presented with data it did not expect. The library also ignores user
> requests to remove asserts using Posix's -DNDEBUG. Posix asserts are a
> deugging aide intended for developement, and using them in production
> software ranges from questionable to insecure.

Crashing in a controlled fashion may also be *more* secure that
continuing execution with undefined results. Depending on circumstances,
of course.

I read the general statement "asserts considered harmful" as your
personal opionion, likely based on experience with very different
development projects than I'm involved with. And gmp-bugs isn't really
the right place for that debate (and neither is the nettle mailinglist).

> Second, the SIGABRT terminates the process and can write a core file.

A security sensitive application can easily disable generation of core
files, using setrlimit (on the linux kernel, prctl may also be useful).
That's all part of crashing in a *controlled* fashion on assertion
failures. As far as I'm aware, disabling core dumps is a fairly common
practice in security sensitive applications.

(And besides, most systems have zero ulimit -c as the system default
these days. Which makes sense to me (any application might handle data
that is sensitive to the user), even though as a developer, it's
annoying with extra hoops required to get proper core dumps, including
disabling the core dump collection "services" you mention).

And as Vincent says, there are many ways to crash due to bugs, without
triggering any assertion failure. And you should avoid generating core
dumps for those crashes too.

Regards,
/Niels

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Vincent Lefevre]

On 2019-01-01 11:22:56 +0100, Joerg Arndt wrote:
> * Vincent Lefevre  [Jan 01. 2019 11:11]:
> > [...]
> > > 
> > > Second, the SIGABRT terminates the process and can write a core file.
> > 
> > That's the default behavior, but you can trap SIGABRT if you want.
> 
> From man 3 abort:
>   If the SIGABRT signal is ignored, or caught by a handler that
>   returns, the abort() function will still terminate the process.  It
>   does this by restoring the default disposition for SIGABRT and then
>   raising the signal for a second time.

Yes, that's why if you want to avoid process termination, you need
to write a handler that does not return (as said in the man page).
This is rather intuitive: if the handler returns, resuming the
program at the same point makes no sense because in general, the
memory state at this point is not valid.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Asserts considered harmful (or GMP spills its sensitive information)

[Jeffrey Walton]

On Mon, Dec 31, 2018 at 2:16 PM Vincent Lefevre  wrote:
>
> On 2018-12-31 13:03:27 -0500, Jeffrey Walton wrote:
> > The GMP library uses asserts to crash a program at runtime when
> > presented with data it did not expect. The library also ignores user
> > requests to remove asserts using Posix's -DNDEBUG. Posix asserts are a
> > deugging aide intended for developement, and using them in production
> > software ranges from questionable to insecure.
>
> That's much better than letting the program run erratically, with
> possible memory corruption and/or sensitive information leakage
> to unauthorized users. You'd better fix bugs in your program.

To play devil's advocate for this particular example, GMP could have
validated the parameters and refused to process the data. That is, the
function could have returned failure and avoided the potential
information leak.

> > Many programs can safely use assert to crash a program at runtime.
> > However, the prequisite is, the program cannot handle sensitive
> > information like user passwords, user keys or sensitive documents.
> >
> > High integrity software, like GMP and Nettle, cannot safely use an
> > assert to crash a program. To understand why the data flow must be
> > examined. First, when an assert fires, a SIGABRT is eventually sent to
> > the program on Unix and Linux
> > (http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html).
> >
> > Second, the SIGABRT terminates the process and can write a core file.
>
> That's the default behavior, but you can trap SIGABRT if you want.
> Of course, there is no guarantee because the memory may already be
> in an inconsistent state.

To play devil's advocate again, that strategy requires every developer
to have the knowledge and implement the sigtrap. On the other hand,
developers are usually pretty good about checking return values at a
call site.

> > This is the first point of unwanted data egress. Sensitive information
> > like user passwords and keys can be written to the filesystem
> > unprotected.
>
> This can occur with any program, even not using asserts, e.g. due to
> a segmentation fault (which may happen as a consequence of not using
> asserts, with possibly worse consequences).
>
> If you don't want a core file, then you can instruct the kernel not
> to write a core file. See getrlimit.

To play devil's advocate again, that strategy requires every user to
have the knowledge. If RTFM was going to worked, It should have
happened in the last 50 years or so.

Refusing to process the data and failing the API call requires no
knowledge on the user's part.

> > Third, the dump is sometimes sent to an error reporting service like
> > Apple Crash Report, Android Crash Report, Ubuntu Apport, and Windows
> > Error Reporting. This is the second point of unwanted data egress.
> > Sensitive information can be sent to the error reporting service. The
> > platform provider like Apple, Google, Microsoft and Ubuntu gain access
> > to the sensitive information, in addition to the developer.
>
> If you don't like them, do not use these services. Not using asserts
> can also yield a crash, which will have the same consequences.

I hope I don't sound too argumentative, but the summary seems to
conflate what's happening. You seem to be arguing all crashes are
outside the programs control. That holds sometimes but not always.

In this instance the library did not validate parameters and return an
error code. Instead it choose to crash. The library was not an
innocent victim of a memory corruption. It was a willing participant
in the data egress. Instigator may be a better term than participant
in this case.

Jeff
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs