Bug 49805

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);
  }

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;

?

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.

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

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?

i timed out and merged this a while ago: http://cgit.freedesktop.org/wocky/commit/?id=388f4fbd9474cf92b9c7f0cca8871fe5e4dc7569