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