[PATCH] Fix bug in recursive authz logic (Was: Re: problem with AUTHZ_SVN_RECURSIVE in mod_authz_svn.c)

David Anderson <[email protected]> Sat, 23 Jul 2005 06:29:13 +0200
Newsgroups gmane.comp.version-control.subversion.devel,gmane.mail.eyebrowse.user
Message-ID <42E1C799.6040706__29531.7592136145$1122093187$gmane$org@calixo.net> 
I'm crossposting this to dev@ for review of my patches. Any followup 
discussion can probably stay on dev@ (remove users@ if replying, but 
keep Cc-ing to Bernd).

Bernd Rinn wrote:
> I think that I have found a bug in mod_authz_svn.c of svn 1.2.1 with
> respect to operations that require AUTHZ_SVN_RECURSIVE access.

Thanks for your report and the effort you put into identifying the cause 
of the problem!  You are indeed right, this is a nasty bug in the authz 
algorithm.

I have recently being altering the authz code quite wildly in trunk.  As 
such, your patch no longer applies at all (the relevant code is now in 
libsvn_repos).  I have ported your fix to be applicable to trunk, and 
ask, if it is satisfactory to the commiters, that the fix be nominated 
for inclusion in the upcoming 1.2.2 release.

As for your concerns about separators, there is no problem: paths in 
authz files are pieces of URIs, and as such always use forward slashes, 
whatever the platform.

Commiters: I include two patches that should be commited separately:
  - bernd_rinn_authz_bug_1.2.patch corrects the bug in a way similar to 
Bernd Rinn's proposed fix, and should be nominated for inclusion in 1.2.2 .
  - bernd_rinn_authz_bug_1.3.patch alters the code of the previous patch 
to use svn_path_is_ancestor, an new API of libsvn_subr introduced for 
svn 1.3 . The previous patch basically reimplements this function 
internal to authz.c; this patch undoes that and uses the new API.

Here are the two commit messages:

[[[
Fix a bug in the authz recursive lookup logic, reported at
http://subversion.tigris.org/servlets/ReadMsg?list=users&msgNo=35734 .

Suggested by: Bernd Rinn <[email protected]>
Patch by: David Anderson <[email protected]>

* subversion/libsvn_repos/authz.c
   (authz_path_is_ancestor): New internal function.
   (authz_parse_section): use authz_path_is_ancestor to establish
     relationships between paths instead of just strncmp.
* subversion/tests/libsvn_repos/repos-test.c
   (authz): New regression test.
]]]

[[[
Port rXXXXX to the 1.3 API.

Patch by: David Anderson <[email protected]>

* subversion/libsvn_repos/authz.c
   (authz_path_is_ancestor): Delete internal function.
   (authz_parse_section): use svn_path_is_ancestor instead of
     authz_path_is_ancestor.
]]]

Thanks again for your help!
- Dave.

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
bernd_rinn_authz_bug_1.2.patch (text/x-patch, 3.3 KB) 
=== trunk/subversion/tests/libsvn_repos/repos-test.c
==================================================================
--- trunk/subversion/tests/libsvn_repos/repos-test.c  (revision 127)
+++ trunk/subversion/tests/libsvn_repos/repos-test.c  (revision 131)
@@ -1130,6 +1130,12 @@
    * and another containing a reference to an undefined group.  Verify
    * that svn_repos_authz_read fails to load both and returns an
    * "invalid configuration" error.
+   *
+   * 3. Regression test for a bug in how recursion is handled in
+   * authz.  The bug was that paths not under the parent path
+   * requested were being considered during the determination of
+   * access rights (eg. a rule for /dir2 matched during a lookup for
+   * /dir), due to incomplete tests on path relations.
    */
 
   /* The authz rules for the phase 1 tests. */
@@ -1254,6 +1260,33 @@
                               err ? "unexpected" : "no");
   svn_error_clear (err);
 
