[PATCH] gnutls: Add SNI support

[PATCH] gnutls: Add SNI support

[Toke Høiland-Jørgensen]

Currently, Emacs does not support Server Name Identification. This means
that servers that host multiple SSL sites on the same IP will send the
wrong certificate to Emacs. In addition, some servers refuse connections
entirely if the SNI extension is not included in the client handshake.

This patch adds what I think is the required call into GnuTLS to add the
hostname information on the handshake. Unfortunately I have been unable
to actually test the patch, since I can't get the git trunk to compile.
However, I thought I'd post it anyway to maybe get someone else to have
a look.

Oh, and there's a bit of a hack in trying to detect whether the hostname
is an IPv4 or IPv6 literal (in which case the hostname shouldn't be
included in the handshake). Not sure if omitting that check entirely
will work, or if there's a better way to detect this case.

-Toke



diff --git a/src/gnutls.c b/src/gnutls.c
index 22e3aec..e8a6966 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -187,6 +187,9 @@ DEF_GNUTLS_FN (int, gnutls_x509_crt_get_key_id,
 DEF_GNUTLS_FN (const char*, gnutls_sec_param_get_name, (gnutls_sec_param_t));
 DEF_GNUTLS_FN (const char*, gnutls_sign_algorithm_get_name,
 	       (gnutls_sign_algorithm_t));
+DEF_GNUTLS_FN (int, gnutls_server_name_set, (gnutls_session_t,
+					     gnutls_server_name_type_t,
+					     const void *, size_t));
 
 static bool
 init_gnutls_functions (void)
@@ -263,6 +266,7 @@ init_gnutls_functions (void)
   LOAD_GNUTLS_FN (library, gnutls_x509_crt_get_key_id);
   LOAD_GNUTLS_FN (library, gnutls_sec_param_get_name);
   LOAD_GNUTLS_FN (library, gnutls_sign_algorithm_get_name);
+  LOAD_GNUTLS_FN (library, gnutls_server_name_set);
 
   max_log_level = global_gnutls_log_level;
 
@@ -335,6 +339,7 @@ init_gnutls_functions (void)
 #define fn_gnutls_x509_crt_get_key_id           gnutls_x509_crt_get_key_id
 #define fn_gnutls_sec_param_get_name            gnutls_sec_param_get_name
 #define fn_gnutls_sign_algorithm_get_name       gnutls_sign_algorithm_get_name
+#define fn_gnutls_server_name_set		gnutls_server_name_set
 
 #endif /* !WINDOWSNT */
 
@@ -1137,6 +1142,7 @@ one trustfile (usually a CA bundle).  */)
   char const *priority_string_ptr = "NORMAL"; /* default priority string.  */
   unsigned int peer_verification;
   char *c_hostname;
+  char *c; bool send_hostname = 0;
 
   /* Placeholders for the property list elements.  */
   Lisp_Object priority_string;
@@ -1375,6 +1381,22 @@ one trustfile (usually a CA bundle).  */)
   if (ret < GNUTLS_E_SUCCESS)
     return gnutls_make_error (ret);
 
+  /* Quick and dirty test of the hostname; shouldn't be an IP. If it
+     contains letters, we assume it's a hostname, unless it contains a
+     : in which case we assume it's a literal IPv6 address. */
+  for(c = c_hostname; c; c++) {
+    if(c >= 'a') send_hostname = 1;
+    if(c == ':') {send_hostname = 0; break;}
+  }
+
+  if(send_hostname) {
+    GNUTLS_LOG (1, max_log_level, "setting TLS hostname");
+    ret = fn_gnutls_server_name_set(state, GNUTLS_NAME_DNS, c_hostname, strlen(c_hostname));
+    if (ret < GNUTLS_E_SUCCESS)
+      return gnutls_make_error (ret);
+  }
+
+
   GNUTLS_INITSTAGE (proc) = GNUTLS_STAGE_CRED_SET;
   ret = emacs_gnutls_handshake (XPROCESS (proc));
   if (ret < GNUTLS_E_SUCCESS)

Re: [PATCH] gnutls: Add SNI support

[Thien-Thi Nguyen]

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

() Toke Høiland-Jørgensen <toke@toke.dk>
() Mon, 24 Nov 2014 13:17:22 +0100

   +  char *c; bool send_hostname = 0;

      /* Placeholders for the property list elements.  */
      Lisp_Object priority_string;
   @@ -1375,6 +1381,22 @@ one trustfile (usually a CA bundle).  */)
      if (ret < GNUTLS_E_SUCCESS)
        return gnutls_make_error (ret);

   +  /* Quick and dirty test of the hostname; shouldn't be an IP. If it
   +     contains letters, we assume it's a hostname, unless it contains a
   +     : in which case we assume it's a literal IPv6 address. */
   +  for(c = c_hostname; c; c++) {
   +    if(c >= 'a') send_hostname = 1;
   +    if(c == ':') {send_hostname = 0; break;}
   +  }

