explicit_bzero and -std=c99

explicit_bzero and -std=c99

[Simon Josefsson via Gnulib discussion list]

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]

Hi.  We got a bug report about a build failure (see below), and I'm
wondering:

1) Does gnulib support building with gcc -std=c99?  I think we should,
but it could have documented missing functionality or breakage.

2) It seems explicit_bzero.c in gnulib fall backs to using 'asm' for
GCC, which isn't working in non-GNU modes of gcc.  Further wondering:

   1) The reason for having explicit_bzero is read_file, which needs it
   for reading sensitive files, a feature we don't use.  Uncoupling this
   unnecessary dependency would have been nice.

   2) Is there no other way to implement explicit_bzero without 'asm'?
   There is a another fallback code using volatile pointers, but I'm not
   sure it really has the same semantics.

   3) Is there a way to detect if the compiler supports 'asm'?  The
   current test 'defined __GNUC__ && !defined __clang__' is what is
   really failing here.

3) Is the idiom of using separate functions bzero() vs explicit_bzero()
   to avoid security-problematic compiler optimization still a good one?

   1) If yes, I think we should have read_sensitive_file() rather than
   extending read_file() with a flag for this purpose.

   2) If no, what is the better idiom to use here instead of
   explicit_bzero?

Other thoughts?

/Simon

Simon Josefsson via Discussion list for GNU Libtasn1
<help-libtasn1@gnu.org> writes:

> Vincent Fortier <th0ma7@gmail.com> writes:
>
>> While preparing a gnutls update I ended-up updating libtasn1 from
>> 4.16.  Going to 4.17 works but anything after that fails with:
>
> Thanks for the report!  I can reproduce this using:
>
> ./configure ac_cv_func_explicit_bzero=no CPPFLAGS="-std=c99"
>
> In other words, the problem is due to a combination of a platform
> without explicit_bzero and forcing GCC into C99 mode where it doesn't
> support 'asm', so it is not possible to implement 'explicit_bzero'.  Why
> do you hard code -std=c99?  Try just removing that.  Or use -std=gnu99'
> to allow GCC to use 'asm'.
>
> Analysing further, the 'explicit_bzero' function is only used by
> 'read_file' in a mode that we never use (for reading sensitive files),
> so it is never needed by libtasn1.  This is not ideal, but I'm not sure
> what a maintainable solution is.
>
> /Simon
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]

Re: explicit_bzero and -std=c99

[Bruno Haible]

Hi Simon,

> 1) Does gnulib support building with gcc -std=c99?  I think we should,
> but it could have documented missing functionality or breakage.

No, Gnulib does not support this. We freely use GCC extensions,
such as ({...}) or asm, usually conditionalized with
  defined __GNUC__ || defined __clang__
Only in math.in.h and xalloc-oversized.h we also test __STRICT_ANSI__.

We could test __STRICT_ANSI__ also in more places, but what really is the
point? So that people then complain "the asyncsafe-spin and simple-atomic
tests fail for me"?

The point of '-std=c99' is to verify that the source code is pure ISO C
without any extensions. Gnulib is not in that category.

> 2) It seems explicit_bzero.c in gnulib fall backs to using 'asm' for
> GCC, which isn't working in non-GNU modes of gcc.

In lib/explicit_bzero.c you see that we need to defeat compiler optimizations.
'asm' lets us formulate a memory barrier. Without 'asm' we have a fallback
code that defeated compiler optimizations at the time it was written, but
that will break in the long run as compilers are becoming better.

> Further wondering:
> 
>    1) The reason for having explicit_bzero is read_file, which needs it
>    for reading sensitive files, a feature we don't use.  Uncoupling this
>    unnecessary dependency would have been nice.

No, we have explicit_bzero because it's a glibc function that we think
should be available to programs on all OSes.
<https://www.gnu.org/software/gnulib/manual/html_node/explicit_005fbzero.html>

>    2) Is there no other way to implement explicit_bzero without 'asm'?
>    There is a another fallback code using volatile pointers, but I'm not
>    sure it really has the same semantics.

That's the point, exactly: Here 'asm' is necessary.

>    3) Is there a way to detect if the compiler supports 'asm'?  The
>    current test 'defined __GNUC__ && !defined __clang__' is what is
>    really failing here.

Probably something like
  (defined __GNUC__ || defined __clang__) && !defined __STRICT_ANSI__

> 3) Is the idiom of using separate functions bzero() vs explicit_bzero()
>    to avoid security-problematic compiler optimization still a good one?
> 
>    1) If yes, I think we should have read_sensitive_file() rather than
>    extending read_file() with a flag for this purpose.
> 
>    2) If no, what is the better idiom to use here instead of
>    explicit_bzero?

When the code for average contexts and the code for secure contexts differ
only by a few lines of code, we would like to avoid code duplication. As
code duplication means twice the maintenance effort in the future.

