Bug 49805 - SSL Wildcard support is too lenient (and a bunch of SSL tests are broken under OpenSSL)
Summary: SSL Wildcard support is too lenient (and a bunch of SSL tests are broken unde...
Status: RESOLVED FIXED
Alias: None
Product: Wocky
Classification: Unclassified
Component: General (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/wj...
Whiteboard:
Keywords: patch
Depends on:
Blocks: 39070
  Show dependency treegraph
 
Reported: 2012-05-11 09:04 UTC by Vivek Dasmohapatra
Modified: 2013-01-22 16:42 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Vivek Dasmohapatra 2012-05-11 09:04:50 UTC
The wildcard implementation accepted *oogle.com as well as *.whatever
style wildcards. 

This is a) a time-bomb and b) only required by the HTTP SSL RFC.

Also, a bunch of the SSL tests were broken under OpenSSL,
they should all work again now.
Comment 1 Will Thompson 2012-05-15 08:09:57 UTC
Review of the first few patches:

+    { "/connector/cert-verification/tls/wildcard/level-mismatch/fail",

Could you add a comment specifying why these tests should fail? I *believe* that that one fails because:

+        { "weasel-juice.org", PORT_XMPP, "thud.org", REACHABLE, UNREACHABLE },
+        { PLAINTEXT_OK,
+          { "moose@weasel-juice.org", "something", PLAIN, TLS },
+          { NULL, 0, XMPP_V1, STARTTLS, CERT_CHECK_STRICT, TLS_CA_DIR } } },

the certificate is for *.weasel-juice.org, which should not match weasel-juice.org. But I'm not sure where thud.org comes into it. Ditto the subsequent tests.

It would be good if what's bad about the BADWILD certificate were written down.

In http://cgit.collabora.com/git/user/vivek/wocky.git/commit/?h=wildcarded-certificate-check&id=0c8d0c9b1f871d8794a3c6716540a904dfba989a :

+static inline gboolean
+compare_hostname (const char *host, const char *cert)
+{
+    /* advance to first different character */
+    for (; CASELESS_CHARCMP (*cert, *host); cert++, host++);
+
+    /* were the strings entirely, caselessly equal? */
+    return (strlen (cert) == 0 && strlen (host) == 0);
+}

can be replaced by (g_ascii_strcasecmp (host, cert) == 0);

+  while( *certname++ == '*' && *certname++ == '.' )
+      /* a leading '*.' swallows the next domain word */
+      hostname = index( hostname, '.' );
+
+      if( hostname == NULL )

Coding style: while (...) not while( ... ).

I don't really understand this loop. Given this comment:

+  /* wildcard handling: we only allow leading '*.' wildcards:
+     no *foo.blerg.org - that would be a biiig security hole */

why is it a loop? Is it meant to allow lol.*.co.uk to match lol.google.co.uk? If not, how about this:

if (g_str_has_prefix (certname, "*."))
  {
    const gchar *certname_tail = certname + 2;
    const gchar *hostname_tail = index (hostname, '.');

    if (hostname_tail == NULL)
      return FALSE;

    hostname_tail++;
    DEBUG ("%s ~ %s", hostname_tail, certname_tail);
    return compare_hostname (hostname_tail, certname_tail);
  }
Comment 2 Will Thompson 2012-05-15 08:15:55 UTC
Based on discussion on IRC:

+static inline gboolean
+invalid_wildcard (const char *name, int size)

is needed because GNUTLS allows wildcards anywhere in the certificate name, not just a leading "*.". So could its guts be replaced by:

  if (name[0] == '*' && name[1] == '.')
    name += 2;

  return index (name, '*') == NULL;

?
Comment 3 Will Thompson 2012-05-15 08:29:44 UTC
Review of the CRL-related patches:

Why does one half of http://cgit.collabora.com/git/user/vivek/wocky.git/commit/?h=wildcarded-certificate-check&id=e4812722e460f8ebd014eced924e6bb5c6c70d00 use g_file_*, and the other half use stat?

+
+GSList *
+wocky_tls_handler_get_crl (WockyTLSHandler *self)

doc comment please.
Comment 4 Vivek Dasmohapatra 2012-05-15 08:33:52 UTC
(In reply to comment #3)
> Review of the CRL-related patches:
> 
> Why does one half of
> http://cgit.collabora.com/git/user/vivek/wocky.git/commit/?h=wildcarded-certificate-check&id=e4812722e460f8ebd014eced924e6bb5c6c70d00
> use g_file_*, and the other half use stat?

Copy pasta, I think. The openssl "this is how you do CRLs" examples
used stat and I gutted and reused them for the gnutls. I'll change
them all over.
Comment 5 Will Thompson 2012-11-21 17:41:56 UTC
I've fixed up those of my complaints which I think matter. Anyone care to take a look at my patches at the top of the branch?
Comment 6 Will Thompson 2013-01-22 16:42:47 UTC
i timed out and merged this a while ago: http://cgit.freedesktop.org/wocky/commit/?id=388f4fbd9474cf92b9c7f0cca8871fe5e4dc7569


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.