+  /* The authz rules for the phase 3 tests */
+  contents =
+    "[/]"
+    APR_EOL_STR
+    "* = rw"
+    APR_EOL_STR
+    APR_EOL_STR
+    "[greek:/dir2/secret]"
+    APR_EOL_STR
+    "* ="
+    APR_EOL_STR;
+
+  /* Load the test authz rules. */
+  SVN_ERR (authz_get_handle(&authz, contents, subpool));
+
+  /* Verify that the rule on /dir2/secret doesn't affect this
+     request */
+  SVN_ERR (svn_repos_authz_check_access (authz, "greek",
+                                         "/dir", NULL,
+                                         (svn_authz_read
+                                          | svn_authz_recursive),
+                                         &access_granted, subpool));
+  if (!access_granted)
+    return svn_error_create (SVN_ERR_TEST_FAILED, NULL,
+                             "Regression: incomplete ancestry test "
+                             "for recursive access lookup.");
+
   /* That's a wrap! */
   svn_pool_destroy (subpool);
   return SVN_NO_ERROR;
=== trunk/subversion/libsvn_repos/authz.c
==================================================================
--- trunk/subversion/libsvn_repos/authz.c  (revision 127)
+++ trunk/subversion/libsvn_repos/authz.c  (revision 131)
@@ -197,6 +197,24 @@
 
 
 
+/*
+ * Utility function for authz_parse_section, to determine whether
+ * path2 is a child of path1 (or *is* path1).
+ */
+static svn_boolean_t
+authz_path_is_ancestor (const char *path1, const char *path2)
+{
+  apr_size_t path1_len = strlen (path1);
+
+  if (strncmp (path1, path2, path1_len) == 0)
+    return path1[path1_len - 1] == '/'
+      || (path2[path1_len] == '/' || path2[path1_len] == '\0');
+
+  return FALSE;
+}
+
+
+
 /* Callback to parse a section and update the authz_baton if the
  * section denies access to the subtree the baton describes.
  */
@@ -207,10 +225,10 @@
   svn_boolean_t conclusive;
 
   /* Does the section apply to us? */
-  if (strncmp (section_name, b->qualified_repos_path,
-               strlen (b->qualified_repos_path)) != 0
-      && strncmp (section_name, b->repos_path,
-                  strlen (b->repos_path)) != 0)
+  if (authz_path_is_ancestor (b->qualified_repos_path,
+                              section_name) == FALSE
+      && authz_path_is_ancestor (b->repos_path,
+                                 section_name) == FALSE)
     return TRUE;
 
   /* Work out what this section grants. */
bernd_rinn_authz_bug_1.3.patch (text/x-patch, 1.4 KB) 
=== trunk/subversion/libsvn_repos/authz.c
==================================================================
--- trunk/subversion/libsvn_repos/authz.c  (revision 131)
+++ trunk/subversion/libsvn_repos/authz.c  (revision 132)
@@ -197,24 +197,6 @@
 
 
 
-/*
- * Utility function for authz_parse_section, to determine whether
- * path2 is a child of path1 (or *is* path1).
- */
-static svn_boolean_t
-authz_path_is_ancestor (const char *path1, const char *path2)
-{
-  apr_size_t path1_len = strlen (path1);
-
-  if (strncmp (path1, path2, path1_len) == 0)
-    return path1[path1_len - 1] == '/'
-      || (path2[path1_len] == '/' || path2[path1_len] == '\0');
-
-  return FALSE;
-}
-
-
-
 /* Callback to parse a section and update the authz_baton if the
  * section denies access to the subtree the baton describes.
  */
@@ -225,10 +207,10 @@
   svn_boolean_t conclusive;
 
   /* Does the section apply to us? */
-  if (authz_path_is_ancestor (b->qualified_repos_path,
-                              section_name) == FALSE
-      && authz_path_is_ancestor (b->repos_path,
-                                 section_name) == FALSE)
+  if (svn_path_is_ancestor (b->qualified_repos_path,
+                            section_name) == FALSE
+      && svn_path_is_ancestor (b->repos_path,
+                               section_name) == FALSE)
     return TRUE;
 
   /* Work out what this section grants. */