unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* GnuTLS and zeroing keys in Emacs
@ 2017-07-14 23:42 Paul Eggert
  2017-07-15 19:00 ` Ted Zlatanov
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Eggert @ 2017-07-14 23:42 UTC (permalink / raw)
  To: Ted Zlatanov; +Cc: Emacs development discussions

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

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".


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-GnuTLS-integer-overflow-and-style-fixes.patch --]
[-- Type: text/x-patch; name="0001-GnuTLS-integer-overflow-and-style-fixes.patch", Size: 45678 bytes --]

From 05b8b866993b957f5fd575846cf8ea3035e60f7e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 14 Jul 2017 16:18:37 -0700
Subject: [PATCH] GnuTLS integer-overflow and style fixes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This tweaks the recently-added GnuTLS improvements so that
they avoid some integer-overflow problems and follow typical
Emacs style a bit better.
* configure.ac (HAVE_GNUTLS3_HMAC, HAVE_GNUTLS3_AEAD)
(HAVE_GNUTLS3_CIPHER): Use AC_CACHE_CHECK so that the
configure-time results are displayed.
* src/fns.c (extract_data_from_object): Return char *, not char
const *, since one gnutls caller wants a non-const pointer.  Use
CONSP rather than !NILP when testing for conses.  Use CAR_SAFE
instead of rolling our own code.  Prefer signed types to unsigned
when either will do.  Report problems for lengths out of range,
instead of silently mishandling them.
* src/gnutls.c (emacs_gnutls_strerror): New function, to simplify
callers.  All callers of gnutls_sterror changed.
(Fgnutls_boot): Check for integers out of range rather than
silently truncating them.
(gnutls_symmetric_aead): Check for integer overflow in size
calculations.
(gnutls_symmetric_aead, Fgnutls_macs, Fgnutls_digests):
Prefer signed to unsigned integers where either	will do.
(gnutls_symmetric_aead, gnutls_symmetric):
Work even if ptrdiff_t is wider than ‘long’.
(gnutls_symmetric, Fgnutls_hash_mac, Fgnutls_hash_digest):
Check for integer overflow in algorithm selection.
---
 configure.ac | 134 +++++++++-------
 src/fns.c    |  49 +++---
 src/gnutls.c | 489 ++++++++++++++++++++++++++---------------------------------
 src/lisp.h   |   4 +-
 4 files changed, 317 insertions(+), 359 deletions(-)

diff --git a/configure.ac b/configure.ac
index 525aa51598..056c8c35c5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2832,60 +2832,86 @@ AC_DEFUN
     EMACS_CHECK_MODULES([LIBGNUTLS3], [gnutls >= 3.0.0],
       [AC_DEFINE(HAVE_GNUTLS3, 1, [Define if using GnuTLS v3.])], [])
 
-    AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-#include <gnutls/gnutls.h>
-#include <gnutls/crypto.h>
-]],
-[[
-int main (int argc, char **argv)
-{
-  gnutls_hmac_hd_t handle;
-  gnutls_hmac_deinit(handle, NULL);
-}
-]])],
-    [AC_DEFINE(HAVE_GNUTLS3_HMAC, 1, [Define if using GnuTLS v3 with HMAC support.])])
-
-    AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-#include <gnutls/gnutls.h>
-#include <gnutls/crypto.h>
-]],
-[[
-int main (int argc, char **argv)
-{
-    gnutls_aead_cipher_hd_t handle;
-    gnutls_aead_cipher_deinit(handle);
-}
-]])],
-    [AC_DEFINE(HAVE_GNUTLS3_AEAD, 1, [Define if using GnuTLS v3 with AEAD support.])])
-
-    AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-#include <gnutls/gnutls.h>
-#include <gnutls/crypto.h>
-]],
-[[
-int main (int argc, char **argv)
-{
-    gnutls_cipher_hd_t handle;
-    gnutls_cipher_encrypt2 (handle,
-                            NULL, 0,
-                            NULL, 0);
-    gnutls_cipher_deinit(handle);
-}
-]])],
-    [AC_DEFINE(HAVE_GNUTLS3_CIPHER, 1, [Define if using GnuTLS v3 with cipher support.])])
-
-    AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-#include <gnutls/gnutls.h>
-#include <gnutls/crypto.h>
-]],
-[[
-int main (int argc, char **argv)
-{
-    gnutls_hash_hd_t handle;
-    gnutls_hash_deinit(handle, NULL);
-}
-]])],
-    [AC_DEFINE(HAVE_GNUTLS3_DIGEST, 1, [Define if using GnuTLS v3 with digest support.])])
+    AC_CACHE_CHECK([for GnuTLS v3 with HMAC], [emacs_cv_gnutls3_hmac],
+      [AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM([[
+	      #include <gnutls/gnutls.h>
+	      #include <gnutls/crypto.h>
+	   ]], [[
+	     int
+	     main (void)
+	     {
+	       gnutls_hmac_hd_t handle;
+	       gnutls_hmac_deinit (handle, NULL);
+	     }
+	   ]])],
+	[emacs_cv_gnutls3_hmac=yes],
+	[emacs_cv_gnutls3_hmac=no])])
+    if test "$emacs_cv_gnutls3_hmac" = yes; then
+      AC_DEFINE([HAVE_GNUTLS3_HMAC], [1],
+	[Define if using GnuTLS v3 with HMAC support.])
+    fi
+
+    AC_CACHE_CHECK([for GnuTLS v3 with AEAD], [emacs_cv_gnutls3_aead],
+      [AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM([[
+	      #include <gnutls/gnutls.h>
+	      #include <gnutls/crypto.h>
+	   ]], [[
+	     int
+	     main (void)
+	     {
+	       gnutls_aead_cipher_hd_t handle;
+	       gnutls_aead_cipher_deinit (handle);
+	     }
+	   ]])],
+	[emacs_cv_gnutls3_aead=yes],
+	[emacs_cv_gnutls3_aead=no])])
+    if test "$emacs_cv_gnutls3_aead" = yes; then
+      AC_DEFINE([HAVE_GNUTLS3_AEAD], [1],
+	[Define if using GnuTLS v3 with AEAD support.])
+    fi
+
+    AC_CACHE_CHECK([for GnuTLS v3 with cipher], [emacs_cv_gnutls3_cipher],
+      [AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM([[
+	      #include <gnutls/gnutls.h>
+	      #include <gnutls/crypto.h>
+	   ]], [[
+	     int
+	     main (void)
+	     {
+	       gnutls_cipher_hd_t handle;
+	       gnutls_cipher_encrypt2 (handle, NULL, 0, NULL, 0);
+	       gnutls_cipher_deinit (handle);
+	     }
+	   ]])],
+	[emacs_cv_gnutls3_cipher=yes],
+	[emacs_cv_gnutls3_cipher=no])])
+    if test "$emacs_cv_gnutls3_cipher" = yes; then
+      AC_DEFINE([HAVE_GNUTLS3_CIPHER], [1],
+	[Define if using GnuTLS v3 with cipher support.])
+    fi
+
+    AC_CACHE_CHECK([for GnuTLS v3 with digest], [emacs_cv_gnutls3_digest],
+      [AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM([[
+	      #include <gnutls/gnutls.h>
+	      #include <gnutls/crypto.h>
+	   ]], [[
+	     int
+	     main (void)
+	     {
+	       gnutls_hash_hd_t handle;
+	       gnutls_hash_deinit (handle, NULL);
+	     }
+	   ]])],
+	[emacs_cv_gnutls3_digest=yes],
+	[emacs_cv_gnutls3_digest=no])])
+    if test "$emacs_cv_gnutls3_digest" = yes; then
+      AC_DEFINE([HAVE_GNUTLS3_DIGEST], [1],
+	[Define if using GnuTLS v3 with digest support.])
+    fi
   fi
 
   # Windows loads GnuTLS dynamically
diff --git a/src/fns.c b/src/fns.c
index b678a482bb..fb1296bc6f 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -46,10 +46,6 @@ static void sort_vector_copy (Lisp_Object, ptrdiff_t,
 enum equal_kind { EQUAL_NO_QUIT, EQUAL_PLAIN, EQUAL_INCLUDING_PROPERTIES };
 static bool internal_equal (Lisp_Object, Lisp_Object,
 			    enum equal_kind, int, Lisp_Object);
-static Lisp_Object
-secure_hash (Lisp_Object algorithm, Lisp_Object object, Lisp_Object start,
-	     Lisp_Object end, Lisp_Object coding_system, Lisp_Object noerror,
-	     Lisp_Object binary);
 
 DEFUN ("identity", Fidentity, Sidentity, 1, 1, 0,
        doc: /* Return the argument unchanged.  */
@@ -4767,29 +4763,24 @@ DEFUN ("secure-hash-algorithms", Fsecure_hash_algorithms,
 (BUFFER-OR-STRING-OR-SYMBOL START END CODING-SYSTEM NOERROR) which behave as
 specified with `secure-hash' and in Info node
 `(elisp)Format of GnuTLS Cryptography Inputs'.  */
-const char*
+char *
 extract_data_from_object (Lisp_Object spec,
                           ptrdiff_t *start_byte,
                           ptrdiff_t *end_byte)
 {
-  ptrdiff_t size, start_char = 0, end_char = 0;
-  register EMACS_INT b, e;
-  register struct buffer *bp;
-  EMACS_INT temp;
+  Lisp_Object object = XCAR (spec);
 
-  Lisp_Object object        = XCAR (spec);
+  if (CONSP (spec)) spec = XCDR (spec);
+  Lisp_Object start = CAR_SAFE (spec);
 
-  if (! NILP (spec)) spec = XCDR (spec);
-  Lisp_Object start	    = (CONSP (spec)) ? XCAR (spec) : Qnil;
+  if (CONSP (spec)) spec = XCDR (spec);
+  Lisp_Object end = CAR_SAFE (spec);
 
-  if (! NILP (spec)) spec = XCDR (spec);
-  Lisp_Object end	    = (CONSP (spec)) ? XCAR (spec) : Qnil;
+  if (CONSP (spec)) spec = XCDR (spec);
+  Lisp_Object coding_system = CAR_SAFE (spec);
 
-  if (! NILP (spec)) spec = XCDR (spec);
-  Lisp_Object coding_system = (CONSP (spec)) ? XCAR (spec) : Qnil;
-
-  if (! NILP (spec)) spec = XCDR (spec);
-  Lisp_Object noerror	    = (CONSP (spec)) ? XCAR (spec) : Qnil;
+  if (CONSP (spec)) spec = XCDR (spec);
+  Lisp_Object noerror = CAR_SAFE (spec);
 
   if (STRINGP (object))
     {
@@ -4817,7 +4808,7 @@ extract_data_from_object (Lisp_Object spec,
       if (STRING_MULTIBYTE (object))
 	object = code_convert_string (object, coding_system, Qnil, 1, 0, 1);
 
-      size = SCHARS (object);
+      ptrdiff_t size = SCHARS (object), start_char, end_char;
       validate_subarray (object, start, end, size, &start_char, &end_char);
 
       *start_byte = !start_char ? 0 : string_char_to_byte (object, start_char);
@@ -4828,12 +4819,13 @@ extract_data_from_object (Lisp_Object spec,
   else if (BUFFERP (object))
     {
       struct buffer *prev = current_buffer;
+      EMACS_INT b, e;
 
       record_unwind_current_buffer ();
 
       CHECK_BUFFER (object);
 
-      bp = XBUFFER (object);
+      struct buffer *bp = XBUFFER (object);
       set_buffer_internal (bp);
 
       if (NILP (start))
@@ -4853,7 +4845,11 @@ extract_data_from_object (Lisp_Object spec,
 	}
 
       if (b > e)
-	temp = b, b = e, e = temp;
+	{
+	  EMACS_INT temp = b;
+	  b = e;
+	  e = temp;
+	}
 
       if (!(BEGV <= b && e <= ZV))
 	args_out_of_range (start, end);
@@ -4932,14 +4928,13 @@ extract_data_from_object (Lisp_Object spec,
   else if (EQ (object, Qiv_auto))
     {
 #ifdef HAVE_GNUTLS3
-      // Format: (iv-auto REQUIRED-LENGTH)
+      /* Format: (iv-auto REQUIRED-LENGTH).  */
 
-      if (! INTEGERP (start))
+      if (! NATNUMP (start))
         error ("Without a length, iv-auto can't be used. See manual.");
       else
         {
-          /* Make sure the value of "start" doesn't change.  */
-          size_t start_hold = XUINT (start);
+	  EMACS_INT start_hold = XFASTINT (start);
           object = make_uninit_string (start_hold);
           gnutls_rnd (GNUTLS_RND_NONCE, SSDATA (object), start_hold);
 
@@ -4971,7 +4966,7 @@ secure_hash (Lisp_Object algorithm, Lisp_Object object, Lisp_Object start,
 
   Lisp_Object spec = list5 (object, start, end, coding_system, noerror);
 
-  const char* input = extract_data_from_object (spec, &start_byte, &end_byte);
+  const char *input = extract_data_from_object (spec, &start_byte, &end_byte);
 
   if (input == NULL)
     error ("secure_hash: failed to extract data from object, aborting!");
diff --git a/src/gnutls.c b/src/gnutls.c
index 761fe7df3a..5717b3075c 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -672,6 +672,13 @@ emacs_gnutls_read (struct Lisp_Process *proc, char *buf, ptrdiff_t nbyte)
   }
 }
 
+static char const *
+emacs_gnutls_strerror (int err)
+{
+  char const *str = gnutls_strerror (err);
+  return str ? str : "unknown";
+}
+
 /* Report a GnuTLS error to the user.
    Return true if the error code was successfully handled.  */
 static bool
@@ -680,7 +687,6 @@ emacs_gnutls_handle_error (gnutls_session_t session, int err)
   int max_log_level = 0;
 
   bool ret;
-  const char *str;
 
   /* TODO: use a Lisp_Object generated by gnutls_make_error?  */
   if (err >= 0)
@@ -692,9 +698,7 @@ emacs_gnutls_handle_error (gnutls_session_t session, int err)
 
   /* TODO: use gnutls-error-fatalp and gnutls-error-string.  */
 
-  str = gnutls_strerror (err);
-  if (!str)
-    str = "unknown";
+  char const *str = emacs_gnutls_strerror (err);
 
   if (gnutls_error_is_fatal (err))
     {
@@ -708,11 +712,11 @@ emacs_gnutls_handle_error (gnutls_session_t session, int err)
 #endif
 
       GNUTLS_LOG2 (level, max_log_level, "fatal error:", str);
-      ret = 0;
+      ret = false;
     }
   else
     {
-      ret = 1;
+      ret = true;
 
       switch (err)
         {
@@ -900,7 +904,7 @@ usage: (gnutls-error-string ERROR)  */)
   if (! TYPE_RANGED_INTEGERP (int, err))
     return build_string ("Not an error symbol or code");
 
-  return build_string (gnutls_strerror (XINT (err)));
+  return build_string (emacs_gnutls_strerror (XINT (err)));
 }
 
 DEFUN ("gnutls-deinit", Fgnutls_deinit, Sgnutls_deinit, 1, 1, 0,
@@ -1592,9 +1596,9 @@ one trustfile (usually a CA bundle).  */)
       XPROCESS (proc)->gnutls_x509_cred = x509_cred;
 
       verify_flags = Fplist_get (proplist, QCverify_flags);
-      if (NUMBERP (verify_flags))
+      if (TYPE_RANGED_INTEGERP (unsigned int, verify_flags))
 	{
-	  gnutls_verify_flags = XINT (verify_flags);
+	  gnutls_verify_flags = XFASTINT (verify_flags);
 	  GNUTLS_LOG (2, max_log_level, "setting verification flags");
 	}
       else if (NILP (verify_flags))
@@ -1818,39 +1822,32 @@ This function may also return `gnutls-e-again', or
 DEFUN ("gnutls-ciphers", Fgnutls_ciphers, Sgnutls_ciphers, 0, 0, 0,
        doc: /* Return alist of GnuTLS symmetric cipher descriptions as plists.
 The alist key is the cipher name. */)
