From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Ted Zlatanov Newsgroups: gmane.emacs.devel Subject: Re: GnuTLS and zeroing keys in Emacs Date: Sat, 15 Jul 2017 15:00:23 -0400 Organization: =?utf-8?B?0KLQtdC+0LTQvtGAINCX0LvQsNGC0LDQvdC+0LI=?= @ Cienfuegos Message-ID: <877ez9frug.fsf@lifelogs.com> References: <9ee79781-8de0-06ef-6477-28af9852a92b@cs.ucla.edu> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1500145284 12842 195.159.176.226 (15 Jul 2017 19:01:24 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 15 Jul 2017 19:01:24 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Jul 15 21:01:20 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dWSJc-000370-9H for ged-emacs-devel@m.gmane.org; Sat, 15 Jul 2017 21:01:20 +0200 Original-Received: from localhost ([::1]:43014 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dWSJg-00031d-Re for ged-emacs-devel@m.gmane.org; Sat, 15 Jul 2017 15:01:24 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:33602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dWSJ2-00031R-6m for emacs-devel@gnu.org; Sat, 15 Jul 2017 15:00:45 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dWSIz-0005MD-36 for emacs-devel@gnu.org; Sat, 15 Jul 2017 15:00:44 -0400 Original-Received: from [195.159.176.226] (port=57134 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dWSIy-0005In-Ep for emacs-devel@gnu.org; Sat, 15 Jul 2017 15:00:40 -0400 Original-Received: from list by blaine.gmane.org with local (Exim 4.84_2) (envelope-from ) id 1dWSIp-0000ju-Tx for emacs-devel@gnu.org; Sat, 15 Jul 2017 21:00:31 +0200 X-Injected-Via-Gmane: http://gmane.org/ Mail-Followup-To: emacs-devel@gnu.org Original-Lines: 92 Original-X-Complaints-To: usenet@blaine.gmane.org X-Face: bd.DQ~'29fIs`T_%O%C\g%6jW)yi[zuz6; d4V0`@y-~$#3P_Ng{@m+e4o<4P'#(_GJQ%TT= D}[Ep*b!\e,fBZ'j_+#"Ps?s2!4H2-Y"sx" Mail-Copies-To: never Cancel-Lock: sha1:F3mFNx/m/DycIZgsJB1MOz2xkJ0= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 195.159.176.226 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:216701 Archived-At: On Fri, 14 Jul 2017 16:42:22 -0700 Paul Eggert 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