Subversion's Serf RA layer does not correctly validate certificates with wildcards in them for HTTPS. Summary: ======== Subversion's Serf RA layer improperly evaluates wildcards for HTTPS. This means it will accept certificates that it should not accept as matching the hostname the client is using to make the request. This can lead to a man-in-the-middle attack. There are no known instances of this problem being exploited in the wild and in practice it should be difficult to actually exploit this vulnerability. Known vulnerable: ================= Subversion clients using Serf 1.4.0 through 1.7.17 (inclusive) Subversion clients using Serf 1.8.0 through 1.8.9 (inclusive) Known fixed: ============ Subversion 1.7.18 Subversion 1.8.10 Subversion clients not using HTTPS or using Neon. Details: ======== Using the Serf RA layer of Subversion for HTTPS uses the apr_fnmatch API to handle matching wildcards in certificate Common Names and Subject Alternate Names. However, apr_fnmatch is not designed for this purpose. Instead it is designed to behave like common shell globbing. In particular this means that '*' is not limited to a single label within a hostname (i.e. it will match '.'). But even further apr_fnmatch supports '?' and character classes (neither of which are part of the RFCs defining how certificate validation works). For HTTPS URLs Subversion has historically been able to use one of two HTTP libraries, Serf or Neon. Serf support was first added as a build time option in Subversion 1.4.0, which meant only Serf or Neon was available in a given build of Subversion. Starting with 1.5.0 Subversion supported building support for both Serf and Neon and choosing which one to use based on a run-time configuration option. Subversion 1.8.0 removed support for Neon. Clients using Neon are not vulnerable to this issue, since the Neon library itself implements the name validation and does so correctly. Users can see if their client supports serf by running `svn --version` and looking to see if the ra_serf repository access module is available. Severity: ========= CVSSv2 Base Score: 4.0 CVSSv2 Base Vector: AV:N/AC:H/Au:N/C:P/I:P/A:N We consider this to be a medium risk vulnerability. Taking advantage of this vulnerability to execute a man in the middle attack requires access to the network or DNS of the client in order to redirect it to the attackers server. Additionally, the attacker needs a certificate signed by an issuer that the client trusts. The most concerning aspect of this vulnerability is the '*' wildcard matching across hostname labels. This for instance could allow a certificate with the Common Name set to only "*" to be matched against any hostname. Another possibility would be for an attacker to try and get a certificate signed for a domain they control that is a suffix for the domain they wish to attack. For example if attacking "example.com" the attacker could try registering "ample.com" and then request a certificate for "*ample.com" (note the lack of a '.' between the '*' and 'a'). However, issuers should not be signing such certificates. This issue when combined with the Serf vulnerability CVE-2014-3504, does however, make things slightly worse. CVE-2014-3504 means that serf does not properly handle certificates with embedded NUL bytes in their Common Names or Subject Alternate Names. So an attacker could request a certificate with the Common Name of "*\0.example.com" where "\0" is a NUL byte and ".example.com" is a domain the attacker owns. Such a certificate would be matched for any hostname. Again issuers should not be signing such certificates. There is of course no requirement that the attacker use certificates signed by a Certifying Authority. However, if an end user is going to accept the certificate (without fingerprint verification) despite an error telling them it is not certified by a trusted authority then this vulnerability is not needed to man-in-the-middle that user. Since the attacker can simply generate a certificate with the expected values. As such, useful exploitation of this vulnerability comes from getting a trusted Certifying Authority to sign a certificate the client will accept. A successful man-in-the-middle attack would give the attacker access to the plaintext of the encrypted communications between the client and the server. This would expose any information from the repository the client has requested. It may expose authentication credentials which can be used by the attacker to impersonate the user of the attacked client and thus gain access to more information or even be able to modify the repository. In all cases the attacker would still be subject to any authorization rules configured on the repository. Recommendations: ================ We recommend all users to upgrade to Subversion 1.8.10. Users of Subversion 1.7.x or 1.8.x who are unable to upgrade may apply the included patch. We also recommend that all users upgrade to Serf 1.3.7 or newer to resolve CVE-2014-3504. New Subversion packages can be found at: http://subversion.apache.org/packages.html There are no workarounds available. However, some of the risk can be mitigated by users taking some care. Users should not accept certificates that are not signed by a trusted certifying authority, without verifying their authenticity via the fingerprint. Users may also configure their clients not to trust certifying authorities (or decrease the number of certifying authorities they trust). Unfortunately, these mitigating steps require a great deal of care on the part of users, so upgrading the client immediately is the best course of action. References: =========== CVE-2014-3522 (Subversion) CVE-2014-3504 (Serf - different but related vulnerability) Reported by: ============ Ben Reser, WANdisco Patches: ======== Patch against 1.7.17: [[[ Index: subversion/include/private/svn_cert.h =================================================================== --- subversion/include/private/svn_cert.h (nonexistent) +++ subversion/include/private/svn_cert.h (working copy) @@ -0,0 +1,68 @@ +/** + * @copyright + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * @endcopyright + * + * @file svn_cert.h + * @brief Implementation of certificate validation functions + */ + +#ifndef SVN_CERT_H +#define SVN_CERT_H + +#include + +#include "svn_types.h" +#include "svn_string.h" + +#ifdef __cplusplus +extern "C" { +#endif /* __cplusplus */ + + +/* Return TRUE iff @a pattern matches @a hostname as defined + * by the matching rules of RFC 6125. In the context of RFC + * 6125 the pattern is the domain name portion of the presented + * identifier (which comes from the Common Name or a DNSName + * portion of the subjectAltName of an X.509 certificate) and + * the hostname is the source domain (i.e. the host portion + * of the URI the user entered). + * + * @note With respect to wildcards we only support matching + * wildcards in the left-most label and as the only character + * in the left-most label (i.e. we support RFC 6125 s. 6.4.3 + * Rule 1 and 2 but not the optional Rule 3). This may change + * in the future. + * + * @note Subversion does not at current support internationalized + * domain names. Both values are presumed to be in NR-LDH label + * or A-label form (see RFC 5890 for the definition). + * + * @since New in 1.9. + */ +svn_boolean_t +svn_cert__match_dns_identity(svn_string_t *pattern, svn_string_t *hostname); + + +#ifdef __cplusplus +} +#endif /* __cplusplus */ + +#endif /* SVN_CERT_H */ Index: subversion/libsvn_ra_serf/util.c =================================================================== --- subversion/libsvn_ra_serf/util.c (revision 1615128) +++ subversion/libsvn_ra_serf/util.c (working copy) @@ -28,7 +28,6 @@ #define APR_WANT_STRFUNC #include #include -#include #include #include @@ -40,6 +39,7 @@ #include "svn_xml.h" #include "private/svn_dep_compat.h" #include "private/svn_fspath.h" +#include "private/svn_cert.h" #include "ra_serf.h" @@ -202,7 +202,10 @@ ssl_server_cert(void *baton, int failures, apr_hash_t *issuer, *subject, *serf_cert; apr_array_header_t *san; void *creds; - int found_matching_hostname = 0; + svn_boolean_t found_matching_hostname = FALSE; + svn_boolean_t found_san_entry = FALSE; + svn_string_t *actual_hostname = + svn_string_create(conn->hostname, scratch_pool); /* Implicitly approve any non-server certs. */ if (serf_ssl_cert_depth(cert) > 0) @@ -237,30 +240,42 @@ ssl_server_cert(void *baton, int failures, | conn->server_cert_failures); /* Try to find matching server name via subjectAltName first... */ - if (san) { + if (san) + { int i; - for (i = 0; i < san->nelts; i++) { + found_san_entry = san->nelts > 0; + for (i = 0; i < san->nelts; i++) + { char *s = APR_ARRAY_IDX(san, i, char*); - if (apr_fnmatch(s, conn->hostname, - APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_SUCCESS) + svn_string_t *cert_hostname = svn_string_create(s, scratch_pool); + + if (svn_cert__match_dns_identity(cert_hostname, actual_hostname)) { - found_matching_hostname = 1; + found_matching_hostname = TRUE; cert_info.hostname = s; break; } - } - } + } + } - /* Match server certificate CN with the hostname of the server */ - if (!found_matching_hostname && cert_info.hostname) + /* Match server certificate CN with the hostname of the server iff + * we didn't find any subjectAltName fields and try to match them. + * Per RFC 2818 they are authoritative if present and CommonName + * should be ignored. */ + if (!found_matching_hostname && !found_san_entry && cert_info.hostname) { - if (apr_fnmatch(cert_info.hostname, conn->hostname, - APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_FNM_NOMATCH) + svn_string_t *cert_hostname = svn_string_create(cert_info.hostname, + scratch_pool); + + if (svn_cert__match_dns_identity(cert_hostname, actual_hostname)) { - svn_failures |= SVN_AUTH_SSL_CNMISMATCH; + found_matching_hostname = TRUE; } } + if (!found_matching_hostname) + svn_failures |= SVN_AUTH_SSL_CNMISMATCH; + svn_auth_set_parameter(conn->session->wc_callbacks->auth_baton, SVN_AUTH_PARAM_SSL_SERVER_FAILURES, &svn_failures); Index: subversion/libsvn_subr/dirent_uri.c =================================================================== --- subversion/libsvn_subr/dirent_uri.c (revision 1615128) +++ subversion/libsvn_subr/dirent_uri.c (working copy) @@ -38,6 +38,7 @@ #include "dirent_uri.h" #include "private/svn_fspath.h" +#include "private/svn_cert.h" /* The canonical empty path. Can this be changed? Well, change the empty test below and the path library will work, not so sure about the fs/wc @@ -2631,3 +2632,81 @@ svn_urlpath__canonicalize(const char *uri, } return uri; } + + +/* -------------- The cert API (see private/svn_cert.h) ------------- */ + +svn_boolean_t +svn_cert__match_dns_identity(svn_string_t *pattern, svn_string_t *hostname) +{ + apr_size_t pattern_pos = 0, hostname_pos = 0; + + /* support leading wildcards that composed of the only character in the + * left-most label. */ + if (pattern->len >= 2 && + pattern->data[pattern_pos] == '*' && + pattern->data[pattern_pos + 1] == '.') + { + while (hostname_pos < hostname->len && + hostname->data[hostname_pos] != '.') + { + hostname_pos++; + } + /* Assume that the wildcard must match something. Rule 2 says + * that *.example.com should not match example.com. If the wildcard + * ends up not matching anything then it matches .example.com which + * seems to be essentially the same as just example.com */ + if (hostname_pos == 0) + return FALSE; + + pattern_pos++; + } + + while (pattern_pos < pattern->len && hostname_pos < hostname->len) + { + char pattern_c = pattern->data[pattern_pos]; + char hostname_c = hostname->data[hostname_pos]; + + /* fold case as described in RFC 4343. + * Note: We actually convert to lowercase, since our URI + * canonicalization code converts to lowercase and generally + * most certs are issued with lowercase DNS names, meaning + * this avoids the fold operation in most cases. The RFC + * suggests the opposite transformation, but doesn't require + * any specific implementation in any case. It is critical + * that this folding be locale independent so you can't use + * tolower(). */ + pattern_c = canonicalize_to_lower(pattern_c); + hostname_c = canonicalize_to_lower(hostname_c); + + if (pattern_c != hostname_c) + { + /* doesn't match */ + return FALSE; + } + else + { + /* characters match so skip both */ + pattern_pos++; + hostname_pos++; + } + } + + /* ignore a trailing period on the hostname since this has no effect on the + * security of the matching. See the following for the long explanation as + * to why: + * https://bugzilla.mozilla.org/show_bug.cgi?id=134402#c28 + */ + if (pattern_pos == pattern->len && + hostname_pos == hostname->len - 1 && + hostname->data[hostname_pos] == '.') + hostname_pos++; + + if (pattern_pos != pattern->len || hostname_pos != hostname->len) + { + /* end didn't match */ + return FALSE; + } + + return TRUE; +} Index: subversion/tests/libsvn_subr/dirent_uri-test.c =================================================================== --- subversion/tests/libsvn_subr/dirent_uri-test.c (revision 1615128) +++ subversion/tests/libsvn_subr/dirent_uri-test.c (working copy) @@ -37,6 +37,7 @@ #include "svn_pools.h" #include "svn_dirent_uri.h" #include "private/svn_fspath.h" +#include "private/svn_cert.h" #include "../svn_test.h" @@ -2821,6 +2822,145 @@ test_fspath_get_longest_ancestor(apr_pool_t *pool) return SVN_NO_ERROR; } +struct cert_match_dns_test { + const char *pattern; + const char *hostname; + svn_boolean_t expected; +}; + +static svn_error_t * +run_cert_match_dns_tests(struct cert_match_dns_test *tests, apr_pool_t *pool) +{ + struct cert_match_dns_test *ct; + apr_pool_t *iterpool = svn_pool_create(pool); + + for (ct = tests; ct->pattern; ct++) + { + svn_boolean_t result; + svn_string_t *pattern, *hostname; + + svn_pool_clear(iterpool); + + pattern = svn_string_create(ct->pattern, iterpool); + hostname = svn_string_create(ct->hostname, iterpool); + + result = svn_cert__match_dns_identity(pattern, hostname); + if (result != ct->expected) + return svn_error_createf(SVN_ERR_TEST_FAILED, NULL, + "Expected %s but got %s for pattern '%s' on " + "hostname '%s'", + ct->expected ? "match" : "no match", + result ? "match" : "no match", + pattern->data, hostname->data); + + } + + svn_pool_destroy(iterpool); + + return SVN_NO_ERROR; +} + +static struct cert_match_dns_test cert_match_dns_tests[] = { + { "foo.example.com", "foo.example.com", TRUE }, /* exact match */ + { "foo.example.com", "FOO.EXAMPLE.COM", TRUE }, /* case differences */ + { "FOO.EXAMPLE.COM", "foo.example.com", TRUE }, + { "*.example.com", "FoO.ExAmPlE.CoM", TRUE }, + { "*.ExAmPlE.CoM", "foo.example.com", TRUE }, + { "ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz", TRUE }, + { "abcdefghijklmnopqrstuvwxyz", "ABCDEFGHIJKLMNOPQRSTUVWXYZ", TRUE }, + { "foo.example.com", "bar.example.com", FALSE }, /* difference at start */ + { "foo.example.com", "foo.example.net", FALSE }, /* difference at end */ + { "foo.example.com", "foo.example.commercial", FALSE }, /* hostname longer */ + { "foo.example.commercial", "foo.example.com", FALSE }, /* pattern longer */ + { "foo.example.comcom", "foo.example.com", FALSE }, /* repeated suffix */ + { "foo.example.com", "foo.example.comcom", FALSE }, + { "foo.example.com.com", "foo.example.com", FALSE }, + { "foo.example.com", "foo.example.com.com", FALSE }, + { "foofoo.example.com", "foo.example.com", FALSE }, /* repeated prefix */ + { "foo.example.com", "foofoo.example.com", FALSE }, + { "foo.foo.example.com", "foo.example.com", FALSE }, + { "foo.example.com", "foo.foo.example.com", FALSE }, + { "foo.*.example.com", "foo.bar.example.com", FALSE }, /* RFC 6125 s. 6.4.3 + Rule 1 */ + { "*.example.com", "foo.example.com", TRUE }, /* RFC 6125 s. 6.4.3 Rule 2 */ + { "*.example.com", "bar.foo.example.com", FALSE }, /* Rule 2 */ + { "*.example.com", "example.com", FALSE }, /* Rule 2 */ + { "*.example.com", ".example.com", FALSE }, /* RFC doesn't say what to do + here and a leading period on + a hostname doesn't make sense + so we'll just reject this. */ + { "*", "foo.example.com", FALSE }, /* wildcard must be left-most label, + implies that there must be more than + one label. */ + { "*", "example.com", FALSE }, + { "*", "com", FALSE }, + { "*.example.com", "foo.example.net", FALSE }, /* difference in literal text + with a wildcard. */ + { "*.com", "example.com", TRUE }, /* See Errata ID 3090 for RFC 6125, + probably shouldn't allow this but + we do for now. */ + { "*.", "example.com", FALSE }, /* test some dubious 2 character wildcard + patterns */ + { "*.", "example.", TRUE }, /* This one feels questionable */ + { "*.", "example", FALSE }, + { "*.", ".", FALSE }, + { "a", "a", TRUE }, /* check that single letter exact matches work */ + { "a", "b", FALSE }, /* and single letter not matches shouldn't */ + { "*.*.com", "foo.example.com", FALSE }, /* unsupported wildcards */ + { "*.*.com", "example.com", FALSE }, + { "**.example.com", "foo.example.com", FALSE }, + { "**.example.com", "example.com", FALSE }, + { "f*.example.com", "foo.example.com", FALSE }, + { "f*.example.com", "bar.example.com", FALSE }, + { "*o.example.com", "foo.example.com", FALSE }, + { "*o.example.com", "bar.example.com", FALSE }, + { "f*o.example.com", "foo.example.com", FALSE }, + { "f*o.example.com", "bar.example.com", FALSE }, + { "foo.e*.com", "foo.example.com", FALSE }, + { "foo.*e.com", "foo.example.com", FALSE }, + { "foo.e*e.com", "foo.example.com", FALSE }, + { "foo.example.com", "foo.example.com.", TRUE }, /* trailing dot */ + { "*.example.com", "foo.example.com.", TRUE }, + { "foo", "foo.", TRUE }, + { "foo.example.com.", "foo.example.com", FALSE }, + { "*.example.com.", "foo.example.com", FALSE }, + { "foo.", "foo", FALSE }, + { "foo.example.com", "foo.example.com..", FALSE }, + { "*.example.com", "foo.example.com..", FALSE }, + { "foo", "foo..", FALSE }, + { "foo.example.com..", "foo.example.com", FALSE }, + { "*.example.com..", "foo.example.com", FALSE }, + { "foo..", "foo", FALSE }, + { NULL } +}; + +static svn_error_t * +test_cert_match_dns_identity(apr_pool_t *pool) +{ + return run_cert_match_dns_tests(cert_match_dns_tests, pool); +} + +/* This test table implements results that should happen if we supported + * RFC 6125 s. 6.4.3 Rule 3. We don't so it's expected to fail for now. */ +static struct cert_match_dns_test rule3_tests[] = { + { "baz*.example.net", "baz1.example.net", TRUE }, + { "*baz.example.net", "foobaz.example.net", TRUE }, + { "b*z.example.net", "buuz.example.net", TRUE }, + { "b*z.example.net", "bz.example.net", FALSE }, /* presume wildcard can't + match nothing */ + { "baz*.example.net", "baz.example.net", FALSE }, + { "*baz.example.net", "baz.example.net", FALSE }, + { "b*z.example.net", "buuzuuz.example.net", TRUE }, /* presume wildcard + should be greedy */ + { NULL } +}; + +static svn_error_t * +test_rule3(apr_pool_t *pool) +{ + return run_cert_match_dns_tests(rule3_tests, pool); +} + /* The test table. */ @@ -2925,5 +3065,9 @@ struct svn_test_descriptor_t test_funcs[] = "test svn_fspath__dirname/basename/split"), SVN_TEST_PASS2(test_fspath_get_longest_ancestor, "test svn_fspath__get_longest_ancestor"), + SVN_TEST_PASS2(test_cert_match_dns_identity, + "test svn_cert__match_dns_identity"), + SVN_TEST_XFAIL2(test_rule3, + "test match with RFC 6125 s. 6.4.3 Rule 3"), SVN_TEST_NULL }; ]]] Patch against 1.8.9: [[[ Index: subversion/include/private/svn_cert.h =================================================================== --- subversion/include/private/svn_cert.h (nonexistent) +++ subversion/include/private/svn_cert.h (working copy) @@ -0,0 +1,68 @@ +/** + * @copyright + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * @endcopyright + * + * @file svn_cert.h + * @brief Implementation of certificate validation functions + */ + +#ifndef SVN_CERT_H +#define SVN_CERT_H + +#include + +#include "svn_types.h" +#include "svn_string.h" + +#ifdef __cplusplus +extern "C" { +#endif /* __cplusplus */ + + +/* Return TRUE iff @a pattern matches @a hostname as defined + * by the matching rules of RFC 6125. In the context of RFC + * 6125 the pattern is the domain name portion of the presented + * identifier (which comes from the Common Name or a DNSName + * portion of the subjectAltName of an X.509 certificate) and + * the hostname is the source domain (i.e. the host portion + * of the URI the user entered). + * + * @note With respect to wildcards we only support matching + * wildcards in the left-most label and as the only character + * in the left-most label (i.e. we support RFC 6125 s. 6.4.3 + * Rule 1 and 2 but not the optional Rule 3). This may change + * in the future. + * + * @note Subversion does not at current support internationalized + * domain names. Both values are presumed to be in NR-LDH label + * or A-label form (see RFC 5890 for the definition). + * + * @since New in 1.9. + */ +svn_boolean_t +svn_cert__match_dns_identity(svn_string_t *pattern, svn_string_t *hostname); + + +#ifdef __cplusplus +} +#endif /* __cplusplus */ + +#endif /* SVN_CERT_H */ Index: subversion/libsvn_ra_serf/util.c =================================================================== --- subversion/libsvn_ra_serf/util.c (revision 1615128) +++ subversion/libsvn_ra_serf/util.c (working copy) @@ -28,7 +28,6 @@ #define APR_WANT_STRFUNC #include #include -#include #include #include @@ -49,6 +48,7 @@ #include "private/svn_fspath.h" #include "private/svn_subr_private.h" #include "private/svn_auth_private.h" +#include "private/svn_cert.h" #include "ra_serf.h" @@ -274,7 +274,6 @@ ssl_server_cert(void *baton, int failures, apr_hash_t *subject = NULL; apr_hash_t *serf_cert = NULL; void *creds; - int found_matching_hostname = 0; svn_failures = (ssl_convert_serf_failures(failures) | conn->server_cert_failures); @@ -286,26 +285,37 @@ ssl_server_cert(void *baton, int failures, ### This should really be handled by serf, which should pass an error for this case, but that has backwards compatibility issues. */ apr_array_header_t *san; + svn_boolean_t found_san_entry = FALSE; + svn_boolean_t found_matching_hostname = FALSE; + svn_string_t *actual_hostname = + svn_string_create(conn->session->session_url.hostname, scratch_pool); serf_cert = serf_ssl_cert_certificate(cert, scratch_pool); san = svn_hash_gets(serf_cert, "subjectAltName"); /* Try to find matching server name via subjectAltName first... */ - if (san) { + if (san) + { int i; - for (i = 0; i < san->nelts; i++) { + found_san_entry = san->nelts > 0; + for (i = 0; i < san->nelts; i++) + { const char *s = APR_ARRAY_IDX(san, i, const char*); - if (apr_fnmatch(s, conn->session->session_url.hostname, - APR_FNM_PERIOD | APR_FNM_CASE_BLIND) == APR_SUCCESS) - { - found_matching_hostname = 1; + svn_string_t *cert_hostname = svn_string_create(s, scratch_pool); + + if (svn_cert__match_dns_identity(cert_hostname, actual_hostname)) + { + found_matching_hostname = TRUE; break; - } - } - } + } + } + } - /* Match server certificate CN with the hostname of the server */ - if (!found_matching_hostname) + /* Match server certificate CN with the hostname of the server iff + * we didn't find any subjectAltName fields and try to match them. + * Per RFC 2818 they are authoritative if present and CommonName + * should be ignored. */ + if (!found_matching_hostname && !found_san_entry) { const char *hostname = NULL; @@ -314,13 +324,20 @@ ssl_server_cert(void *baton, int failures, if (subject) hostname = svn_hash_gets(subject, "CN"); - if (!hostname - || apr_fnmatch(hostname, conn->session->session_url.hostname, - APR_FNM_PERIOD | APR_FNM_CASE_BLIND) != APR_SUCCESS) - { - svn_failures |= SVN_AUTH_SSL_CNMISMATCH; - } - } + if (hostname) + { + svn_string_t *cert_hostname = svn_string_create(hostname, + scratch_pool); + + if (svn_cert__match_dns_identity(cert_hostname, actual_hostname)) + { + found_matching_hostname = TRUE; + } + } + } + + if (!found_matching_hostname) + svn_failures |= SVN_AUTH_SSL_CNMISMATCH; } if (!svn_failures) Index: subversion/libsvn_subr/dirent_uri.c =================================================================== --- subversion/libsvn_subr/dirent_uri.c (revision 1615128) +++ subversion/libsvn_subr/dirent_uri.c (working copy) @@ -38,6 +38,7 @@ #include "dirent_uri.h" #include "private/svn_fspath.h" +#include "private/svn_cert.h" /* The canonical empty path. Can this be changed? Well, change the empty test below and the path library will work, not so sure about the fs/wc @@ -2597,3 +2598,81 @@ svn_urlpath__canonicalize(const char *uri, } return uri; } + + +/* -------------- The cert API (see private/svn_cert.h) ------------- */ + +svn_boolean_t +svn_cert__match_dns_identity(svn_string_t *pattern, svn_string_t *hostname) +{ + apr_size_t pattern_pos = 0, hostname_pos = 0; + + /* support leading wildcards that composed of the only character in the + * left-most label. */ + if (pattern->len >= 2 && + pattern->data[pattern_pos] == '*' && + pattern->data[pattern_pos + 1] == '.') + { + while (hostname_pos < hostname->len && + hostname->data[hostname_pos] != '.') + { + hostname_pos++; + } + /* Assume that the wildcard must match something. Rule 2 says + * that *.example.com should not match example.com. If the wildcard + * ends up not matching anything then it matches .example.com which + * seems to be essentially the same as just example.com */ + if (hostname_pos == 0) + return FALSE; + + pattern_pos++; + } + + while (pattern_pos < pattern->len && hostname_pos < hostname->len) + { + char pattern_c = pattern->data[pattern_pos]; + char hostname_c = hostname->data[hostname_pos]; + + /* fold case as described in RFC 4343. + * Note: We actually convert to lowercase, since our URI + * canonicalization code converts to lowercase and generally + * most certs are issued with lowercase DNS names, meaning + * this avoids the fold operation in most cases. The RFC + * suggests the opposite transformation, but doesn't require + * any specific implementation in any case. It is critical + * that this folding be locale independent so you can't use + * tolower(). */ + pattern_c = canonicalize_to_lower(pattern_c); + hostname_c = canonicalize_to_lower(hostname_c); + + if (pattern_c != hostname_c) + { + /* doesn't match */ + return FALSE; + } + else + { + /* characters match so skip both */ + pattern_pos++; + hostname_pos++; + } + } + + /* ignore a trailing period on the hostname since this has no effect on the + * security of the matching. See the following for the long explanation as + * to why: + * https://bugzilla.mozilla.org/show_bug.cgi?id=134402#c28 + */ + if (pattern_pos == pattern->len && + hostname_pos == hostname->len - 1 && + hostname->data[hostname_pos] == '.') + hostname_pos++; + + if (pattern_pos != pattern->len || hostname_pos != hostname->len) + { + /* end didn't match */ + return FALSE; + } + + return TRUE; +} Index: subversion/tests/libsvn_subr/dirent_uri-test.c =================================================================== --- subversion/tests/libsvn_subr/dirent_uri-test.c (revision 1615128) +++ subversion/tests/libsvn_subr/dirent_uri-test.c (working copy) @@ -37,6 +37,7 @@ #include "svn_pools.h" #include "svn_dirent_uri.h" #include "private/svn_fspath.h" +#include "private/svn_cert.h" #include "../svn_test.h" @@ -2714,6 +2715,145 @@ test_fspath_get_longest_ancestor(apr_pool_t *pool) return SVN_NO_ERROR; } +struct cert_match_dns_test { + const char *pattern; + const char *hostname; + svn_boolean_t expected; +}; + +static svn_error_t * +run_cert_match_dns_tests(struct cert_match_dns_test *tests, apr_pool_t *pool) +{ + struct cert_match_dns_test *ct; + apr_pool_t *iterpool = svn_pool_create(pool); + + for (ct = tests; ct->pattern; ct++) + { + svn_boolean_t result; + svn_string_t *pattern, *hostname; + + svn_pool_clear(iterpool); + + pattern = svn_string_create(ct->pattern, iterpool); + hostname = svn_string_create(ct->hostname, iterpool); + + result = svn_cert__match_dns_identity(pattern, hostname); + if (result != ct->expected) + return svn_error_createf(SVN_ERR_TEST_FAILED, NULL, + "Expected %s but got %s for pattern '%s' on " + "hostname '%s'", + ct->expected ? "match" : "no match", + result ? "match" : "no match", + pattern->data, hostname->data); + + } + + svn_pool_destroy(iterpool); + + return SVN_NO_ERROR; +} + +static struct cert_match_dns_test cert_match_dns_tests[] = { + { "foo.example.com", "foo.example.com", TRUE }, /* exact match */ + { "foo.example.com", "FOO.EXAMPLE.COM", TRUE }, /* case differences */ + { "FOO.EXAMPLE.COM", "foo.example.com", TRUE }, + { "*.example.com", "FoO.ExAmPlE.CoM", TRUE }, + { "*.ExAmPlE.CoM", "foo.example.com", TRUE }, + { "ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz", TRUE }, + { "abcdefghijklmnopqrstuvwxyz", "ABCDEFGHIJKLMNOPQRSTUVWXYZ", TRUE }, + { "foo.example.com", "bar.example.com", FALSE }, /* difference at start */ + { "foo.example.com", "foo.example.net", FALSE }, /* difference at end */ + { "foo.example.com", "foo.example.commercial", FALSE }, /* hostname longer */ + { "foo.example.commercial", "foo.example.com", FALSE }, /* pattern longer */ + { "foo.example.comcom", "foo.example.com", FALSE }, /* repeated suffix */ + { "foo.example.com", "foo.example.comcom", FALSE }, + { "foo.example.com.com", "foo.example.com", FALSE }, + { "foo.example.com", "foo.example.com.com", FALSE }, + { "foofoo.example.com", "foo.example.com", FALSE }, /* repeated prefix */ + { "foo.example.com", "foofoo.example.com", FALSE }, + { "foo.foo.example.com", "foo.example.com", FALSE }, + { "foo.example.com", "foo.foo.example.com", FALSE }, + { "foo.*.example.com", "foo.bar.example.com", FALSE }, /* RFC 6125 s. 6.4.3 + Rule 1 */ + { "*.example.com", "foo.example.com", TRUE }, /* RFC 6125 s. 6.4.3 Rule 2 */ + { "*.example.com", "bar.foo.example.com", FALSE }, /* Rule 2 */ + { "*.example.com", "example.com", FALSE }, /* Rule 2 */ + { "*.example.com", ".example.com", FALSE }, /* RFC doesn't say what to do + here and a leading period on + a hostname doesn't make sense + so we'll just reject this. */ + { "*", "foo.example.com", FALSE }, /* wildcard must be left-most label, + implies that there must be more than + one label. */ + { "*", "example.com", FALSE }, + { "*", "com", FALSE }, + { "*.example.com", "foo.example.net", FALSE }, /* difference in literal text + with a wildcard. */ + { "*.com", "example.com", TRUE }, /* See Errata ID 3090 for RFC 6125, + probably shouldn't allow this but + we do for now. */ + { "*.", "example.com", FALSE }, /* test some dubious 2 character wildcard + patterns */ + { "*.", "example.", TRUE }, /* This one feels questionable */ + { "*.", "example", FALSE }, + { "*.", ".", FALSE }, + { "a", "a", TRUE }, /* check that single letter exact matches work */ + { "a", "b", FALSE }, /* and single letter not matches shouldn't */ + { "*.*.com", "foo.example.com", FALSE }, /* unsupported wildcards */ + { "*.*.com", "example.com", FALSE }, + { "**.example.com", "foo.example.com", FALSE }, + { "**.example.com", "example.com", FALSE }, + { "f*.example.com", "foo.example.com", FALSE }, + { "f*.example.com", "bar.example.com", FALSE }, + { "*o.example.com", "foo.example.com", FALSE }, + { "*o.example.com", "bar.example.com", FALSE }, + { "f*o.example.com", "foo.example.com", FALSE }, + { "f*o.example.com", "bar.example.com", FALSE }, + { "foo.e*.com", "foo.example.com", FALSE }, + { "foo.*e.com", "foo.example.com", FALSE }, + { "foo.e*e.com", "foo.example.com", FALSE }, + { "foo.example.com", "foo.example.com.", TRUE }, /* trailing dot */ + { "*.example.com", "foo.example.com.", TRUE }, + { "foo", "foo.", TRUE }, + { "foo.example.com.", "foo.example.com", FALSE }, + { "*.example.com.", "foo.example.com", FALSE }, + { "foo.", "foo", FALSE }, + { "foo.example.com", "foo.example.com..", FALSE }, + { "*.example.com", "foo.example.com..", FALSE }, + { "foo", "foo..", FALSE }, + { "foo.example.com..", "foo.example.com", FALSE }, + { "*.example.com..", "foo.example.com", FALSE }, + { "foo..", "foo", FALSE }, + { NULL } +}; + +static svn_error_t * +test_cert_match_dns_identity(apr_pool_t *pool) +{ + return run_cert_match_dns_tests(cert_match_dns_tests, pool); +} + +/* This test table implements results that should happen if we supported + * RFC 6125 s. 6.4.3 Rule 3. We don't so it's expected to fail for now. */ +static struct cert_match_dns_test rule3_tests[] = { + { "baz*.example.net", "baz1.example.net", TRUE }, + { "*baz.example.net", "foobaz.example.net", TRUE }, + { "b*z.example.net", "buuz.example.net", TRUE }, + { "b*z.example.net", "bz.example.net", FALSE }, /* presume wildcard can't + match nothing */ + { "baz*.example.net", "baz.example.net", FALSE }, + { "*baz.example.net", "baz.example.net", FALSE }, + { "b*z.example.net", "buuzuuz.example.net", TRUE }, /* presume wildcard + should be greedy */ + { NULL } +}; + +static svn_error_t * +test_rule3(apr_pool_t *pool) +{ + return run_cert_match_dns_tests(rule3_tests, pool); +} + /* The test table. */ @@ -2812,5 +2952,9 @@ struct svn_test_descriptor_t test_funcs[] = "test svn_fspath__dirname/basename/split"), SVN_TEST_PASS2(test_fspath_get_longest_ancestor, "test svn_fspath__get_longest_ancestor"), + SVN_TEST_PASS2(test_cert_match_dns_identity, + "test svn_cert__match_dns_identity"), + SVN_TEST_XFAIL2(test_rule3, + "test match with RFC 6125 s. 6.4.3 Rule 3"), SVN_TEST_NULL }; ]]]