From 1f0d4f3d8dd5c87d3f42a913a1af9d6f1f2da437 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 26 Apr 2014 13:51:14 +0200 Subject: [PATCH 1/3] netops: unit-test the cert host-name pattern matching This kind of stuff should have unit tests, even if it's just to show what we expect to match successfully. --- src/netops.c | 8 ++++---- src/netops.h | 13 +++++++++++++ tests/network/matchhost.c | 13 +++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 tests/network/matchhost.c diff --git a/src/netops.c b/src/netops.c index ad27d84cf..23f482b12 100644 --- a/src/netops.c +++ b/src/netops.c @@ -207,7 +207,7 @@ static int gitno_ssl_teardown(gitno_ssl *ssl) } /* Match host names according to RFC 2818 rules */ -static int match_host(const char *pattern, const char *host) +int gitno__match_host(const char *pattern, const char *host) { for (;;) { char c = tolower(*pattern++); @@ -230,9 +230,9 @@ static int match_host(const char *pattern, const char *host) while(*host) { char h = tolower(*host); if (c == h) - return match_host(pattern, host++); + return gitno__match_host(pattern, host++); if (h == '.') - return match_host(pattern, host); + return gitno__match_host(pattern, host); host++; } return -1; @@ -250,7 +250,7 @@ static int check_host_name(const char *name, const char *host) if (!strcasecmp(name, host)) return 0; - if (match_host(name, host) < 0) + if (gitno__match_host(name, host) < 0) return -1; return 0; diff --git a/src/netops.h b/src/netops.h index 666d66b12..8e3a2524f 100644 --- a/src/netops.h +++ b/src/netops.h @@ -54,6 +54,19 @@ enum { GITNO_CONNECT_SSL_NO_CHECK_CERT = 2, }; +/** + * Check if the name in a cert matches the wanted hostname + * + * Check if a pattern from a certificate matches the hostname we + * wanted to connect to according to RFC2818 rules (which specifies + * HTTP over TLS). Mainly, an asterisk matches anything, but is + * limited to a single url component. + * + * Note that this does not set an error message. It expects the user + * to provide the message for the user. + */ +int gitno__match_host(const char *pattern, const char *host); + void gitno_buffer_setup(gitno_socket *t, gitno_buffer *buf, char *data, size_t len); void gitno_buffer_setup_callback(gitno_socket *t, gitno_buffer *buf, char *data, size_t len, int (*recv)(gitno_buffer *buf), void *cb_data); int gitno_recv(gitno_buffer *buf); diff --git a/tests/network/matchhost.c b/tests/network/matchhost.c new file mode 100644 index 000000000..3100dc21d --- /dev/null +++ b/tests/network/matchhost.c @@ -0,0 +1,13 @@ +#include "clar_libgit2.h" +#include "netops.h" + +void test_network_matchhost__match(void) +{ + cl_git_pass(gitno__match_host("*.example.org", "www.example.org")); + cl_git_pass(gitno__match_host("*.foo.example.org", "www.foo.example.org")); + cl_git_fail(gitno__match_host("*.foo.example.org", "foo.example.org")); + cl_git_fail(gitno__match_host("*.foo.example.org", "www.example.org")); + cl_git_fail(gitno__match_host("*.example.org", "example.org")); + cl_git_fail(gitno__match_host("*.example.org", "www.foo.example.org")); + cl_git_fail(gitno__match_host("*.example.org", "blah.www.www.example.org")); +} From 51d3f6f5f2f9dc6c9f9dd64d3ccbd0afdcf6fb6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 26 Apr 2014 14:16:42 +0200 Subject: [PATCH 2/3] netops: provide more specific error for cert failure Specify what we do not like about the certificate. In this case, we do not like the name. --- src/netops.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/netops.c b/src/netops.c index 23f482b12..1e1832112 100644 --- a/src/netops.c +++ b/src/netops.c @@ -321,7 +321,7 @@ static int verify_server_cert(gitno_ssl *ssl, const char *host) GENERAL_NAMES_free(alts); if (matched == 0) - goto cert_fail; + goto cert_fail_name; if (matched == 1) return 0; @@ -358,11 +358,11 @@ static int verify_server_cert(gitno_ssl *ssl, const char *host) int size = ASN1_STRING_to_UTF8(&peer_cn, str); GITERR_CHECK_ALLOC(peer_cn); if (memchr(peer_cn, '\0', size)) - goto cert_fail; + goto cert_fail_name; } if (check_host_name((char *)peer_cn, host) < 0) - goto cert_fail; + goto cert_fail_name; OPENSSL_free(peer_cn); @@ -372,9 +372,9 @@ on_error: OPENSSL_free(peer_cn); return ssl_set_error(ssl, 0); -cert_fail: +cert_fail_name: OPENSSL_free(peer_cn); - giterr_set(GITERR_SSL, "Certificate host name check failed"); + giterr_set(GITERR_SSL, "hostname does not match certificate"); return -1; } From 783555d8e11516fdc01b66da0f873f5854b9bff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 26 Apr 2014 14:36:32 +0200 Subject: [PATCH 3/3] netops: catch the server not sending a certificate It's possible for an encrypted connection not have a certificate. In this case, SSL_get_verify_result() will return OK because no error happened (as it never even tried to validate anything). SSL_get_peer_certificate() will return NULL in this case so we need to catch that. On the upside, the current code would segfault in this situation instead of letting it through as a valid cert. --- src/netops.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/netops.c b/src/netops.c index 1e1832112..24092c17f 100644 --- a/src/netops.c +++ b/src/netops.c @@ -287,6 +287,10 @@ static int verify_server_cert(gitno_ssl *ssl, const char *host) cert = SSL_get_peer_certificate(ssl->ssl); + if (!cert) { + giterr_set(GITERR_SSL, "the server did not provide a certificate"); + return -1; + } /* Check the alternative names */ alts = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);