Re: Builtin GnuTLS support and certificate verification

Re: Builtin GnuTLS support and certificate verification

[Ted Zlatanov]

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

On Sun, 08 Dec 2013 09:39:23 +0100 Vincent Bernat <bernat@luffy.cx> wrote: 

VB>  ❦  8 décembre 2013 05:22 CET, Ted Zlatanov <tzz@lifelogs.com> :
>>>> Verification options could be:
>>>> 
>>>> - `expired-certificate`
>>>> - `revoked-certificate`
>>>> - `untrusted-certificate`
>>>> - `hostname-mismatch`
>> 
>> I'm not sure this granularity is necessary.  I just have :trustfiles and
>> :hostname as options right now.  Anyone else with an opinion?

VB> I thought you wanted something granular, hence the proposition. I don't
VB> know of any other software proposing granular verification. So, no
VB> problem with dropping this.

Yeah, I went back and forth.  I think two (:trustfiles and :hostname)
verification options is the right balance for now.  I couldn't think of
any cases where I'd want more granularity, but that's why I'm asking :)

>> My concern is that suddenly connections will start failing for our users
>> and bug reports will flow, and I don't have time to explain to everyone
>> why their self-signed certificates need exceptions.  This can be really,
>> really annoying.  But logging in *Messages* is not very useful either,
>> users don't read it.  So what's the right thing?  How about a default
>> behavior of flashing a warning, then sit-for 3 seconds?  A hard error
>> can be optional but not the default.

VB> For a first release, just default to the previous behaviour. However,
VB> some day, it would be nice to be "secure by default".

http://debbugs.gnu.org/13374 and other related bugs are about enabling
"secure by default" and I would like to make that change now, before the
Emacs code freeze.

It feels like the sit-for option is safest.  It preserves the current
behavior while forcing the user to notice something's wrong.  In the
next release, based on user feedback, we can turn verify-error on.  If
no one disagrees, I will implement it on top of the patch attached here.

>> I'm also not sure I like the look and feel of the `gnutls-verify-error'
>> defcustom.  It's kind of awkward.

VB> I am don't know defcustom enough to know if this would give something
VB> like this:

VB> '((".*\\.internal" :hostname)
VB>   (".*" :trustfiles :hostname))

VB> If yes, this seems fine for me. Just add the possible values for tags in
VB> the document string.

Yes, except the cdr is a nested list because I use the "set"
customization type.  So it's '((".*" (:hostname :trustfiles)))

>> I'd like to get this done before the Emacs code freeze next week or so.
>> Please give me your opinions and test the code.

VB> I'll try to test later this day.

Thanks.  Posting patch to emacs-devel as well for review and feedback.

Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gnutls-verify-error.patch --]
[-- Type: text/x-diff, Size: 8558 bytes --]

=== modified file 'lisp/net/gnutls.el'
--- lisp/net/gnutls.el	2013-09-18 04:50:54 +0000
+++ lisp/net/gnutls.el	2013-12-08 04:00:04 +0000
@@ -51,6 +51,19 @@
   :type '(choice (const nil)
                  string))
 
