GnuTLS and zeroing keys in Emacs

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

GnuTLS and zeroing keys in Emacs

Paul Eggert
Thanks for your recent GnuTLS-related contributions for Emacs. I found
some minor glitches with respect to integer overflow detection and
style, and installed the attached patch to try to help clear up the
problems I found.

I did notice one other thing: sometimes the new code zeros out keys that
will be garbage, I guess under the theory that this will help protect
the user from hostile code within Emacs that attempts to steal keys by
reading "unused" memory. However, zeroing out these keys does not
actually destroy the keys reliably, as some compilers elide code that
clears about-to-become-garbage objects, and some of the strings may be
moved in garbage-collected memory (so their old contents are not zeroed).

I left in the code that clears keys, but I'm wondering: is there some
general Emacs-internal guideline about this sort of thing? If we're
serious about clearing keys I suppose we need to tell the GC and
compilers not to move them around or optimize away useless stores. If
not, then perhaps we should mark the key-clearing code in some way that
says "this doesn't work in general but is the best we can easily do".


0001-GnuTLS-integer-overflow-and-style-fixes.patch (45K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GnuTLS and zeroing keys in Emacs

Ted Zlatanov
On Fri, 14 Jul 2017 16:42:22 -0700 Paul Eggert <[hidden email]> wrote:

PE> Thanks for your recent GnuTLS-related contributions for Emacs. I found some
PE> minor glitches with respect to integer overflow detection and style, and
PE> installed the attached patch to try to help clear up the problems I
PE> found.

Beautiful, thank you for this. I appreciate your corrections very much
and will try to follow that style in the future as well. I agree with
them except in minor issues as noted below, and you're welcome to push
them any time.

PE> I did notice one other thing: sometimes the new code zeros out keys that will be
PE> garbage, I guess under the theory that this will help protect the user from
PE> hostile code within Emacs that attempts to steal keys by reading "unused"
PE> memory. However, zeroing out these keys does not actually destroy the keys
PE> reliably, as some compilers elide code that clears about-to-become-garbage
PE> objects, and some of the strings may be moved in garbage-collected memory (so
PE> their old contents are not zeroed).

PE> I left in the code that clears keys, but I'm wondering: is there some general
PE> Emacs-internal guideline about this sort of thing? If we're serious about
PE> clearing keys I suppose we need to tell the GC and compilers not to move them
PE> around or optimize away useless stores. If not, then perhaps we should mark the
PE> key-clearing code in some way that says "this doesn't work in general but is the
PE> best we can easily do".

Hmm, the best way is to either use gnutls_memset() (available since only
3.4.0 in lib/safe-memfuncs.c) or to copy it. If you think this will only
have utility in GnuTLS-related code, I'd make a wrapper that calls
gnutls_memset() if it's available, taking advantage of future
improvements, or our own version of it. If you think it's generally
useful, I'd copy it right away so it's available even if GnuTLS is not.

My vote is to copy it. Let me know if you want to do it, or if I should.

/**
 * gnutls_memset:
 * @data: the memory to set
 * @c: the constant byte to fill the memory with
 * @size: the size of memory
 *
 * This function will operate similarly to memset(), but will
 * not be optimized out by the compiler.
 *
 * Returns: void.
 *
 * Since: 3.4.0
 **/
void gnutls_memset(void *data, int c, size_t size)
{
        volatile unsigned volatile_zero = 0;
        volatile char *vdata = (volatile char*)data;

        /* This is based on a nice trick for safe memset,
         * sent by David Jacobson in the openssl-dev mailing list.
         */

        if (size > 0) {
                do {
                        memset(data, c, size);
                } while(vdata[volatile_zero] != c);
        }
}

 
PE> -      Lisp_Object cp = listn (CONSTYPE_HEAP, 15,
PE> -                              /* A symbol representing the cipher */
PE> -                              intern (gnutls_cipher_get_name (gca)),

You removed the comments from these lists in the C code in several
places, but I do think they are useful to a reader. Can we keep them?

PE> +  ret = ((encrypting ? gnutls_aead_cipher_encrypt : gnutls_aead_cipher_decrypt)
PE> + (acipher, vdata, vsize, aead_auth_data, aead_auth_size,
PE> +  cipher_tag_size, idata, isize, storage, &storage_length));
...
PE> +  ret = ((encrypting ? gnutls_cipher_encrypt2 : gnutls_cipher_decrypt2)
PE> + (hcipher, idata, iend_byte - istart_byte,
PE> +  SSDATA (storage), storage_length));

As a matter of readability, I'd make a function pointer variable and
assign it separately. But I realize the advantage of brevity and
type-agnostic code here, so I leave it up to you.
 
PE> -Returns nil on error.
PE> +Return nil on error.

Is this right? It seems it's talking from the POV of the function.

Thanks
Ted


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GnuTLS and zeroing keys in Emacs

Paul Eggert
Ted Zlatanov wrote:
> the best way is to either use gnutls_memset() (available since only
> 3.4.0 in lib/safe-memfuncs.c) or to copy it.

These days glibc's explicit_bzero is a better way to go, as its implementation
should be more reliable than the 'volatile' trick used by gnutls_memset. So I
installed the attached patches into master: they either use explicit_bzero, or
copy it.

I'll file a bug report with the GnuTLS folks to suggest that they use
explicit_bzero if available.

0001-Merge-from-gnulib.patch (8K) Download Attachment
0002-Use-explicit_bzero-to-clear-GnuTLS-keys.patch (10K) Download Attachment
0003-Use-memset-not-bzero.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GnuTLS and zeroing keys in Emacs

Paul Eggert
In reply to this post by Ted Zlatanov
Ted Zlatanov wrote:

> PE> -      Lisp_Object cp = listn (CONSTYPE_HEAP, 15,
> PE> -                              /* A symbol representing the cipher */
> PE> -                              intern (gnutls_cipher_get_name (gca)),
>
> You removed the comments from these lists in the C code in several
> places, but I do think they are useful to a reader. Can we keep them?

I restored those particular comments by installing the attached patch. That
being said, I found the comments to have more cost (in terms of clutter) than
benefit (in terms of useful information conveyed), and similarly for some nearby
comments that I also removed. For example, anyone who reads the Emacs C code
should know that "intern (gnutls_cipher_get_name (gca))" returns a symbol
representing the cipher name, and the nearby comment "/* A symbol representing
the GnuTLS cipher. */" just gets in the way; it has the feel of "n++; /*
Increment N. */".

Admittedly this is a judgment call.

> PE> -Returns nil on error.
> PE> +Return nil on error.
>
> Is this right? It seems it's talking from the POV of the function.

Doc strings for functions should use the imperative form, as it is typically
shorter and less likely to be misunderstood. For example the doc string for the
'length' function begins "Return the length of ...".

0001-src-gnutls.c-Restore-some-comments.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GnuTLS and zeroing keys in Emacs

Ted Zlatanov
In reply to this post by Paul Eggert
On Sun, 16 Jul 2017 16:53:20 -0700 Paul Eggert <[hidden email]> wrote:

PE> Ted Zlatanov wrote:
>> the best way is to either use gnutls_memset() (available since only
>> 3.4.0 in lib/safe-memfuncs.c) or to copy it.

PE> These days glibc's explicit_bzero is a better way to go, as its implementation
PE> should be more reliable than the 'volatile' trick used by gnutls_memset. So I
PE> installed the attached patches into master: they either use explicit_bzero, or
PE> copy it.

PE> I'll file a bug report with the GnuTLS folks to suggest that they use
PE> explicit_bzero if available.

Terrific, thanks again for working on this.

Ted


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GnuTLS and zeroing keys in Emacs

Ted Zlatanov
In reply to this post by Paul Eggert
On Sun, 16 Jul 2017 17:29:09 -0700 Paul Eggert <[hidden email]> wrote:

PE> I restored those particular comments by installing the attached patch. That
PE> being said, I found the comments to have more cost (in terms of clutter) than
PE> benefit (in terms of useful information conveyed), and similarly for some nearby
PE> comments that I also removed.

Understood. I think this is a balancing act for sure. Thank you for
restoring some of the comments.

PE> Doc strings for functions should use the imperative form, as it is typically
PE> shorter and less likely to be misunderstood. For example the doc string for the
PE> 'length' function begins "Return the length of ...".

I had never noticed that in the docs, and it's explained already in
doc/lispref/tips.texi so I'll make sure to re-read those tips :) Thanks!

Ted


Loading...