Re: GnuTLS and zeroing keys in Emacs

[Ted Zlatanov <tzz@lifelogs.com>]

On Fri, 14 Jul 2017 16:42:22 -0700 Paul Eggert <eggert@cs.ucla.edu> 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