Upstream-Status: Backport Signed-off-by: Jackie Huang 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 };