-     (void)
+  (void)
 {
   Lisp_Object ciphers = Qnil;
 
-  const gnutls_cipher_algorithm_t* gciphers = gnutls_cipher_list ();
-  for (size_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
+  const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
+  for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
     {
-      const gnutls_cipher_algorithm_t gca = gciphers[pos];
-
-      Lisp_Object cp = listn (CONSTYPE_HEAP, 15,
-                              /* A symbol representing the cipher */
-                              intern (gnutls_cipher_get_name (gca)),
-                              /* The internally meaningful cipher ID */
-                              QCcipher_id,
-                              make_number (gca),
-                              /* The type (vs. other GnuTLS objects). */
-                              QCtype,
-                              Qgnutls_type_cipher,
-                              /* The tag size (nonzero means AEAD). */
-                              QCcipher_aead_capable,
-                              (gnutls_cipher_get_tag_size (gca) == 0) ? Qnil : Qt,
-                              /* The tag size (nonzero means AEAD). */
-                              QCcipher_tagsize,
-                              make_number (gnutls_cipher_get_tag_size (gca)),
-                              /* The block size */
-                              QCcipher_blocksize,
-                              make_number (gnutls_cipher_get_block_size (gca)),
-                              /* The key size */
-                              QCcipher_keysize,
-                              make_number (gnutls_cipher_get_key_size (gca)),
-                              /* IV size */
-                              QCcipher_ivsize,
-                              make_number (gnutls_cipher_get_iv_size (gca)));
+      gnutls_cipher_algorithm_t gca = gciphers[pos];
+      Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
+      ptrdiff_t cipher_tag_size = gnutls_cipher_get_tag_size (gca);
+
+      Lisp_Object cp
+	= listn (CONSTYPE_HEAP, 15, cipher_symbol,
+		 QCcipher_id, make_number (gca),
+		 QCtype, Qgnutls_type_cipher,
+		 QCcipher_aead_capable, cipher_tag_size == 0 ? Qnil : Qt,
+		 QCcipher_tagsize, make_number (cipher_tag_size),
+
+		 QCcipher_blocksize,
+		 make_number (gnutls_cipher_get_block_size (gca)),
+
+		 QCcipher_keysize,
+		 make_number (gnutls_cipher_get_key_size (gca)),
+
+		 QCcipher_ivsize,
+		 make_number (gnutls_cipher_get_iv_size (gca)));
 
       ciphers = Fcons (cp, ciphers);
     }
@@ -1861,36 +1858,35 @@ The alist key is the cipher name. */)
 static Lisp_Object
 gnutls_symmetric_aead (bool encrypting, gnutls_cipher_algorithm_t gca,
                        Lisp_Object cipher,
-                       const char* kdata, size_t ksize,
-                       const char* vdata, size_t vsize,
-                       const char* idata, size_t isize,
+		       const char *kdata, ptrdiff_t ksize,
+		       const char *vdata, ptrdiff_t vsize,
+		       const char *idata, ptrdiff_t isize,
                        Lisp_Object aead_auth)
 {
 #ifdef HAVE_GNUTLS3_AEAD
 
-  const char* desc = (encrypting ? "encrypt" : "decrypt");
-  int ret = GNUTLS_E_SUCCESS;
+  const char *desc = encrypting ? "encrypt" : "decrypt";
   Lisp_Object actual_iv = make_unibyte_string (vdata, vsize);
 
   gnutls_aead_cipher_hd_t acipher;
-  gnutls_datum_t key_datum = { (unsigned char*) kdata, ksize };
-  ret = gnutls_aead_cipher_init (&acipher, gca, &key_datum);
+  gnutls_datum_t key_datum = { (unsigned char *) kdata, ksize };
+  int ret = gnutls_aead_cipher_init (&acipher, gca, &key_datum);
 
   if (ret < GNUTLS_E_SUCCESS)
-    {
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
-      error ("GnuTLS AEAD cipher %s/%s initialization failed: %s",
-             gnutls_cipher_get_name (gca), desc, str);
-    }
-
-  size_t storage_length = isize + gnutls_cipher_get_tag_size (gca);
+    error ("GnuTLS AEAD cipher %s/%s initialization failed: %s",
+	   gnutls_cipher_get_name (gca), desc, emacs_gnutls_strerror (ret));
+
+  ptrdiff_t cipher_tag_size = gnutls_cipher_get_tag_size (gca);
+  ptrdiff_t tagged_size;
+  if (INT_ADD_WRAPV (isize, cipher_tag_size, &tagged_size)
+      || SIZE_MAX < tagged_size)
+    memory_full (SIZE_MAX);
+  size_t storage_length = tagged_size;
   USE_SAFE_ALLOCA;
-  unsigned char *storage = SAFE_ALLOCA (storage_length);
+  char *storage = SAFE_ALLOCA (storage_length);
 
-  const char* aead_auth_data = NULL;
-  size_t aead_auth_size = 0;
+  const char *aead_auth_data = NULL;
+  ptrdiff_t aead_auth_size = 0;
 
   if (!NILP (aead_auth))
     {
@@ -1900,8 +1896,8 @@ gnutls_symmetric_aead (bool encrypting, gnutls_cipher_algorithm_t gca,
       CHECK_CONS (aead_auth);
 
       ptrdiff_t astart_byte, aend_byte;
-      const char* adata = extract_data_from_object (aead_auth, &astart_byte, &aend_byte);
-
+      const char *adata
+	= extract_data_from_object (aead_auth, &astart_byte, &aend_byte);
       if (adata == NULL)
         error ("GnuTLS AEAD cipher auth extraction failed");
 
@@ -1909,53 +1905,38 @@ gnutls_symmetric_aead (bool encrypting, gnutls_cipher_algorithm_t gca,
       aead_auth_size = aend_byte - astart_byte;
     }
 
-  size_t expected_remainder = 0;
-
-  if (!encrypting)
-    expected_remainder = gnutls_cipher_get_tag_size (gca);
+  ptrdiff_t expected_remainder = encrypting ? 0 : cipher_tag_size;
+  ptrdiff_t cipher_block_size = gnutls_cipher_get_block_size (gca);
 
-  if ((isize - expected_remainder) % gnutls_cipher_get_block_size (gca) != 0)
-    error ("GnuTLS AEAD cipher %s/%s input block length %ld was not a "
-           "multiple of the required %ld plus the expected tag remainder %ld",
+  if (isize < expected_remainder
+      || (isize - expected_remainder) % cipher_block_size != 0)
+    error (("GnuTLS AEAD cipher %s/%s input block length %"pD"d "
+	    "is not %"pD"d greater than a multiple of the required %"pD"d"),
            gnutls_cipher_get_name (gca), desc,
-           (long) isize, (long) gnutls_cipher_get_block_size (gca),
-           (long) expected_remainder);
-
-  if (encrypting)
-    ret = gnutls_aead_cipher_encrypt (acipher,
-                                      vdata, vsize,
-                                      aead_auth_data, aead_auth_size,
-                                      gnutls_cipher_get_tag_size (gca),
-                                      idata, isize,
-                                      storage, &storage_length);
-  else
-    ret = gnutls_aead_cipher_decrypt (acipher,
-                                      vdata, vsize,
-                                      aead_auth_data, aead_auth_size,
-                                      gnutls_cipher_get_tag_size (gca),
-                                      idata, isize,
-                                      storage, &storage_length);
+	   isize, expected_remainder, cipher_block_size);
+
+  ret = ((encrypting ? gnutls_aead_cipher_encrypt : gnutls_aead_cipher_decrypt)
+	 (acipher, vdata, vsize, aead_auth_data, aead_auth_size,
+	  cipher_tag_size, idata, isize, storage, &storage_length));
 
   if (ret < GNUTLS_E_SUCCESS)
     {
       memset (storage, 0, storage_length);
       SAFE_FREE ();
       gnutls_aead_cipher_deinit (acipher);
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
       error ("GnuTLS AEAD cipher %s %sion failed: %s",
-             gnutls_cipher_get_name (gca), desc, str);
+	     gnutls_cipher_get_name (gca), desc, emacs_gnutls_strerror (ret));
     }
 
   gnutls_aead_cipher_deinit (acipher);
 
-  Lisp_Object output = make_unibyte_string ((const char *)storage, storage_length);
+  Lisp_Object output = make_unibyte_string (storage, storage_length);
   memset (storage, 0, storage_length);
   SAFE_FREE ();
   return list2 (output, actual_iv);
 #else
-  error ("GnuTLS AEAD cipher %ld was invalid or not found", (long) gca);
+  printmax_t print_gca = gca;
+  error ("GnuTLS AEAD cipher %"pMd" is invalid or not found", print_gca);
 #endif
 }
 
@@ -1980,9 +1961,7 @@ gnutls_symmetric (bool encrypting, Lisp_Object cipher,
   CHECK_CONS (iv);
 
 
-  const char* desc = (encrypting ? "encrypt" : "decrypt");
-
-  int ret = GNUTLS_E_SUCCESS;
+  const char *desc = encrypting ? "encrypt" : "decrypt";
 
   gnutls_cipher_algorithm_t gca = GNUTLS_CIPHER_UNKNOWN;
 
@@ -1992,7 +1971,7 @@ gnutls_symmetric (bool encrypting, Lisp_Object cipher,
 
   if (SYMBOLP (cipher))
     info = XCDR (Fassq (cipher, Fgnutls_ciphers ()));
-  else if (INTEGERP (cipher))
+  else if (TYPE_RANGED_INTEGERP (gnutls_cipher_algorithm_t, cipher))
     gca = XINT (cipher);
   else
     info = cipher;
@@ -2000,41 +1979,44 @@ gnutls_symmetric (bool encrypting, Lisp_Object cipher,
   if (!NILP (info) && CONSP (info))
     {
       Lisp_Object v = Fplist_get (info, QCcipher_id);
-      if (INTEGERP (v))
+      if (TYPE_RANGED_INTEGERP (gnutls_cipher_algorithm_t, v))
         gca = XINT (v);
     }
 
-  if (gca == GNUTLS_CIPHER_UNKNOWN)
-    error ("GnuTLS cipher was invalid or not found");
+  ptrdiff_t key_size = gnutls_cipher_get_key_size (gca);
+  if (key_size == 0)
+    error ("GnuTLS cipher is invalid or not found");
 
   ptrdiff_t kstart_byte, kend_byte;
-  const char* kdata = extract_data_from_object (key, &kstart_byte, &kend_byte);
+  const char *kdata = extract_data_from_object (key, &kstart_byte, &kend_byte);
 
   if (kdata == NULL)
     error ("GnuTLS cipher key extraction failed");
 
-  if ((kend_byte - kstart_byte) != gnutls_cipher_get_key_size (gca))
-    error ("GnuTLS cipher %s/%s key length %" pD "d was not equal to "
-           "the required %ld",
+  if (kend_byte - kstart_byte != key_size)
+    error (("GnuTLS cipher %s/%s key length %"pD"d is not equal to "
+	    "the required %"pD"d"),
            gnutls_cipher_get_name (gca), desc,
-           kend_byte - kstart_byte, (long) gnutls_cipher_get_key_size (gca));
+	   kend_byte - kstart_byte, key_size);
 
   ptrdiff_t vstart_byte, vend_byte;
-  const char* vdata = extract_data_from_object (iv, &vstart_byte, &vend_byte);
+  char *vdata = extract_data_from_object (iv, &vstart_byte, &vend_byte);
 
   if (vdata == NULL)
     error ("GnuTLS cipher IV extraction failed");
 
-  if ((vend_byte - vstart_byte) != gnutls_cipher_get_iv_size (gca))
-    error ("GnuTLS cipher %s/%s IV length %" pD "d was not equal to "
-           "the required %ld",
+  ptrdiff_t iv_size = gnutls_cipher_get_iv_size (gca);
+  if (vend_byte - vstart_byte != iv_size)
+    error (("GnuTLS cipher %s/%s IV length %"pD"d is not equal to "
+	    "the required %"pD"d"),
            gnutls_cipher_get_name (gca), desc,
-           vend_byte - vstart_byte, (long) gnutls_cipher_get_iv_size (gca));
+	   vend_byte - vstart_byte, iv_size);
 
   Lisp_Object actual_iv = make_unibyte_string (vdata, vend_byte - vstart_byte);
 
   ptrdiff_t istart_byte, iend_byte;
-  const char* idata = extract_data_from_object (input, &istart_byte, &iend_byte);
+  const char *idata
+    = extract_data_from_object (input, &istart_byte, &iend_byte);
 
   if (idata == NULL)
     error ("GnuTLS cipher input extraction failed");
@@ -2053,44 +2035,34 @@ gnutls_symmetric (bool encrypting, Lisp_Object cipher,
       return aead_output;
     }
 
-  if ((iend_byte - istart_byte) % gnutls_cipher_get_block_size (gca) != 0)
-    error ("GnuTLS cipher %s/%s input block length %" pD "d was not a multiple "
-           "of the required %ld",
+  ptrdiff_t cipher_block_size = gnutls_cipher_get_block_size (gca);
+  if ((iend_byte - istart_byte) % cipher_block_size != 0)
+    error (("GnuTLS cipher %s/%s input block length %"pD"d is not a multiple "
+	    "of the required %"pD"d"),
            gnutls_cipher_get_name (gca), desc,
-           iend_byte - istart_byte, (long) gnutls_cipher_get_block_size (gca));
+	   iend_byte - istart_byte, cipher_block_size);
 
   gnutls_cipher_hd_t hcipher;
-  gnutls_datum_t key_datum = { (unsigned char*) kdata, kend_byte - kstart_byte };
+  gnutls_datum_t key_datum
+    = { (unsigned char *) kdata, kend_byte - kstart_byte };
 
-  ret = gnutls_cipher_init (&hcipher, gca, &key_datum, NULL);
+  int ret = gnutls_cipher_init (&hcipher, gca, &key_datum, NULL);
 
   if (ret < GNUTLS_E_SUCCESS)
-    {
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
-      error ("GnuTLS cipher %s/%s initialization failed: %s",
-             gnutls_cipher_get_name (gca), desc, str);
-    }
+    error ("GnuTLS cipher %s/%s initialization failed: %s",
+	   gnutls_cipher_get_name (gca), desc, emacs_gnutls_strerror (ret));
 
   /* Note that this will not support streaming block mode. */
-  gnutls_cipher_set_iv (hcipher, (void*) vdata, vend_byte - vstart_byte);
+  gnutls_cipher_set_iv (hcipher, vdata, vend_byte - vstart_byte);
 
-  /*
-   * GnuTLS docs: "For the supported ciphers the encrypted data length
-   * will equal the plaintext size."
-   */
-  size_t storage_length = iend_byte - istart_byte;
+  /* GnuTLS docs: "For the supported ciphers the encrypted data length
+     will equal the plaintext size."  */
+  ptrdiff_t storage_length = iend_byte - istart_byte;
   Lisp_Object storage = make_uninit_string (storage_length);
 
-  if (encrypting)
-    ret = gnutls_cipher_encrypt2 (hcipher,
-                                  idata, iend_byte - istart_byte,
-                                  SSDATA (storage), storage_length);
-  else
-    ret = gnutls_cipher_decrypt2 (hcipher,
-                                  idata, iend_byte - istart_byte,
-                                  SSDATA (storage), storage_length);
+  ret = ((encrypting ? gnutls_cipher_encrypt2 : gnutls_cipher_decrypt2)
+	 (hcipher, idata, iend_byte - istart_byte,
+	  SSDATA (storage), storage_length));
 
   if (STRINGP (XCAR (key)))
     Fclear_string (XCAR (key));
@@ -2098,11 +2070,8 @@ gnutls_symmetric (bool encrypting, Lisp_Object cipher,
   if (ret < GNUTLS_E_SUCCESS)
     {
       gnutls_cipher_deinit (hcipher);
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
       error ("GnuTLS cipher %s %sion failed: %s",
-             gnutls_cipher_get_name (gca), desc, str);
+	     gnutls_cipher_get_name (gca), desc, emacs_gnutls_strerror (ret));
     }
 
   gnutls_cipher_deinit (hcipher);
@@ -2110,41 +2079,46 @@ gnutls_symmetric (bool encrypting, Lisp_Object cipher,
   return list2 (storage, actual_iv);
 }
 
-DEFUN ("gnutls-symmetric-encrypt", Fgnutls_symmetric_encrypt, Sgnutls_symmetric_encrypt, 4, 5, 0,
+DEFUN ("gnutls-symmetric-encrypt", Fgnutls_symmetric_encrypt,
+       Sgnutls_symmetric_encrypt, 4, 5, 0,
        doc: /* Encrypt INPUT with symmetric CIPHER, KEY+AEAD_AUTH, and IV to a unibyte string.
 
-Returns nil on error.
+Return nil on error.
 
-The KEY can be specified as a buffer or string or in other ways
-(see Info node `(elisp)Format of GnuTLS Cryptography Inputs').  The KEY will be
-wiped after use if it's a string.
+The KEY can be specified as a buffer or string or in other ways (see
+Info node `(elisp)Format of GnuTLS Cryptography Inputs').  The KEY
+will be wiped after use if it's a string.
 
-The IV and INPUT and the optional AEAD_AUTH can be
-specified as a buffer or string or in other ways (see Info node `(elisp)Format of GnuTLS Cryptography Inputs').
+The IV and INPUT and the optional AEAD_AUTH can be specified as a
+buffer or string or in other ways (see Info node `(elisp)Format of
+GnuTLS Cryptography Inputs').
 
 The alist of symmetric ciphers can be obtained with `gnutls-ciphers`.
 The CIPHER may be a string or symbol matching a key in that alist, or
-a plist with the `:cipher-id' numeric property, or the number itself.
+a plist with the :cipher-id numeric property, or the number itself.
 
 AEAD ciphers: these ciphers will have a `gnutls-ciphers' entry with
 :cipher-aead-capable set to t.  AEAD_AUTH can be supplied for
 these AEAD ciphers, but it may still be omitted (nil) as well. */)
-     (Lisp_Object cipher, Lisp_Object key, Lisp_Object iv, Lisp_Object input, Lisp_Object aead_auth)
+  (Lisp_Object cipher, Lisp_Object key, Lisp_Object iv,
+   Lisp_Object input, Lisp_Object aead_auth)
 {
   return gnutls_symmetric (true, cipher, key, iv, input, aead_auth);
 }
 
-DEFUN ("gnutls-symmetric-decrypt", Fgnutls_symmetric_decrypt, Sgnutls_symmetric_decrypt, 4, 5, 0,
+DEFUN ("gnutls-symmetric-decrypt", Fgnutls_symmetric_decrypt,
+       Sgnutls_symmetric_decrypt, 4, 5, 0,
        doc: /* Decrypt INPUT with symmetric CIPHER, KEY+AEAD_AUTH, and IV to a unibyte string.
 
-Returns nil on error.
+Return nil on error.
 
-The KEY can be specified as a buffer or string or in other ways
-(see Info node `(elisp)Format of GnuTLS Cryptography Inputs').  The KEY will be
-wiped after use if it's a string.
+The KEY can be specified as a buffer or string or in other ways (see
+Info node `(elisp)Format of GnuTLS Cryptography Inputs').  The KEY
+will be wiped after use if it's a string.
 
-The IV and INPUT and the optional AEAD_AUTH can be
-specified as a buffer or string or in other ways (see Info node `(elisp)Format of GnuTLS Cryptography Inputs').
+The IV and INPUT and the optional AEAD_AUTH can be specified as a
+buffer or string or in other ways (see Info node `(elisp)Format of
+GnuTLS Cryptography Inputs').
 
 The alist of symmetric ciphers can be obtained with `gnutls-ciphers`.
 The CIPHER may be a string or symbol matching a key in that alist, or
@@ -2153,7 +2127,8 @@ a plist with the `:cipher-id' numeric property, or the number itself.
 AEAD ciphers: these ciphers will have a `gnutls-ciphers' entry with
 :cipher-aead-capable set to t.  AEAD_AUTH can be supplied for
 these AEAD ciphers, but it may still be omitted (nil) as well. */)
-     (Lisp_Object cipher, Lisp_Object key, Lisp_Object iv, Lisp_Object input, Lisp_Object aead_auth)
+  (Lisp_Object cipher, Lisp_Object key, Lisp_Object iv,
+   Lisp_Object input, Lisp_Object aead_auth)
 {
   return gnutls_symmetric (false, cipher, key, iv, input, aead_auth);
 }
@@ -2164,32 +2139,26 @@ DEFUN ("gnutls-macs", Fgnutls_macs, Sgnutls_macs, 0, 0, 0,
 Use the value of the alist (extract it with `alist-get' for instance)
 with `gnutls-hash-mac'.  The alist key is the mac-algorithm method
 name. */)
-     (void)
+  (void)
 {
   Lisp_Object mac_algorithms = Qnil;
-  const gnutls_mac_algorithm_t* macs = gnutls_mac_list ();
-  for (size_t pos = 0; macs[pos] != 0; pos++)
+  const gnutls_mac_algorithm_t *macs = gnutls_mac_list ();
+  for (ptrdiff_t pos = 0; macs[pos] != 0; pos++)
     {
       const gnutls_mac_algorithm_t gma = macs[pos];
 
-      const char* name = gnutls_mac_get_name (gma);
-
-      Lisp_Object mp = listn (CONSTYPE_HEAP, 11,
-                              /* A symbol representing the mac-algorithm. */
-                              intern (name),
-                              /* The internally meaningful mac-algorithm ID. */
-                              QCmac_algorithm_id,
-                              make_number (gma),
-                              /* The type (vs. other GnuTLS objects). */
-                              QCtype,
-                              Qgnutls_type_mac_algorithm,
-                              /* The output length. */
+      const char *name = gnutls_mac_get_name (gma);
+
+      Lisp_Object mp = listn (CONSTYPE_HEAP, 11, intern (name),
+			      QCmac_algorithm_id, make_number (gma),
+			      QCtype, Qgnutls_type_mac_algorithm,
+
                               QCmac_algorithm_length,
                               make_number (gnutls_hmac_get_len (gma)),
-                              /* The key size. */
+
                               QCmac_algorithm_keysize,
                               make_number (gnutls_mac_get_key_size (gma)),
-                              /* The nonce size. */
+
                               QCmac_algorithm_noncesize,
                               make_number (gnutls_mac_get_nonce_size (gma)));
       mac_algorithms = Fcons (mp, mac_algorithms);
@@ -2204,25 +2173,20 @@ DEFUN ("gnutls-digests", Fgnutls_digests, Sgnutls_digests, 0, 0, 0,
 Use the value of the alist (extract it with `alist-get' for instance)
 with `gnutls-hash-digest'.  The alist key is the digest-algorithm
 method name. */)
-     (void)
+  (void)
 {
   Lisp_Object digest_algorithms = Qnil;
-  const gnutls_digest_algorithm_t* digests = gnutls_digest_list ();
-  for (size_t pos = 0; digests[pos] != 0; pos++)
+  const gnutls_digest_algorithm_t *digests = gnutls_digest_list ();
+  for (ptrdiff_t pos = 0; digests[pos] != 0; pos++)
     {
       const gnutls_digest_algorithm_t gda = digests[pos];
 
-      const char* name = gnutls_digest_get_name (gda);
-
-      Lisp_Object mp = listn (CONSTYPE_HEAP, 7,
-                              /* A symbol representing the digest-algorithm. */
-                              intern (name),
-                              /* The internally meaningful digest-algorithm ID. */
-                              QCdigest_algorithm_id,
-                              make_number (gda),
-                              QCtype,
-                              Qgnutls_type_digest_algorithm,
-                              /* The digest length. */
+      const char *name = gnutls_digest_get_name (gda);
+
+      Lisp_Object mp = listn (CONSTYPE_HEAP, 7, intern (name),
+			      QCdigest_algorithm_id, make_number (gda),
+			      QCtype, Qgnutls_type_digest_algorithm,
+
                               QCdigest_algorithm_length,
                               make_number (gnutls_hash_get_len (gda)));
 
@@ -2235,11 +2199,11 @@ method name. */)
 DEFUN ("gnutls-hash-mac", Fgnutls_hash_mac, Sgnutls_hash_mac, 3, 3, 0,
        doc: /* Hash INPUT with HASH-METHOD and KEY into a unibyte string.
 
-Returns nil on error.
+Return nil on error.
 
-The KEY can be specified as a buffer or string or in other ways
-(see Info node `(elisp)Format of GnuTLS Cryptography Inputs').  The KEY will be
-wiped after use if it's a string.
+The KEY can be specified as a buffer or string or in other ways (see
+Info node `(elisp)Format of GnuTLS Cryptography Inputs').  The KEY
+will be wiped after use if it's a string.
 
 The INPUT can be specified as a buffer or string or in other
 ways (see Info node `(elisp)Format of GnuTLS Cryptography Inputs').
@@ -2248,7 +2212,7 @@ The alist of MAC algorithms can be obtained with `gnutls-macs`.  The
 HASH-METHOD may be a string or symbol matching a key in that alist, or
 a plist with the `:mac-algorithm-id' numeric property, or the number
 itself. */)
-     (Lisp_Object hash_method, Lisp_Object key, Lisp_Object input)
+  (Lisp_Object hash_method, Lisp_Object key, Lisp_Object input)
 {
   if (BUFFERP (input) || STRINGP (input))
     input = list1 (input);
@@ -2260,8 +2224,6 @@ itself. */)
 
   CHECK_CONS (key);
 
-  int ret = GNUTLS_E_SUCCESS;
-
   gnutls_mac_algorithm_t gma = GNUTLS_MAC_UNKNOWN;
 
   Lisp_Object info = Qnil;
@@ -2270,7 +2232,7 @@ itself. */)
 
   if (SYMBOLP (hash_method))
     info = XCDR (Fassq (hash_method, Fgnutls_macs ()));
-  else if (INTEGERP (hash_method))
+  else if (TYPE_RANGED_INTEGERP (gnutls_mac_algorithm_t, hash_method))
     gma = XINT (hash_method);
   else
     info = hash_method;
@@ -2278,37 +2240,32 @@ itself. */)
   if (!NILP (info) && CONSP (info))
     {
       Lisp_Object v = Fplist_get (info, QCmac_algorithm_id);
-      if (INTEGERP (v))
+      if (TYPE_RANGED_INTEGERP (gnutls_mac_algorithm_t, v))
         gma = XINT (v);
     }
 
-  if (gma == GNUTLS_MAC_UNKNOWN)
-    error ("GnuTLS MAC-method was invalid or not found");
+  ptrdiff_t digest_length = gnutls_hmac_get_len (gma);
+  if (digest_length == 0)
+    error ("GnuTLS MAC-method is invalid or not found");
 
   ptrdiff_t kstart_byte, kend_byte;
-  const char* kdata = extract_data_from_object (key, &kstart_byte, &kend_byte);
-  gnutls_hmac_hd_t hmac;
-  ret = gnutls_hmac_init (&hmac, gma,
-                          kdata + kstart_byte, kend_byte - kstart_byte);
-
+  const char *kdata = extract_data_from_object (key, &kstart_byte, &kend_byte);
   if (kdata == NULL)
     error ("GnuTLS MAC key extraction failed");
 
+  gnutls_hmac_hd_t hmac;
+  int ret = gnutls_hmac_init (&hmac, gma,
+			      kdata + kstart_byte, kend_byte - kstart_byte);
   if (ret < GNUTLS_E_SUCCESS)
-    {
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
-      error ("GnuTLS MAC %s initialization failed: %s",
-             gnutls_mac_get_name (gma), str);
-    }
+    error ("GnuTLS MAC %s initialization failed: %s",
+	   gnutls_mac_get_name (gma), emacs_gnutls_strerror (ret));
 
   ptrdiff_t istart_byte, iend_byte;
-  const char* idata = extract_data_from_object (input, &istart_byte, &iend_byte);
+  const char *idata
+    = extract_data_from_object (input, &istart_byte, &iend_byte);
   if (idata == NULL)
     error ("GnuTLS MAC input extraction failed");
 
-  size_t digest_length = gnutls_hmac_get_len (gma);
   Lisp_Object digest = make_uninit_string (digest_length);
 
   ret = gnutls_hmac (hmac, idata + istart_byte, iend_byte - istart_byte);
@@ -2319,12 +2276,8 @@ itself. */)
   if (ret < GNUTLS_E_SUCCESS)
     {
       gnutls_hmac_deinit (hmac, NULL);
-
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
       error ("GnuTLS MAC %s application failed: %s",
-             gnutls_mac_get_name (gma), str);
+	     gnutls_mac_get_name (gma), emacs_gnutls_strerror (ret));
     }
 
   gnutls_hmac_output (hmac, SSDATA (digest));
@@ -2336,7 +2289,7 @@ itself. */)
 DEFUN ("gnutls-hash-digest", Fgnutls_hash_digest, Sgnutls_hash_digest, 2, 2, 0,
        doc: /* Digest INPUT with DIGEST-METHOD into a unibyte string.
 
-Returns nil on error.
+Return nil on error.
 
 The INPUT can be specified as a buffer or string or in other
 ways (see Info node `(elisp)Format of GnuTLS Cryptography Inputs').
@@ -2345,15 +2298,13 @@ The alist of digest algorithms can be obtained with `gnutls-digests`.
 The DIGEST-METHOD may be a string or symbol matching a key in that
 alist, or a plist with the `:digest-algorithm-id' numeric property, or
 the number itself. */)
-     (Lisp_Object digest_method, Lisp_Object input)
+  (Lisp_Object digest_method, Lisp_Object input)
 {
   if (BUFFERP (input) || STRINGP (input))
     input = list1 (input);
 
   CHECK_CONS (input);
 
-  int ret = GNUTLS_E_SUCCESS;
-
   gnutls_digest_algorithm_t gda = GNUTLS_DIG_UNKNOWN;
 
   Lisp_Object info = Qnil;
@@ -2362,7 +2313,7 @@ the number itself. */)
 
   if (SYMBOLP (digest_method))
     info = XCDR (Fassq (digest_method, Fgnutls_digests ()));
-  else if (INTEGERP (digest_method))
+  else if (TYPE_RANGED_INTEGERP (gnutls_digest_algorithm_t, digest_method))
     gda = XINT (digest_method);
   else
     info = digest_method;
@@ -2370,29 +2321,26 @@ the number itself. */)
   if (!NILP (info) && CONSP (info))
     {
       Lisp_Object v = Fplist_get (info, QCdigest_algorithm_id);
-      if (INTEGERP (v))
+      if (TYPE_RANGED_INTEGERP (gnutls_digest_algorithm_t, v))
         gda = XINT (v);
     }
 
-  if (gda == GNUTLS_DIG_UNKNOWN)
-    error ("GnuTLS digest-method was invalid or not found");
+  ptrdiff_t digest_length = gnutls_hash_get_len (gda);
+  if (digest_length == 0)
+    error ("GnuTLS digest-method is invalid or not found");
 
   gnutls_hash_hd_t hash;
-  ret = gnutls_hash_init (&hash, gda);
+  int ret = gnutls_hash_init (&hash, gda);
 
   if (ret < GNUTLS_E_SUCCESS)
-    {
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
-      error ("GnuTLS digest initialization failed: %s", str);
-    }
+    error ("GnuTLS digest initialization failed: %s",
+	   emacs_gnutls_strerror (ret));
 
-  size_t digest_length = gnutls_hash_get_len (gda);
   Lisp_Object digest = make_uninit_string (digest_length);
 
   ptrdiff_t istart_byte, iend_byte;
-  const char* idata = extract_data_from_object (input, &istart_byte, &iend_byte);
+  const char *idata
+    = extract_data_from_object (input, &istart_byte, &iend_byte);
   if (idata == NULL)
     error ("GnuTLS digest input extraction failed");
 
@@ -2401,11 +2349,8 @@ the number itself. */)
   if (ret < GNUTLS_E_SUCCESS)
     {
       gnutls_hash_deinit (hash, NULL);
-
-      const char* str = gnutls_strerror (ret);
-      if (!str)
-        str = "unknown";
-      error ("GnuTLS digest application failed: %s", str);
+      error ("GnuTLS digest application failed: %s",
+	     emacs_gnutls_strerror (ret));
     }
 
   gnutls_hash_output (hash, SSDATA (digest));
@@ -2420,57 +2365,51 @@ DEFUN ("gnutls-available-p", Fgnutls_available_p, Sgnutls_available_p, 0, 0, 0,
        doc: /* Return list of capabilities if GnuTLS is available in this instance of Emacs.
 
 ...if supported         : then...
-GnuTLS 3 or higher      : the list will contain 'gnutls3.
-GnuTLS MACs             : the list will contain 'macs.
-GnuTLS digests          : the list will contain 'digests.
-GnuTLS symmetric ciphers: the list will contain 'ciphers.
-GnuTLS AEAD ciphers     : the list will contain 'AEAD-ciphers.  */)
-     (void)
+GnuTLS 3 or higher      : the list will contain `gnutls3'.
+GnuTLS MACs             : the list will contain `macs'.
+GnuTLS digests          : the list will contain `digests'.
+GnuTLS symmetric ciphers: the list will contain `ciphers'.
+GnuTLS AEAD ciphers     : the list will contain `AEAD-ciphers'.  */)
+  (void)
 {
-#ifdef HAVE_GNUTLS
-  Lisp_Object capabilities = Qnil;
+#ifdef WINDOWSNT
+  Lisp_Object found = Fassq (Qgnutls, Vlibrary_cache);
+  if (CONSP (found))
+    return XCDR (found); /* TODO: use capabilities.  */
+  else
+    {
+      Lisp_Object status;
+      /* TODO: should the capabilities be dynamic here?  */
+      status = init_gnutls_functions () ? capabilities : Qnil;
+      Vlibrary_cache = Fcons (Fcons (Qgnutls, status), Vlibrary_cache);
+      return status;
+    }
+#else
 
-#ifdef HAVE_GNUTLS3
+  Lisp_Object capabilities = Qnil;
 
+# ifdef HAVE_GNUTLS3
   capabilities = Fcons (intern("gnutls3"), capabilities);
 
-#ifdef HAVE_GNUTLS3_DIGEST
+#  ifdef HAVE_GNUTLS3_DIGEST
   capabilities = Fcons (intern("digests"), capabilities);
-#endif
+#  endif
 
-#ifdef HAVE_GNUTLS3_CIPHER
+#  ifdef HAVE_GNUTLS3_CIPHER
   capabilities = Fcons (intern("ciphers"), capabilities);
 
-#ifdef HAVE_GNUTLS3_AEAD
+#   ifdef HAVE_GNUTLS3_AEAD
   capabilities = Fcons (intern("AEAD-ciphers"), capabilities);
-#endif
+#   endif
 
-#ifdef HAVE_GNUTLS3_HMAC
+#   ifdef HAVE_GNUTLS3_HMAC
   capabilities = Fcons (intern("macs"), capabilities);
-#endif
-
-#endif
-
-#endif
+#   endif
+#  endif
+# endif
 
-# ifdef WINDOWSNT
-  Lisp_Object found = Fassq (Qgnutls, Vlibrary_cache);
-  if (CONSP (found))
-    return XCDR (found); // TODO: use capabilities.
-  else
-    {
-      Lisp_Object status;
-      // TODO: should the capabilities be dynamic here?
-      status = init_gnutls_functions () ? capabilities : Qnil;
-      Vlibrary_cache = Fcons (Fcons (Qgnutls, status), Vlibrary_cache);
-      return status;
-    }
-# else	/* !WINDOWSNT */
   return capabilities;
-# endif	 /* !WINDOWSNT */
-#else  /* !HAVE_GNUTLS */
-  return Qnil;
-#endif	/* !HAVE_GNUTLS */
+#endif
 }
 
 void
diff --git a/src/lisp.h b/src/lisp.h
index a5134a9532..9464bf8559 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3386,9 +3386,7 @@ enum { NEXT_ALMOST_PRIME_LIMIT = 11 };
 extern EMACS_INT next_almost_prime (EMACS_INT) ATTRIBUTE_CONST;
 extern Lisp_Object larger_vector (Lisp_Object, ptrdiff_t, ptrdiff_t);
 extern void sweep_weak_hash_tables (void);
-extern const char* extract_data_from_object (Lisp_Object spec,
-                                             ptrdiff_t *start_byte,
-                                             ptrdiff_t *end_byte);
+extern char *extract_data_from_object (Lisp_Object, ptrdiff_t *, ptrdiff_t *);
 EMACS_UINT hash_string (char const *, ptrdiff_t);
 EMACS_UINT sxhash (Lisp_Object, int);
 Lisp_Object make_hash_table (struct hash_table_test, EMACS_INT, float, float,
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: GnuTLS and zeroing keys in Emacs
  2017-07-14 23:42 GnuTLS and zeroing keys in Emacs Paul Eggert
@ 2017-07-15 19:00 ` Ted Zlatanov
  2017-07-16 23:53   ` Paul Eggert
  2017-07-17  0:29   ` Paul Eggert
  0 siblings, 2 replies; 6+ messages in thread
From: Ted Zlatanov @ 2017-07-15 19:00 UTC (permalink / raw)
  To: emacs-devel

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: GnuTLS and zeroing keys in Emacs
  2017-07-15 19:00 ` Ted Zlatanov
@ 2017-07-16 23:53   ` Paul Eggert
  2017-07-17 13:52     ` Ted Zlatanov
  2017-07-17  0:29   ` Paul Eggert
  1 sibling, 1 reply; 6+ messages in thread
From: Paul Eggert @ 2017-07-16 23:53 UTC (permalink / raw)
  To: emacs-devel

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

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Merge-from-gnulib.patch --]
[-- Type: text/x-patch; name="0001-Merge-from-gnulib.patch", Size: 8235 bytes --]

From 252444aaa3a7cb9fc70289a5a3920f8a9d848109 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 16 Jul 2017 16:22:33 -0700
Subject: [PATCH 1/3] Merge from gnulib

This incorporates:
2017-07-16 explicit_bzero: new module
2017-07-15 getdtablesize: Add minimal support for OpenVMS.
* lib/getdtablesize.c, lib/string.in.h, m4/getdtablesize.m4:
* m4/string_h.m4:
Copy from Gnulib.
* lib/gnulib.mk.in: Regenerate.
---
 lib/getdtablesize.c |  2 +-
 lib/gnulib.mk.in    |  6 +++++-
 lib/string.in.h     | 17 +++++++++++++++++
 m4/getdtablesize.m4 | 54 +++++++++++++++++++++++++++++++++--------------------
 m4/string_h.m4      |  2 ++
 5 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/lib/getdtablesize.c b/lib/getdtablesize.c
index c356cf4..a092863 100644
--- a/lib/getdtablesize.c
+++ b/lib/getdtablesize.c
@@ -1,4 +1,4 @@
-/* getdtablesize() function for platforms that don't have it.
+/* getdtablesize() function: Return maximum possible file descriptor value + 1.
    Copyright (C) 2008-2017 Free Software Foundation, Inc.
    Written by Bruno Haible <bruno@clisp.org>, 2008.
 
diff --git a/lib/gnulib.mk.in b/lib/gnulib.mk.in
index fd0f9e5..ae5ae87 100644
--- a/lib/gnulib.mk.in
+++ b/lib/gnulib.mk.in
@@ -125,6 +125,7 @@ GNULIB_DUP2 = @GNULIB_DUP2@
 GNULIB_DUP3 = @GNULIB_DUP3@
 GNULIB_ENVIRON = @GNULIB_ENVIRON@
 GNULIB_EUIDACCESS = @GNULIB_EUIDACCESS@
+GNULIB_EXPLICIT_BZERO = @GNULIB_EXPLICIT_BZERO@
 GNULIB_FACCESSAT = @GNULIB_FACCESSAT@
 GNULIB_FCHDIR = @GNULIB_FCHDIR@
 GNULIB_FCHMODAT = @GNULIB_FCHMODAT@
@@ -390,6 +391,7 @@ HAVE_DPRINTF = @HAVE_DPRINTF@
 HAVE_DUP2 = @HAVE_DUP2@
 HAVE_DUP3 = @HAVE_DUP3@
 HAVE_EUIDACCESS = @HAVE_EUIDACCESS@
+HAVE_EXPLICIT_BZERO = @HAVE_EXPLICIT_BZERO@
 HAVE_FACCESSAT = @HAVE_FACCESSAT@
 HAVE_FCHDIR = @HAVE_FCHDIR@
 HAVE_FCHMODAT = @HAVE_FCHMODAT@
@@ -2411,6 +2413,7 @@ string.h: string.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNULL_H
 	      -e 's|@''PRAGMA_SYSTEM_HEADER''@|@PRAGMA_SYSTEM_HEADER@|g' \
 	      -e 's|@''PRAGMA_COLUMNS''@|@PRAGMA_COLUMNS@|g' \
 	      -e 's|@''NEXT_STRING_H''@|$(NEXT_STRING_H)|g' \
+	      -e 's/@''GNULIB_EXPLICIT_BZERO''@/$(GNULIB_EXPLICIT_BZERO)/g' \
 	      -e 's/@''GNULIB_FFSL''@/$(GNULIB_FFSL)/g' \
 	      -e 's/@''GNULIB_FFSLL''@/$(GNULIB_FFSLL)/g' \
 	      -e 's/@''GNULIB_MBSLEN''@/$(GNULIB_MBSLEN)/g' \
@@ -2449,7 +2452,8 @@ string.h: string.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNULL_H
 	      -e 's/@''GNULIB_STRSIGNAL''@/$(GNULIB_STRSIGNAL)/g' \
 	      -e 's/@''GNULIB_STRVERSCMP''@/$(GNULIB_STRVERSCMP)/g' \
 	      < $(srcdir)/string.in.h | \
-	  sed -e 's|@''HAVE_FFSL''@|$(HAVE_FFSL)|g' \
+	  sed -e 's|@''HAVE_EXPLICIT_BZERO''@|$(HAVE_EXPLICIT_BZERO)|g' \
+	      -e 's|@''HAVE_FFSL''@|$(HAVE_FFSL)|g' \
 	      -e 's|@''HAVE_FFSLL''@|$(HAVE_FFSLL)|g' \
 	      -e 's|@''HAVE_MBSLEN''@|$(HAVE_MBSLEN)|g' \
 	      -e 's|@''HAVE_MEMCHR''@|$(HAVE_MEMCHR)|g' \
diff --git a/lib/string.in.h b/lib/string.in.h
index 9a6b311..aaff563 100644
--- a/lib/string.in.h
+++ b/lib/string.in.h
@@ -74,6 +74,23 @@
 /* The definition of _GL_WARN_ON_USE is copied here.  */
 
 
+/* Clear a block of memory.  The compiler will not delete a call to
+   this function, even if the block is dead after the call.  */
+#if @GNULIB_EXPLICIT_BZERO@
+# if ! @HAVE_EXPLICIT_BZERO@
+_GL_FUNCDECL_SYS (explicit_bzero, void,
+                  (void *__dest, size_t __n) _GL_ARG_NONNULL ((1)));
+# endif
+_GL_CXXALIAS_SYS (explicit_bzero, void, (void *__dest, size_t __n));
+_GL_CXXALIASWARN (explicit_bzero);
+#elif defined GNULIB_POSIXCHECK
+# undef explicit_bzero
+# if HAVE_RAW_DECL_EXPLICIT_BZERO
+_GL_WARN_ON_USE (explicit_bzero, "explicit_bzero is unportable - "
+                 "use gnulib module explicit_bzero for portability");
+# endif
+#endif
+
 /* Find the index of the least-significant set bit.  */
 #if @GNULIB_FFSL@
 # if !@HAVE_FFSL@
diff --git a/m4/getdtablesize.m4 b/m4/getdtablesize.m4
index 1af2a24..f1e4f5f 100644
--- a/m4/getdtablesize.m4
+++ b/m4/getdtablesize.m4
@@ -1,4 +1,4 @@
-# getdtablesize.m4 serial 6
+# getdtablesize.m4 serial 7
 dnl Copyright (C) 2008-2017 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -12,29 +12,43 @@ AC_DEFUN
   AC_CHECK_DECLS_ONCE([getdtablesize])
   if test $ac_cv_func_getdtablesize = yes &&
      test $ac_cv_have_decl_getdtablesize = yes; then
-    # Cygwin 1.7.25 automatically increases the RLIMIT_NOFILE soft limit
-    # up to an unchangeable hard limit; all other platforms correctly
-    # require setrlimit before getdtablesize() can report a larger value.
     AC_CACHE_CHECK([whether getdtablesize works],
       [gl_cv_func_getdtablesize_works],
-      [AC_RUN_IFELSE([
-        AC_LANG_PROGRAM([[#include <unistd.h>]],
-          [int size = getdtablesize();
-           if (dup2 (0, getdtablesize()) != -1)
-             return 1;
-           if (size != getdtablesize())
-             return 2;
-          ])],
-        [gl_cv_func_getdtablesize_works=yes],
-        [gl_cv_func_getdtablesize_works=no],
-        [case "$host_os" in
-          cygwin*) # on cygwin 1.5.25, getdtablesize() automatically grows
-            gl_cv_func_getdtablesize_works="guessing no" ;;
-          *) gl_cv_func_getdtablesize_works="guessing yes" ;;
-         esac])
+      [dnl There are two concepts: the "maximum possible file descriptor value + 1"
+       dnl and the "maximum number of open file descriptors in a process".
+       dnl Per SUSv2 and POSIX, getdtablesize() should return the first one.
+       dnl On most platforms, the first and the second concept are the same.
+       dnl On OpenVMS, however, they are different and getdtablesize() returns
+       dnl the second one; thus the test below fails. But we don't care
+       dnl because there's no good way to write a replacement getdtablesize().
+       case "$host_os" in
+         vms*) gl_cv_func_getdtablesize_works="no (limitation)" ;;
+         *)
+           dnl Cygwin 1.7.25 automatically increases the RLIMIT_NOFILE soft
+           dnl limit up to an unchangeable hard limit; all other platforms
+           dnl correctly require setrlimit before getdtablesize() can report
+           dnl a larger value.
+           AC_RUN_IFELSE([
+             AC_LANG_PROGRAM([[#include <unistd.h>]],
+               [int size = getdtablesize();
+                if (dup2 (0, getdtablesize()) != -1)
+                  return 1;
+                if (size != getdtablesize())
+                  return 2;
+               ])],
+             [gl_cv_func_getdtablesize_works=yes],
+             [gl_cv_func_getdtablesize_works=no],
+             [case "$host_os" in
+                cygwin*) # on cygwin 1.5.25, getdtablesize() automatically grows
+                  gl_cv_func_getdtablesize_works="guessing no" ;;
+                *) gl_cv_func_getdtablesize_works="guessing yes" ;;
+              esac
+             ])
+           ;;
+       esac
       ])
     case "$gl_cv_func_getdtablesize_works" in
-      *yes) ;;
+      *yes | "no (limitation)") ;;
       *) REPLACE_GETDTABLESIZE=1 ;;
     esac
   else
diff --git a/m4/string_h.m4 b/m4/string_h.m4
index 3d2ad22..ac6311f 100644
--- a/m4/string_h.m4
+++ b/m4/string_h.m4
@@ -43,6 +43,7 @@ AC_DEFUN
 
 AC_DEFUN([gl_HEADER_STRING_H_DEFAULTS],
 [
+  GNULIB_EXPLICIT_BZERO=0; AC_SUBST([GNULIB_EXPLICIT_BZERO])
   GNULIB_FFSL=0;        AC_SUBST([GNULIB_FFSL])
   GNULIB_FFSLL=0;       AC_SUBST([GNULIB_FFSLL])
   GNULIB_MEMCHR=0;      AC_SUBST([GNULIB_MEMCHR])
@@ -82,6 +83,7 @@ AC_DEFUN
   GNULIB_STRVERSCMP=0;  AC_SUBST([GNULIB_STRVERSCMP])
   HAVE_MBSLEN=0;        AC_SUBST([HAVE_MBSLEN])
   dnl Assume proper GNU behavior unless another module says otherwise.
+  HAVE_EXPLICIT_BZERO=1;        AC_SUBST([HAVE_EXPLICIT_BZERO])
   HAVE_FFSL=1;                  AC_SUBST([HAVE_FFSL])
   HAVE_FFSLL=1;                 AC_SUBST([HAVE_FFSLL])
   HAVE_MEMCHR=1;                AC_SUBST([HAVE_MEMCHR])
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Use-explicit_bzero-to-clear-GnuTLS-keys.patch --]
[-- Type: text/x-patch; name="0002-Use-explicit_bzero-to-clear-GnuTLS-keys.patch", Size: 10483 bytes --]

From 59f6972134f312863dc761bf66a954a8036d0d86 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 16 Jul 2017 16:22:33 -0700
Subject: [PATCH 2/3] Use explicit_bzero to clear GnuTLS keys

* admin/merge-gnulib (GNULIB_MODULES): Add explicit_bzero.
* lib/explicit_bzero.c, m4/explicit_bzero.m4: New files.
* lib/gnulib.mk.in, m4/gnulib-comp.m4: Regenerate.
* src/gnutls.c (clear_storage): New function.
(gnutls_symmetric_aead): Use it instead of memset.
---
 admin/merge-gnulib   |  2 +-
 lib/explicit_bzero.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/gnulib.mk.in     | 13 ++++++++++++-
 m4/explicit_bzero.m4 | 22 ++++++++++++++++++++++
 m4/gnulib-comp.m4    |  9 +++++++++
 src/gnutls.c         | 20 ++++++++++++++++++--
 6 files changed, 110 insertions(+), 4 deletions(-)
 create mode 100644 lib/explicit_bzero.c
 create mode 100644 m4/explicit_bzero.m4

diff --git a/admin/merge-gnulib b/admin/merge-gnulib
index 85921ba..2b1a16a 100755
--- a/admin/merge-gnulib
+++ b/admin/merge-gnulib
@@ -30,7 +30,7 @@ GNULIB_MODULES=
   careadlinkat close-stream
   count-leading-zeros count-one-bits count-trailing-zeros
   crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
-  diffseq dtoastr dtotimespec dup2 environ execinfo faccessat
+  diffseq dtoastr dtotimespec dup2 environ execinfo explicit_bzero faccessat
   fcntl fcntl-h fdatasync fdopendir
   filemode filevercmp flexmember fstatat fsync
   getloadavg getopt-gnu gettime gettimeofday gitlog-to-changelog
diff --git a/lib/explicit_bzero.c b/lib/explicit_bzero.c
new file mode 100644
index 0000000..262c68f
--- /dev/null
+++ b/lib/explicit_bzero.c
@@ -0,0 +1,48 @@
+/* Erasure of sensitive data, generic implementation.
+   Copyright (C) 2016-2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public
+   License as published by the Free Software Foundation; either
+   version 3 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* An assembler implementation of explicit_bzero can be created as an
+   assembler alias of an optimized bzero implementation.
+   Architecture-specific implementations also need to define
+   __explicit_bzero_chk.  */
+
+#if !_LIBC
+# include <config.h>
+#endif
+
+#include <string.h>
+
+/* glibc-internal users use __explicit_bzero_chk, and explicit_bzero
+   redirects to that.  */
+#undef explicit_bzero
+
+/* Set LEN bytes of S to 0.  The compiler will not delete a call to
+   this function, even if S is dead after the call.  */
+void
+explicit_bzero (void *s, size_t len)
+{
+#ifdef HAVE_EXPLICIT_MEMSET
+  explicit_memset (s, 0, len);
+#else
+  memset (s, '\0', len);
+# ifdef __GNUC__
+  /* Compiler barrier.  */
+  asm volatile ("" ::: "memory");
+# endif
+#endif
+}
diff --git a/lib/gnulib.mk.in b/lib/gnulib.mk.in
index ae5ae87..e20487b 100644
--- a/lib/gnulib.mk.in
+++ b/lib/gnulib.mk.in
@@ -21,7 +21,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=close --avoid=dup --avoid=fchdir --avoid=fstat --avoid=malloc-posix --avoid=msvc-inval --avoid=msvc-nothrow --avoid=open --avoid=openat-die --avoid=opendir --avoid=raise --avoid=save-cwd --avoid=select --avoid=setenv --avoid=sigprocmask --avoid=stat --avoid=stdarg --avoid=stdbool --avoid=threadlib --avoid=tzset --avoid=unsetenv --avoid=utime --avoid=utime-h --gnu-make --makefile-name=gnulib.mk.in --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt binary-io byteswap c-ctype c-strcase careadlinkat close-stream count-leading-zeros count-one-bits count-trailing-zeros crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 diffseq dtoastr dtotimespec dup2 environ execinfo faccessat fcntl fcntl-h fdatasync fdopendir filemode filevercmp flexmember fstatat fsync getloadavg getopt-gnu gettime gettimeofday gitlog-to-changelog ignore-value intprops largefile lstat manywarnings memrchr minmax mkostemp mktime pipe2 pselect pthread_sigmask putenv qcopy-acl readlink readlinkat sig2str socklen stat-time std-gnu11 stdalign stddef stdio stpcpy strftime strtoimax symlink sys_stat sys_time time time_r time_rz timegm timer-time timespec-add timespec-sub unlocked-io update-copyright utimens vla warnings
+# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=close --avoid=dup --avoid=fchdir --avoid=fstat --avoid=malloc-posix --avoid=msvc-inval --avoid=msvc-nothrow --avoid=open --avoid=openat-die --avoid=opendir --avoid=raise --avoid=save-cwd --avoid=select --avoid=setenv --avoid=sigprocmask --avoid=stat --avoid=stdarg --avoid=stdbool --avoid=threadlib --avoid=tzset --avoid=unsetenv --avoid=utime --avoid=utime-h --gnu-make --makefile-name=gnulib.mk.in --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt binary-io byteswap c-ctype c-strcase careadlinkat close-stream count-leading-zeros count-one-bits count-trailing-zeros crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 diffseq dtoastr dtotimespec dup2 environ execinfo explicit_bzero faccessat fcntl fcntl-h fdatasync fdopendir filemode filevercmp flexmember fstatat fsync getloadavg getopt-gnu gettime gettimeofday gitlog-to-changelog ignore-value intprops largefile lstat manywarnings memrchr minmax mkostemp mktime pipe2 pselect pthread_sigmask putenv qcopy-acl readlink readlinkat sig2str socklen stat-time std-gnu11 stdalign stddef stdio stpcpy strftime strtoimax symlink sys_stat sys_time time time_r time_rz timegm timer-time timespec-add timespec-sub unlocked-io update-copyright utimens vla warnings
 
 
 MOSTLYCLEANFILES += core *.stackdump
@@ -1358,6 +1358,17 @@ EXTRA_libgnu_a_SOURCES += execinfo.c
 endif
 ## end   gnulib module execinfo
 
+## begin gnulib module explicit_bzero
+ifeq (,$(OMIT_GNULIB_MODULE_explicit_bzero))
+
+
+EXTRA_DIST += explicit_bzero.c
+
+EXTRA_libgnu_a_SOURCES += explicit_bzero.c
+
+endif
+## end   gnulib module explicit_bzero
+
 ## begin gnulib module faccessat
 ifeq (,$(OMIT_GNULIB_MODULE_faccessat))
 
diff --git a/m4/explicit_bzero.m4 b/m4/explicit_bzero.m4
new file mode 100644
index 0000000..f9dc678
--- /dev/null
+++ b/m4/explicit_bzero.m4
@@ -0,0 +1,22 @@
+dnl Copyright 2017 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_FUNC_EXPLICIT_BZERO],
+[
+  AC_REQUIRE([gl_HEADER_STRING_H_DEFAULTS])
+
+  dnl Persuade glibc <string.h> to declare explicit_bzero.
+  AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
+
+  AC_CHECK_FUNCS_ONCE([explicit_bzero])
+  if test $ac_cv_func_explicit_bzero = no; then
+    HAVE_EXPLICIT_BZERO=0
+  fi
+])
+
+AC_DEFUN([gl_PREREQ_EXPLICIT_BZERO],
+[
+  AC_CHECK_FUNCS([explicit_memset])
+])
diff --git a/m4/gnulib-comp.m4 b/m4/gnulib-comp.m4
index 107645d..038d78a 100644
--- a/m4/gnulib-comp.m4
+++ b/m4/gnulib-comp.m4
@@ -72,6 +72,7 @@ AC_DEFUN
   # Code from module errno:
   # Code from module euidaccess:
   # Code from module execinfo:
+  # Code from module explicit_bzero:
   # Code from module extensions:
   # Code from module extern-inline:
   # Code from module faccessat:
@@ -210,6 +211,12 @@ AC_DEFUN
   gl_UNISTD_MODULE_INDICATOR([environ])
   gl_HEADER_ERRNO_H
   gl_EXECINFO_H
+  gl_FUNC_EXPLICIT_BZERO
+  if test $HAVE_EXPLICIT_BZERO = 0; then
+    AC_LIBOBJ([explicit_bzero])
+    gl_PREREQ_EXPLICIT_BZERO
+  fi
+  gl_STRING_MODULE_INDICATOR([explicit_bzero])
   AC_REQUIRE([gl_EXTERN_INLINE])
   gl_FUNC_FACCESSAT
   if test $HAVE_FACCESSAT = 0; then
@@ -837,6 +844,7 @@ AC_DEFUN
   lib/euidaccess.c
   lib/execinfo.c
   lib/execinfo.in.h
+  lib/explicit_bzero.c
   lib/faccessat.c
   lib/fcntl.c
   lib/fcntl.in.h
@@ -967,6 +975,7 @@ AC_DEFUN
   m4/errno_h.m4
   m4/euidaccess.m4
   m4/execinfo.m4
+  m4/explicit_bzero.m4
   m4/extensions.m4
   m4/extern-inline.m4
   m4/faccessat.m4
diff --git a/src/gnutls.c b/src/gnutls.c
index e6f01a9..7d19f90 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1883,6 +1883,22 @@ The alist key is the cipher name. */)
   return ciphers;
 }
 
