Attachment 924404 Details for Bug 1127063 – Patch against subversion 1.8.9

[patch] Patch against subversion 1.8.9

CVE-2014-3522-svn-1.8.diff (text/plain), 16.61 KB, created by Tomas Hoger on 2014-08-06 08:44:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomas Hoger

Created: 2014-08-06 08:44:22 UTC

Size: 16.61 KB

patch

obsolete

>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 <apr.h>
>+
>+#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 <apr.h>
> #include <apr_want.h>
>-#include <apr_fnmatch.h>
> 
> #include <serf.h>
> #include <serf_bucket_types.h>
>@@ -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
>   };

Actions: View | Diff

Attachments on bug 1127063: 924402 | 924403 | 924404