[PATCH RFC] explicit_bzero, again

[Zack Weinberg]

I'd like to resume the discussion of adding explicit_bzero or similar
to glibc.  This was originally proposed by Nick Mathewson last
December (https://sourceware.org/ml/libc-alpha/2014-12/msg00506.html)
and I have prepared an updated patchset, which is attached to this
message.  One patch adds the function, and the second adds several
intra-libc uses; I suspect I haven't found all of the places where
it should be used.

To reiterate from last time, the problem explicit_bzero solves is: in
a function of the form

    void encrypt_with_phrase(const char *phrase, const char *in,
                             char *out, size_t n)
    {
      char key[16];
      genkey(phrase, key);
      encrypt(key, in, out, n);

      memset(key, 0, 16);
    }

the compiler may well remove the final call to memset, because `key`
goes out of scope immediately afterward, so no conforming C program
can access the memory to see that it has been cleared.  However, a
crash dump, a debugger, or a plain old bug in the program might expose
that region of the stack to prying eyes.  This is listed at
https://cwe.mitre.org/data/definitions/14.html as a "common weakness".
explicit_bzero is just bzero with an additional, documented guarantee
that the compiler will not optimize it out even if the memory region is
dead afterward.

In earlier discussion, Rich Felker pointed out that this is inadequate
to solve the problem, because the compiler might make temporary copies
of the data in `key` in memory or registers; a complete fix requires
additional compiler features.
(https://sourceware.org/ml/libc-alpha/2014-12/msg00513.html) This is
true, but I do not think it should be an obstacle to adding the
function to gcc, for three reasons:

1) explicit_bzero in glibc is strictly better than the status quo.
In its absence, application developers either cross their fingers and
call `memset` (there are cases of this in glibc itself!) or they
invent their own version, with varying degrees of correctness and
efficiency.  By providing it in glibc we can ensure that applications
using it get as close to the effect they want as is possible without
compiler support.  (In my patch, I have carefully documented what
explicit_bzero can and cannot be expected to do.)

2) If compiler support for erasure of sensitive data becomes available
in the future, explicit_bzero can become a backward-compatible
indicator of which data needs to be reliably erased.  Hypothetically,
in GCC terms, the obvious programmer-visible interface would be
__attribute__ ((sensitive)) applied to a variable or structure field.
GCC could then implicitly apply this attribute to any object passed to
explicit_bzero.

3) It is possible to implement explicit_bzero *in the C library* more
efficiently than it is to implement it anywhere else.  This is
because, in the C library, it doesn't need to be anything more than a
weak alias for bzero!  Exposing bzero under a second name that is
meaningless to the compiler is all you have to do to inhibit the
undesired dead-store removal.  (In my patch I have gone somewhat
beyond this, see below.)

3a) It is impractical for anyone but glibc to equip explicit_bzero
with -D_FORTIFY_SOURCE support.

There was also a question of what to name the function, there being
several competing options.  In my updated patch I have selected
explicit_bzero, largely because it is more broadly adopted by both C
libraries (OpenBSD _and_ FreeBSD, which is more than any other
competitor can say) and in applications (see
e.g. https://codesearch.debian.net/results/explicit_bzero/page_0) than
any of its competitors.

memset_s was also suggested; it has the advantage of being part of an
official standard (C99 Annex K), but the disadvantage of a
significantly more complicated specification and API, which would
require it to be its own function rather than another name for an
existing one.  Moreover, despite Annex K being Microsoft's invention,
MSVC's runtime does _not_ provide it; the only C library I've found
that has it is MacOS X's.  And
https://codesearch.debian.net/results/memset_s/page_0/ indicates that
some programs (notably util-linux) supply their own definition of
memset_s which is _incompatible_ with Annex K.  All these mean that
IMNSHO standardization does not outweigh community adoption in this
case.

Finally, some notes on my implementation.  As I said above, all you
_really_ need to do this reliably (as far as it can be without
compiler support) is to expose a function that does what bzero does,
but under a name that is meaningless to the compiler.  Then it can't
be optimized out, and obviously the implementation will continue to do
what it does.

However, if you have a construct that expresses a read access to the
written memory region, but generates no instructions, you can use that
to force the compiler to preserve the write.  Then it becomes safe to
replace explicit_bzero with memset, which is desirable because e.g. it
re-enables conversion to inline store instructions.  I was able to
find a construct with this effect, but not a universal one.  It only
works in GCC, only when compiling C, and the manual implies that it
might not be sound when the size of the memory region isn't known at
compile time.

    __STRING_INLINE void
    __explicit_bzero_constn(void *__s, size_t __n)
    {
      typedef struct {char __x[__n];} __memblk;
      memset(__s, 0, __n);
      __asm__ ("" : :
        "m" (*(__memblk __attribute__ ((may_alias)) *)__s));
    }

When compiling C++, 'struct {char x[n]}' is a hard error, even if I
convert the whole thing to a macro and guard it with
__builtin_constant_p.  The same is true for clang in both C and C++.

Because we don't have a universal read-use construct, the fortify
wrapper for explicit_bzero is more complicated than it could have
been: in particular, __explicit_bzero_chk has to be a real,
out-of-line function (unlike __bzero_chk, which doesn't exist) and
that caused me a certain amount of headache -- see the other thread
about __memset_chk.  But it works.  All this can also easily be
improved if compiler support improves.

I did add tests, but they are cursory.  For an automated test that
explicit_bzero can't be optimized out, without doing anything
undefined in the test program, we would need something like GCC's
scan-assembler tests, which I don't believe there is any way to do in
the existing glibc testsuite.  I've manually vetted assembly output
for several code fragments, like the one at the top of the message.

(Appropriate additions to debug/tst-chk1.c are stacked up in my repo on
top of the fix for bug 18975.  I was going to hold off posting this
until that got committed, but then someone else started a thread to
which it is Highly Relevant. ;-)

zw
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Use-explicit_bzero-where-appropriate.patch
Type: text/x-patch
Size: 4243 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150809/4d90b115/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-New-library-function-explicit_bzero.patch
Type: text/x-patch
Size: 42760 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150809/4d90b115/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150809/4d90b115/attachment.sig>