+/* Zero out STORAGE (even if it will become inaccessible.  It has
+   STORAGE_LENGTH bytes.  The goal is to improve security a bit, in
+   case an Emacs module or some buggy part of Emacs attempts to
+   inspect STORAGE later to retrieve a secret.
+
+   Calls to this function document when storage containing a secret is
+   known to go out of scope.  This function is not guaranteed to erase
+   the secret, as copies of STORAGE may well be accessible elsewhere
+   on the machine.  */
+
+static void
+clear_storage (void *storage, ptrdiff_t storage_length)
+{
+  explicit_bzero (storage, storage_length);
+}
+
 static Lisp_Object
 gnutls_symmetric_aead (bool encrypting, gnutls_cipher_algorithm_t gca,
                        Lisp_Object cipher,
@@ -1949,7 +1965,7 @@ gnutls_symmetric_aead (bool encrypting, gnutls_cipher_algorithm_t gca,
 
   if (ret < GNUTLS_E_SUCCESS)
     {
-      memset (storage, 0, storage_length);
+      clear_storage (storage, storage_length);
       SAFE_FREE ();
       gnutls_aead_cipher_deinit (acipher);
       if (encrypting)
@@ -1963,7 +1979,7 @@ gnutls_symmetric_aead (bool encrypting, gnutls_cipher_algorithm_t gca,
   gnutls_aead_cipher_deinit (acipher);
 
   Lisp_Object output = make_unibyte_string (storage, storage_length);
-  memset (storage, 0, storage_length);
+  clear_storage (storage, storage_length);
   SAFE_FREE ();
   return list2 (output, actual_iv);
 #else
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Use-memset-not-bzero.patch --]
[-- Type: text/x-patch; name="0003-Use-memset-not-bzero.patch", Size: 1131 bytes --]

From b740b02d2311cb5a3dd61767f824f3bfa770184e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 16 Jul 2017 16:22:33 -0700
Subject: [PATCH 3/3] Use memset, not bzero

* src/ftcrfont.c (ftcrfont_glyph_extents): Use memset instead
of the (less-portable) bzero.
---
 src/ftcrfont.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index d720057..9b592e6 100644
--- a/src/ftcrfont.c
+++ b/src/ftcrfont.c
@@ -81,9 +81,9 @@ ftcrfont_glyph_extents (struct font *font,
       ftcrfont_info->metrics =
 	xrealloc (ftcrfont_info->metrics,
 		  sizeof (struct font_metrics *) * (row + 1));
-      bzero (ftcrfont_info->metrics + ftcrfont_info->metrics_nrows,
-	     (sizeof (struct font_metrics *)
-	      * (row + 1 - ftcrfont_info->metrics_nrows)));
+      memset (ftcrfont_info->metrics + ftcrfont_info->metrics_nrows, 0,
+	      (sizeof (struct font_metrics *)
+	       * (row + 1 - ftcrfont_info->metrics_nrows)));
       ftcrfont_info->metrics_nrows = row + 1;
     }
   if (ftcrfont_info->metrics[row] == NULL)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: GnuTLS and zeroing keys in Emacs
  2017-07-15 19:00 ` Ted Zlatanov
  2017-07-16 23:53   ` Paul Eggert
@ 2017-07-17  0:29   ` Paul Eggert
  2017-07-17 14:07     ` Ted Zlatanov
  1 sibling, 1 reply; 6+ messages in thread
From: Paul Eggert @ 2017-07-17  0:29 UTC (permalink / raw)
  To: emacs-devel

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

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 ...".

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-src-gnutls.c-Restore-some-comments.patch --]
[-- Type: text/x-patch; name="0001-src-gnutls.c-Restore-some-comments.patch", Size: 1918 bytes --]

From 8fdadf52567de8dc5979e9b453b16c429f76bd87 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 16 Jul 2017 17:27:03 -0700
Subject: [PATCH] * src/gnutls.c: Restore some comments.

---
 src/gnutls.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 7d19f90..bcccd7f 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1857,7 +1857,10 @@ The alist key is the cipher name. */)
   for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
     {
       gnutls_cipher_algorithm_t gca = gciphers[pos];
+
+      /* A symbol representing the GnuTLS cipher.  */
       Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
+
       ptrdiff_t cipher_tag_size = gnutls_cipher_get_tag_size (gca);
 
       Lisp_Object cp
@@ -2200,9 +2203,10 @@ name. */)
     {
       const gnutls_mac_algorithm_t gma = macs[pos];
 
-      const char *name = gnutls_mac_get_name (gma);
+      /* A symbol representing the GnuTLS MAC algorithm.  */
+      Lisp_Object gma_symbol = intern (gnutls_mac_get_name (gma));
 
-      Lisp_Object mp = listn (CONSTYPE_HEAP, 11, intern (name),
+      Lisp_Object mp = listn (CONSTYPE_HEAP, 11, gma_symbol,
 			      QCmac_algorithm_id, make_number (gma),
 			      QCtype, Qgnutls_type_mac_algorithm,
 
@@ -2236,9 +2240,10 @@ method name. */)
     {
       const gnutls_digest_algorithm_t gda = digests[pos];
 
-      const char *name = gnutls_digest_get_name (gda);
+      /* A symbol representing the GnuTLS digest algorithm.  */
+      Lisp_Object gda_symbol = intern (gnutls_digest_get_name (gda));
 
-      Lisp_Object mp = listn (CONSTYPE_HEAP, 7, intern (name),
+      Lisp_Object mp = listn (CONSTYPE_HEAP, 7, gda_symbol,
 			      QCdigest_algorithm_id, make_number (gda),
 			      QCtype, Qgnutls_type_digest_algorithm,
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: GnuTLS and zeroing keys in Emacs
  2017-07-16 23:53   ` Paul Eggert
@ 2017-07-17 13:52     ` Ted Zlatanov
  0 siblings, 0 replies; 6+ messages in thread
From: Ted Zlatanov @ 2017-07-17 13:52 UTC (permalink / raw)
  To: emacs-devel

On Sun, 16 Jul 2017 16:53:20 -0700 Paul Eggert <eggert@cs.ucla.edu> 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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: GnuTLS and zeroing keys in Emacs
  2017-07-17  0:29   ` Paul Eggert
@ 2017-07-17 14:07     ` Ted Zlatanov
  0 siblings, 0 replies; 6+ messages in thread
From: Ted Zlatanov @ 2017-07-17 14:07 UTC (permalink / raw)
  To: emacs-devel

On Sun, 16 Jul 2017 17:29:09 -0700 Paul Eggert <eggert@cs.ucla.edu> 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




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-07-17 14:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-14 23:42 GnuTLS and zeroing keys in Emacs Paul Eggert
2017-07-15 19:00 ` Ted Zlatanov
2017-07-16 23:53   ` Paul Eggert
2017-07-17 13:52     ` Ted Zlatanov
2017-07-17  0:29   ` Paul Eggert
2017-07-17 14:07     ` Ted Zlatanov

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).