Aside from the whitespace and brace placement, this code is buggy: ‘c’
is of type ‘char *’ and cannot be meaningfully compared directly to 'a'.

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] gnutls: Add SNI support

[Toke Høiland-Jørgensen]

Thien-Thi Nguyen <ttn@gnu.org> writes:

> Aside from the whitespace and brace placement, this code is buggy: ‘c’
> is of type ‘char *’ and cannot be meaningfully compared directly to
> 'a'.

Ah yes, was missing a pointer deref I guess. But as I noted, there's
probably a better way to check this.

Not sure I'm the right person to integrate this probably; can make it a
feature request instead? Please support SNI... :)

-Toke

Re: [PATCH] gnutls: Add SNI support

[Lars Magne Ingebrigtsen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Not sure I'm the right person to integrate this probably; can make it a
> feature request instead? Please support SNI... :)

This is bug#18208.  Calling inet_pton sounds like a good idea to me, but
might require the gnulib module, as noted:

-------

* This function is missing on some platforms: HP-UX 11.00, OSF/1 4.0,
  Solaris 2.5.1, mingw, MSVC 9, Interix 3.5, BeOS. 
* This function is declared in <netdb.h> instead of <arpa/inet.h> on
  some platforms: NonStop Kernel. 
* This function is declared in <ws2tcpip.h>, with a POSIX incompatible
  declaration, on some platforms: MSVC 9 on Windows >= Vista. 

-------

Paul, any opinions here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] gnutls: Add SNI support

[Florian Weimer]

* Thien-Thi Nguyen:

>    +  /* Quick and dirty test of the hostname; shouldn't be an IP. If it
>    +     contains letters, we assume it's a hostname, unless it contains a
>    +     : in which case we assume it's a literal IPv6 address. */
>    +  for(c = c_hostname; c; c++) {
>    +    if(c >= 'a') send_hostname = 1;
>    +    if(c == ':') {send_hostname = 0; break;}
>    +  }
>
> Aside from the whitespace and brace placement, this code is buggy: ‘c’
> is of type ‘char *’ and cannot be meaningfully compared directly to 'a'.

And what about uppercase names?  Or domain names which do not contain
letters?

(I think to be absolutely correct, you need to track the kind of name
resolution you have performed on the name.)

Re: [PATCH] gnutls: Add SNI support

[Jérémie Courrèges-Anglas]

Toke Høiland-Jørgensen <toke@toke.dk> writes:
[...]
> Oh, and there's a bit of a hack in trying to detect whether the hostname
> is an IPv4 or IPv6 literal (in which case the hostname shouldn't be
> included in the handshake). Not sure if omitting that check entirely
> will work, or if there's a better way to detect this case.

Better not omit the check.  The common trick is to use inet_pton(3) to
see if the param can be converted into an address.  Now this raises
portability issues, which may be solved by the inet_pton gnulib
module[1].

[1] https://www.gnu.org/software/gnulib/manual/html_node/inet_005fpton.html
-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: [PATCH] gnutls: Add SNI support

[Lars Magne Ingebrigtsen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> This patch adds what I think is the required call into GnuTLS to add the
> hostname information on the handshake. Unfortunately I have been unable
> to actually test the patch, since I can't get the git trunk to compile.
> However, I thought I'd post it anyway to maybe get someone else to have
> a look.

Thanks; I'm applying a version of your patch...

> Oh, and there's a bit of a hack in trying to detect whether the hostname
> is an IPv4 or IPv6 literal (in which case the hostname shouldn't be
> included in the handshake). Not sure if omitting that check entirely
> will work, or if there's a better way to detect this case.

... but I left this out.  Does it matter that we send the host name if
we've been given the URL https://01.02.03.04/ or whatever?  First of
all, you'd think that would be kinda rare.  Second of all, does the
library mind getting an IP address as the SNI?

If not, we could just do the SNI always without checking.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] gnutls: Add SNI support

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Second of all, does the library mind getting an IP address as the SNI?

Well, it's documented to not want IP addresses.  So I added a function
to check for that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] gnutls: Add SNI support

[Toke Høiland-Jørgensen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Thanks; I'm applying a version of your patch...

Cool!

> Well, it's documented to not want IP addresses. So I added a function
> to check for that.

Yep, probably a good call.

Would test the change, but emacs trunk still won't compile on my box and
I'm stumped on debugging it. Code looks good to me, though :)

-Toke