+(defcustom gnutls-verify-error nil
+  "If non-nil, this should be a list of checks per hostname regex or t."
+  :group 'gnutls
+  :type '(choice
+          (const t)
+          (repeat :tag "List of hostname regexps with flags for each"
+           (list
+            (choice :tag "Hostname"
+                    (const ".*" :tag "Any hostname")
+                    regexp)
+            (set (const :trustfiles)
+                 (const :hostname))))))
+
 (defcustom gnutls-trustfiles
   '(
     "/etc/ssl/certs/ca-certificates.crt" ; Debian, Ubuntu, Gentoo and Arch Linux
@@ -138,19 +151,25 @@
 \(see `gnutls-min-prime-bits' for more information).  Use nil for the
 default.
 
-When VERIFY-HOSTNAME-ERROR is not nil, an error will be raised
-when the hostname does not match the presented certificate's host
-name.  The exact verification algorithm is a basic implementation
-of the matching described in RFC2818 (HTTPS), which takes into
-account wildcards, and the DNSName/IPAddress subject alternative
-name PKIX extension.  See GnuTLS' gnutls_x509_crt_check_hostname
-for details.  When VERIFY-HOSTNAME-ERROR is nil, only a warning
-will be issued.
-
-When VERIFY-ERROR is not nil, an error will be raised when the
-peer certificate verification fails as per GnuTLS'
-gnutls_certificate_verify_peers2.  Otherwise, only warnings will
-be shown about the verification failure.
+VERIFY-HOSTNAME-ERROR is a backwards compatibility option for
+putting `:hostname' in VERIFY-ERROR.
+
+When VERIFY-ERROR is t or a list containing `:trustfiles', an
+error will be raised when the peer certificate verification fails
+as per GnuTLS' gnutls_certificate_verify_peers2.  Otherwise, only
+warnings will be shown about the verification failure.
+
+When VERIFY-ERROR is t or a list containing `:hostname', an error
+will be raised when the hostname does not match the presented
+certificate's host name.  The exact verification algorithm is a
+basic implementation of the matching described in
+RFC2818 (HTTPS), which takes into account wildcards, and the
+DNSName/IPAddress subject alternative name PKIX extension.  See
+GnuTLS' gnutls_x509_crt_check_hostname for details.  Otherwise,
+only a warning will be issued.
+
+Note that the list in `gnutls-verify-error', matched against the
+HOSTNAME, is the default VERIFY-ERROR.
 
 VERIFY-FLAGS is a numeric OR of verification flags only for
 `gnutls-x509pki' connections.  See GnuTLS' x509.h for details;
@@ -183,8 +202,28 @@
                                 (if gnutls-algorithm-priority
                                     (upcase gnutls-algorithm-priority)
                                   "NORMAL")))))