Things would be different when you want the code for secure contexts to
use exactly the same instructions (just with different data), regardless
of the input data.[1] Since code for average contexts is typically written
to minimize the number of instructions depending on the input data, this
would imply different algorithms and different code. But we are not in this
case here.

Bruno

[1] https://en.wikipedia.org/wiki/Timing_attack

Re: explicit_bzero and -std=c99

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

On 2022-11-27 03:27, Simon Josefsson via Gnulib discussion list wrote:

> 1) Does gnulib support building with gcc -std=c99?  I think we should,
> but it could have documented missing functionality or breakage.

It should, yes. That's a reasonable portability test, so long as Gnulib 
continues to support C99.

> 2) It seems explicit_bzero.c in gnulib fall backs to using 'asm' for
> GCC, which isn't working in non-GNU modes of gcc.  Further wondering:

I hope I fixed this particular problem by installing the attached.

Perhaps Gnulib's other uses of asm should also be changed?

>     1) The reason for having explicit_bzero is read_file, which needs it
>     for reading sensitive files, a feature we don't use.  Uncoupling this
>     unnecessary dependency would have been nice.

In the long run it should be OK; see below.

>     2) Is there no other way to implement explicit_bzero without 'asm'?
>     There is a another fallback code using volatile pointers, but I'm not
>     sure it really has the same semantics.

That fallback should work, though it's a bit slower.

>     3) Is there a way to detect if the compiler supports 'asm'?  The
>     current test 'defined __GNUC__ && !defined __clang__' is what is
>     really failing here.

We could add a configure-time test. Not sure it's worth the hassle.

> 3) Is the idiom of using separate functions bzero() vs explicit_bzero()
>     to avoid security-problematic compiler optimization still a good one?

Yes, though we should switch to memset_explicit as that's the name C23 
has standardized on. I.e., create a memset_explicit module, have other 
modules use that instead of explicit_bzero. No rush, but that's the way 
to proceed.

[-- Attachment #2: 0001-explicit_bzero-work-with-gcc-std-c99.patch --]
[-- Type: text/x-patch, Size: 1286 bytes --]

From 04191d1b325186fcd788a4a0a89274f8b9a9943b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 27 Nov 2022 09:59:32 -0800
Subject: [PATCH] explicit_bzero: work with gcc -std=c99

* lib/explicit_bzero.c (explicit_bzero) [__GNUC__ && !__clang__]:
Use __asm__ instead of asm.
---
 ChangeLog            | 6 ++++++
 lib/explicit_bzero.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 36825874d2..eedab2ae83 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2022-11-27  Paul Eggert  <eggert@cs.ucla.edu>
+
+	explicit_bzero: work with gcc -std=c99
+	* lib/explicit_bzero.c (explicit_bzero) [__GNUC__ && !__clang__]:
+	Use __asm__ instead of asm.
+
 2022-11-26  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Prefer "kill -INT" to killing with a number
diff --git a/lib/explicit_bzero.c b/lib/explicit_bzero.c
index ad0bfd170c..584f982924 100644
--- a/lib/explicit_bzero.c
+++ b/lib/explicit_bzero.c
@@ -57,7 +57,7 @@ explicit_bzero (void *s, size_t len)
 #elif defined __GNUC__ && !defined __clang__
   memset (s, '\0', len);
   /* Compiler barrier.  */
-  asm volatile ("" ::: "memory");
+  __asm__ volatile ("" ::: "memory");
 #elif defined __clang__
   memset (s, '\0', len);
   /* Compiler barrier.  */
-- 
2.37.2

Re: explicit_bzero and -std=c99

[Paul Eggert]

On 2022-11-27 10:05, Paul Eggert wrote:
> It should, yes. That's a reasonable portability test, so long as Gnulib 
> continues to support C99.

Ah, Bruno has corrected me on this. Although compiling with -std=c99 is 
OK for some parts of Gnulib, it's not OK for all. In this particular 
case the fix is easy so I guess it's OK.

Re: explicit_bzero and -std=c99

[Bruno Haible]

Paul Eggert wrote:
> >     2) Is there no other way to implement explicit_bzero without 'asm'?
> >     There is a another fallback code using volatile pointers, but I'm not
> >     sure it really has the same semantics.
> 
> That fallback should work, though it's a bit slower.

I'm afraid that the fallback code

  /* Invoke memset through a volatile function pointer.  This defeats compiler
     optimizations.  */
  void * (* const volatile volatile_memset) (void *, int, size_t) = memset;
  (void) volatile_memset (s, '\0', len);

will stop working, as compilers get "smarter".

The other code based on asm is not so likely to break, since it is documented
that "Using the "memory" clobber effectively forms a read/write memory barrier
for the compiler." [1]

Bruno

[1] https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Extended-Asm.html

Re: explicit_bzero and -std=c99

[Simon Josefsson via Gnulib discussion list]

