SNI names are case sensitive

[jedrzejj]

Currently certificate selection based on an SNI name is case-sensitive. If you connect to hitch server with eg. curl https://HOST.EXAMPLE.COM/ it will not find a certificate for host.example.com.
The suggestion is to convert servername to lowercase before sni_lookup.

[jedrzejj]

diff --git a/src/hitch.c b/src/hitch.c
index 88de33e..c8b41de 100644
--- a/src/hitch.c
+++ b/src/hitch.c
@@ -756,6 +756,8 @@ static sslctx *
 sni_lookup(const char *servername, const sni_name *sn_tab)
 {
        const sni_name *sn;
+       char *p = (char *)servername;
+       for ( ; *p; ++p) *p = tolower(*p);
 
        HASH_FIND_STR(sn_tab, servername, sn);
        if (sn == NULL) {

[dridi]

We could probably change this instead:

hitch/src/foreign/uthash.h

Lines 607 to 608 in 2e68a70

	/* key comparison function; return 0 if keys equal */
	#define HASH_KEYCMP(a,b,len) memcmp(a,b,(unsigned long)(len))

Instead of memcmp we could use strncasecmp.

[jedrzejj]

Doesn't work for me.

Changed to:

#define HASH_KEYCMP(a,b,len) strncasecmp(a,b,(unsigned long)(len))

But still a wrong certificate is selected (first from the configuration, not the matching one) and I get:

* Unable to communicate securely with peer: requested domain name does not match the server's certificate.
* Closing connection 0
curl: (51) Unable to communicate securely with peer: requested domain name does not match the server's certificate.

I don't know the internals of UTHASH. There is some hashing on the key with HASH_FCN.

[dridi]

Duh, my bad.

We should instead #define our own HASH_FUNCTION and have one case-insensitive.

[jedrzejj]

I've found an feature request for case-less find in uthash: troydhanson/uthash#149
But it didn't result in any new case-insensitive hash function. They advised to make a fix outside of uthash (ie. convert the key before supplying it to a hashing function).

[jedrzejj]

I tried to add a new hashing function based on the default HASH_JEN, which basically works the same, but ignores the 5th bit in comparison. This works together with the change of HASH_KEYCMP. There needs to be an additional define in config.h:

#define HASH_FUNCTION HASH_JEN_I

The patch is:

diff --git a/src/foreign/uthash.h b/src/foreign/uthash.h
index 367d295..c7573e7 100644
--- a/src/foreign/uthash.h
+++ b/src/foreign/uthash.h
@@ -468,6 +468,47 @@ do {
   bkt = hashv & (num_bkts-1U);                                                   \
 } while(0)
 
+#define HASH_JEN_I(key,keylen,num_bkts,hashv,bkt)                                \
+do {                                                                             \
+  unsigned _hj_i,_hj_j,_hj_k;                                                    \
+  unsigned const char *_hj_key=(unsigned const char*)(key);                      \
+  hashv = 0xfeedbeefu;                                                           \
+  _hj_i = _hj_j = 0x9e3779b9u;                                                   \
+  _hj_k = (unsigned)(keylen);                                                    \
+  while (_hj_k >= 12U) {                                                         \
+    _hj_i +=    ((_hj_key[0]|0x20) + ( (unsigned)(_hj_key[1]|0x20) << 8 )        \
+        + ( (unsigned)(_hj_key[2]|0x20) << 16 )                                  \
+        + ( (unsigned)(_hj_key[3]|0x20) << 24 ) );                               \
+    _hj_j +=    ((_hj_key[4]|0x20) + ( (unsigned)(_hj_key[5]|0x20) << 8 )        \
+        + ( (unsigned)(_hj_key[6]|0x20) << 16 )                                  \
+        + ( (unsigned)(_hj_key[7]|0x20) << 24 ) );                               \
+    hashv += ((_hj_key[8]|0x20) + ( (unsigned)(_hj_key[9]|0x20) << 8 )           \
+        + ( (unsigned)(_hj_key[10]|0x20) << 16 )                                 \
+        + ( (unsigned)(_hj_key[11]|0x20) << 24 ) );                              \
+                                                                                 \
+     HASH_JEN_MIX(_hj_i, _hj_j, hashv);                                          \
+                                                                                 \
+     _hj_key += 12;                                                              \
+     _hj_k -= 12U;                                                               \
+  }                                                                              \
+  hashv += (unsigned)(keylen);                                                   \
+  switch ( _hj_k ) {                                                             \
+     case 11: hashv += ( (unsigned)(_hj_key[10]|0x20) << 24 ); /* FALLTHROUGH */ \
+     case 10: hashv += ( (unsigned)(_hj_key[9]|0x20) << 16 );  /* FALLTHROUGH */ \
+     case 9:  hashv += ( (unsigned)(_hj_key[8]|0x20) << 8 );   /* FALLTHROUGH */ \
+     case 8:  _hj_j += ( (unsigned)(_hj_key[7]|0x20) << 24 );  /* FALLTHROUGH */ \
+     case 7:  _hj_j += ( (unsigned)(_hj_key[6]|0x20) << 16 );  /* FALLTHROUGH */ \
+     case 6:  _hj_j += ( (unsigned)(_hj_key[5]|0x20) << 8 );   /* FALLTHROUGH */ \
+     case 5:  _hj_j += (_hj_key[4]|0x20);                      /* FALLTHROUGH */ \
+     case 4:  _hj_i += ( (unsigned)(_hj_key[3]|0x20) << 24 );  /* FALLTHROUGH */ \
+     case 3:  _hj_i += ( (unsigned)(_hj_key[2]|0x20) << 16 );  /* FALLTHROUGH */ \
+     case 2:  _hj_i += ( (unsigned)(_hj_key[1]|0x20) << 8 );   /* FALLTHROUGH */ \
+     case 1:  _hj_i += (_hj_key[0]|0x20);                                        \
+  }                                                                              \
+  HASH_JEN_MIX(_hj_i, _hj_j, hashv);                                             \
+  bkt = hashv & (num_bkts-1U);                                                   \
+} while(0)
+
 /* The Paul Hsieh hash function */
 #undef get16bits
 #if (defined(__GNUC__) && defined(__i386__)) || defined(__WATCOMC__)             \
@@ -605,7 +646,7 @@ do {                                                                   \
 #endif  /* HASH_USING_NO_STRICT_ALIASING */
 
 /* key comparison function; return 0 if keys equal */
-#define HASH_KEYCMP(a,b,len) memcmp(a,b,(unsigned long)(len))
+#define HASH_KEYCMP(a,b,len) strncasecmp(a,b,(unsigned long)(len))
 
 /* iterate over items in a known bucket to find desired item */
 #define HASH_FIND_IN_BKT(tbl,hh,head,keyptr,keylen_in,out)                       \

[dridi]

We'll probably discuss it with the team on Monday.

[jedrzejj]

I have both versions in my fork for testing:
https://github.com/jedrzejj/hitch/tree/sni-lowercase-hash-func
https://github.com/jedrzejj/hitch/tree/sni-servername-lowercase

[dridi]

I wasn't around last Monday to bring this up and was unavailable this week. One thing that occurred to me is that we are basically discussing case-insensitivity in an ASCII context and we should also look at IDNA before we move forward with this.

[jedrzejj]

That was just a quick fix, because I needed to get it to work asap. And we only have domain names and certificate CNs is ASCII, so IDNs weren't a problem for me.
Actually, I don't have much hands-on experience with IDNs, because for now, we stick to ASCII only in domain names.
However after a few quick tests I think that clients convert the IDN names to lowercase while converting them to punycode.
e.g. with ASCII name the Host: header retains the capitalized domain:

curl -v http://COSTAM.ehum.psnc.pl/
* About to connect() to COSTAM.ehum.psnc.pl port 80 (#0)
*   Trying 150.254.186.176...
* Connected to COSTAM.ehum.psnc.pl (150.254.186.176) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.29.0
> Host: COSTAM.ehum.psnc.pl
> Accept: */*
> 
< HTTP/1.1 302 Moved Temporarily

While with diacritical characters the generated punycode is lowercase:

curl -v http://CoŚtaM.ehum.psnc.pl/
* Input domain encoded as `UTF-8'
* About to connect() to xn--cotam-vcb.ehum.psnc.pl port 80 (#0)
*   Trying 150.254.186.176...
* Connected to CoŚtaM.ehum.psnc.pl (150.254.186.176) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.29.0
> Host: xn--cotam-vcb.ehum.psnc.pl
> Accept: */*
> 
< HTTP/1.1 302 Moved Temporarily