+         (verify-error (or verify-error
+                           ;; this uses the value of `gnutls-verify-error'
+                           (cond
+                            ;; if t, pass it on
+                            ((eq gnutls-verify-error t)
+                             t)
+                            ;; if a list, look for hostname matches
+                            ((listp gnutls-verify-error)
+                             (mapcan
+                              (lambda (check)
+                                (when (string-match (car check) hostname)
+                                  (cdr check)))
+                              gnutls-verify-error))
+                            ;; else it's nil
+                            (t nil))))
          (min-prime-bits (or min-prime-bits gnutls-min-prime-bits))
-         (params `(:priority ,priority-string
+         params ret)
+
+    (when verify-hostname-error
+      (push :hostname verify-error))
+
+    (setq params `(:priority ,priority-string
                              :hostname ,hostname
                              :loglevel ,gnutls-log-level
                              :min-prime-bits ,min-prime-bits
@@ -193,9 +232,7 @@
                              :keylist ,keylist
                              :verify-flags ,verify-flags
                              :verify-error ,verify-error
-                             :verify-hostname-error ,verify-hostname-error
                              :callbacks nil))
-         ret)
 
     (gnutls-message-maybe
      (setq ret (gnutls-boot process type params))

=== modified file 'src/gnutls.c'
--- src/gnutls.c	2013-11-30 13:31:39 +0000
+++ src/gnutls.c	2013-12-08 04:10:44 +0000
@@ -49,7 +49,7 @@
 static Lisp_Object QCgnutls_bootprop_hostname;
 static Lisp_Object QCgnutls_bootprop_min_prime_bits;
 static Lisp_Object QCgnutls_bootprop_verify_flags;
-static Lisp_Object QCgnutls_bootprop_verify_hostname_error;
+static Lisp_Object QCgnutls_bootprop_verify_error;
 
 /* Callback keys for `gnutls-boot'.  Unused currently.  */
 static Lisp_Object QCgnutls_bootprop_callbacks_verify;
@@ -753,8 +753,12 @@
 :verify-flags is a bitset as per GnuTLS'
 gnutls_certificate_set_verify_flags.
 
-:verify-hostname-error, if non-nil, makes a hostname mismatch an
-error.  Otherwise it will be just a warning.
+:verify-hostname-error is ignored.  Pass :hostname in :verify-error
+instead.
+
+:verify-error is a list of symbols to express verification checks or
+`t' to do all checks.  Currently it can contain `:trustfiles' and
+`:hostname' to verify the certificate or the hostname respectively.
 
 :min-prime-bits is the minimum accepted number of bits the client will
 accept in Diffie-Hellman key exchange.
@@ -798,8 +802,7 @@
   /* Lisp_Object callbacks; */
   Lisp_Object loglevel;
   Lisp_Object hostname;
-  /* Lisp_Object verify_error; */
-  Lisp_Object verify_hostname_error;
+  Lisp_Object verify_error;
   Lisp_Object prime_bits;
 
   CHECK_PROCESS (proc);
@@ -818,11 +821,14 @@
   keylist               = Fplist_get (proplist, QCgnutls_bootprop_keylist);
   crlfiles              = Fplist_get (proplist, QCgnutls_bootprop_crlfiles);
   loglevel              = Fplist_get (proplist, QCgnutls_bootprop_loglevel);
-  verify_hostname_error = Fplist_get (proplist, QCgnutls_bootprop_verify_hostname_error);
+  verify_error          = Fplist_get (proplist, QCgnutls_bootprop_verify_error);
   prime_bits            = Fplist_get (proplist, QCgnutls_bootprop_min_prime_bits);
 
+  if (!Flistp (verify_error))
+    error ("gnutls-boot: invalid :verify_error parameter (not a list)");
+
   if (!STRINGP (hostname))
-    error ("gnutls-boot: invalid :hostname parameter");
+    error ("gnutls-boot: invalid :hostname parameter (not a string)");
   c_hostname = SSDATA (hostname);
 
   state = XPROCESS (proc)->gnutls_state;
@@ -1047,14 +1053,17 @@
 
   if (peer_verification != 0)
     {
-      if (NILP (verify_hostname_error))
-	GNUTLS_LOG2 (1, max_log_level, "certificate validation failed:",
-		     c_hostname);
-      else
-	{
+      if (EQ (verify_error, Qt)
+          || !NILP (Fmember (QCgnutls_bootprop_trustfiles, verify_error)))
+        {
 	  emacs_gnutls_deinit (proc);
 	  error ("Certificate validation failed %s, verification code %d",
 		 c_hostname, peer_verification);
+        }
+      else
+	{
+          GNUTLS_LOG2 (1, max_log_level, "certificate validation failed:",
+                       c_hostname);
 	}
     }
 
@@ -1094,14 +1103,17 @@
 
       if (!fn_gnutls_x509_crt_check_hostname (gnutls_verify_cert, c_hostname))
 	{
-	  if (NILP (verify_hostname_error))
-	    GNUTLS_LOG2 (1, max_log_level, "x509 certificate does not match:",
-			 c_hostname);
-	  else
-	    {
+          if (EQ (verify_error, Qt)
+              || !NILP (Fmember (QCgnutls_bootprop_hostname, verify_error)))
+            {
 	      fn_gnutls_x509_crt_deinit (gnutls_verify_cert);
 	      emacs_gnutls_deinit (proc);
 	      error ("The x509 certificate does not match \"%s\"", c_hostname);
+            }
+	  else
+	    {
+              GNUTLS_LOG2 (1, max_log_level, "x509 certificate does not match:",
+                           c_hostname);
 	    }
 	}
       fn_gnutls_x509_crt_deinit (gnutls_verify_cert);
@@ -1161,7 +1173,7 @@
   DEFSYM (QCgnutls_bootprop_min_prime_bits, ":min-prime-bits");
   DEFSYM (QCgnutls_bootprop_loglevel, ":loglevel");
   DEFSYM (QCgnutls_bootprop_verify_flags, ":verify-flags");
-  DEFSYM (QCgnutls_bootprop_verify_hostname_error, ":verify-hostname-error");
+  DEFSYM (QCgnutls_bootprop_verify_error, ":verify-error");
 
   DEFSYM (Qgnutls_e_interrupted, "gnutls-e-interrupted");
   Fput (Qgnutls_e_interrupted, Qgnutls_code,

Re: Builtin GnuTLS support and certificate verification

[Ted Zlatanov]

I pushed this work to the Emacs trunk with the old behavior (connections
never abort, just warn).  Please customize `gnutls-verify-error' to get
the new behavior, erroring out on validation failures.  I encourage you
to try it and report any issues.

Ted