[-- Attachment #1: Type: text/plain, Size: 4136 bytes --]

Paul Eggert <eggert@cs.ucla.edu> writes:

>> 2) It seems explicit_bzero.c in gnulib fall backs to using 'asm' for
>> GCC, which isn't working in non-GNU modes of gcc.  Further wondering:
>
> I hope I fixed this particular problem by installing the attached.

Thank you!

> Perhaps Gnulib's other uses of asm should also be changed?

Yes I think we should '__asm__' instead of 'asm' for the reason
explained by the gcc manual that Bruno linked to.

>> 3) Is the idiom of using separate functions bzero() vs explicit_bzero()
>>     to avoid security-problematic compiler optimization still a good one?
>
> Yes

If so, I would prefer a read_sensitive_file() API instead of read_file()
with a flag to enable the security-sensitive functionality.  I'll leave
it for the future, as this the immediate problem is resolved.

Bruno Haible <bruno@clisp.org> writes:

>> 1) Does gnulib support building with gcc -std=c99?  I think we should,
>> but it could have documented missing functionality or breakage.
>
> No, Gnulib does not support this. We freely use GCC extensions,
> such as ({...}) or asm, usually conditionalized with
>   defined __GNUC__ || defined __clang__
> Only in math.in.h and xalloc-oversized.h we also test __STRICT_ANSI__.
>
> We could test __STRICT_ANSI__ also in more places, but what really is the
> point? So that people then complain "the asyncsafe-spin and simple-atomic
> tests fail for me"?
>
> The point of '-std=c99' is to verify that the source code is pure ISO C
> without any extensions. Gnulib is not in that category.

Your answer is a bit different from Paul's, and both seems like
reasonable approaches to me.  This may be a situation where sometimes we
make a small effort of being compatible with -std=c99 and sometimes
decide against it.  I think what could help is a bit more documentation
about this problem.  Building gnulib with -std=c99 and fixing some of
the minor issues will likely help future compatibility of code, so I
think we should make small efforts to comply.  I agree that there is
likely some parts of gnulib that simply don't work in C99-mode --
documenting what they are would be useful.

In libtasn1, we want to support C89 environments since it is such a
low-level and bootstrap-relevant library.  At least for the library, the
command-line tool doesn't have to be C89-compatible IMHO.

>>    1) The reason for having explicit_bzero is read_file, which needs it
>>    for reading sensitive files, a feature we don't use.  Uncoupling this
>>    unnecessary dependency would have been nice.
>
> No, we have explicit_bzero because it's a glibc function that we think
> should be available to programs on all OSes.
> <https://www.gnu.org/software/gnulib/manual/html_node/explicit_005fbzero.html>

Sorry I was unclear: the reason for LIBTASN1 to have explicit_bzero is
read_file.  But libtasn1 never uses the sensitive flag, and thus never
really excercise the explicit_bzero code path.

>>    3) Is there a way to detect if the compiler supports 'asm'?  The
>>    current test 'defined __GNUC__ && !defined __clang__' is what is
>>    really failing here.
>
> Probably something like
>   (defined __GNUC__ || defined __clang__) && !defined __STRICT_ANSI__

Using __asm__ instead seems more elegant, and even aligned with gcc
manual.

>> 3) Is the idiom of using separate functions bzero() vs explicit_bzero()
>>    to avoid security-problematic compiler optimization still a good one?
>> 
>>    1) If yes, I think we should have read_sensitive_file() rather than
>>    extending read_file() with a flag for this purpose.
>> 
>>    2) If no, what is the better idiom to use here instead of
>>    explicit_bzero?
>
> When the code for average contexts and the code for secure contexts differ
> only by a few lines of code, we would like to avoid code duplication. As
> code duplication means twice the maintenance effort in the future.

Sure -- although it would be possible to implement the essence of
read_file in a way where support for the sensitive flag is a
compile-time option.

/Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]

Re: explicit_bzero and -std=c99

[Bruno Haible]

Simon Josefsson wrote:
> > Perhaps Gnulib's other uses of asm should also be changed?
> 
> Yes I think we should '__asm__' instead of 'asm' for the reason
> explained by the gcc manual that Bruno linked to.

The remaining uses of 'asm' (as opposed to '__asm__') are in
  lib/simple-atomic.c
  lib/asyncsafe-spin.c
and are for GCC versions < 4.7 (hardly in use nowadays) and for
non-GCC compilers (for which I don't want to spend time to see whether
they support '__asm__').

> If so, I would prefer a read_sensitive_file() API instead of read_file()
> with a flag to enable the security-sensitive functionality.  I'll leave
> it for the future, as this the immediate problem is resolved.

Minimizing module dependencies is a goal when that dependency is large
or otherwise cumbersome. However, the dependency to 'memset_explicit'
is small (produces a tiny .o file, even no .o file at all in the long
term) and does not require any link options. Therefore the dependency
'read-file' -> 'memset_explicit' doesn't bother me a lot.

Bruno