[PATCH] Issue #1949: mod_authz_svn COPY bug

* Ben Collins-Sussman <sussman@collab.net> [2004-07-07 15:37]:
> ### 1949 mod_authz_svn COPY bug: sussman documented a simple way to
> fix it, mthelen volunteered to implement it. Also eed to doc that
> it dampers the O(1) copy behavior.
>
> MTHELEN WILL HAVE PATCH FOR REVIEW ON THURSDAY

As promised, here is the patch. It doesn't include any documentation
changes to point out the dampering of O(1) behavior for copies, though.

Log:
Fix issue #1949. Check COPY requests to make sure the user has read access to
the entire subtree of the path being copied.

* subversion/mod_authz_svn/mod_authz_svn.c
  (AUTHZ_SVN_READ_TREE): New access type, requiring read access to a path and
    every location beneath that path.

  (sections_matching_prefix_baton): New baton type for finding config sections
    that match a prefix.

  (parse_authz_lines): Add parens to clarify precedence.

  (find_sections_matching_prefix): New callback function to determine whether
    a config section name matches a prefix. If it matches, the section name
    is pushed onto an array in the baton. This is used to find all config
    sections for locations beneath a particular path.

  (check_access_subtree): New function to check access to the subtree beneath
    a path. Access to the path itself is not checked, and is assumed to be
    allowed. Access to the subtree is considered to be disallowed only if it
    is restricted by a configuration option in the subtree. Also, the actual
    existence of locations in the subtree is completely ignored. If access to
    a location beneath the path is disallowed, all access to the subtree is
    disallowed regardless of whether the restricted location actually exists
    in the repository.

  (check_access): Rearrange the logic of a do-while loop to make it a while
    loop. Also, if access of AUTHZ_SVN_READ_TREE is required, then call
    check_access_subtree after doing the normal check for read access.

  (req_check_access): Require AUTHZ_SVN_READ_TREE access for the source path
    of a COPY request.

* subversion/include/svn_config.h
  (svn_config_section_enumerator_t): New callback to be called for each
    section in a svn_config_t.

  (svn_config_enumerate_sections): New function that calls a callback for
    each section in a svn_config_t.

  (svn_config_enumerate): Fix a typo in the comment header.

* subversion/libsvn_subr/config.c
  (svn_config_enumerate_sections): New function that calls a callback for
    each section in a svn_config_t.

Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 10171)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -40,8 +40,9 @@
 
 enum {
     AUTHZ_SVN_NONE = 0,
- AUTHZ_SVN_READ,
- AUTHZ_SVN_WRITE
+ AUTHZ_SVN_READ = 1,
+ AUTHZ_SVN_WRITE = 2,
+ AUTHZ_SVN_READ_TREE = 4
 };
 
 typedef struct {
@@ -59,6 +60,12 @@
     int deny;
 };
 
+struct sections_matching_prefix_baton {
+ apr_pool_t *pool;
+ svn_config_t *config;
+ const char *prefix;
+ apr_array_header_t *sections;
+};
 
 /*
  * Configuration
@@ -178,7 +185,7 @@
     svn_config_enumerate(cfg, qualified_repos_path,
                          parse_authz_line, &baton);
     *access = !(baton.deny & required_access)
- || (baton.allow & required_access) != 0;
+ || ((baton.allow & required_access) != 0);
 
     if ((baton.deny & required_access)
         || (baton.allow & required_access))
@@ -187,18 +194,109 @@
     svn_config_enumerate(cfg, repos_path,
                          parse_authz_line, &baton);
     *access = !(baton.deny & required_access)
- || (baton.allow & required_access) != 0;
+ || ((baton.allow & required_access) != 0);
 
     return (baton.deny & required_access)
            || (baton.allow & required_access);
 }
 
-static int check_access(svn_config_t *cfg,
- const char *repos_name, const char *repos_path,
- const char *user, int required_access,
- apr_pool_t *pool)
+
+static svn_boolean_t find_sections_matching_prefix(const char* sec_name,
+ void *baton)
 {
+ struct sections_matching_prefix_baton *b = baton;
+
+ if (strncmp(sec_name, b->prefix, strlen(b->prefix)) == 0)
+ (*((const char **)(apr_array_push(b->sections)))) = sec_name;
+
+ return TRUE;
+}
+
+
+/* Check whether access is allowed to all subtrees of a @a repos_path. This
+ * does *NOT* check access to the path itself or any of its parent
+ * directories. It assumes that access to the path is allowed, and that
+ * access to the subtree is only disallowed if restricted by a configuration
+ * option in the subtree.
+ *
+ * This function also does not check to see whether each portion of the
+ * subtree actually exists in the repository. If access is denied to a
+ * directory beneath @a repos_path, then access will be denied whether that
+ * directory actually exists or not.
+ *
+ * Return the resulting access as @a *access.
+ */
+static int check_access_subtree(svn_config_t *cfg, const char *repos_name,
+ const char *repos_path, const char *user,
+ int required_access, int *access,
+ apr_pool_t *pool)
+{
+ const char *qualified_repos_path;
+ const char *section;
+ apr_array_header_t *list;
+ int i;
+
+ struct sections_matching_prefix_baton sec_baton = { 0 };
+ struct parse_authz_line_baton authz_baton = { 0 };
+
+ sec_baton.pool = pool;
+ sec_baton.config = cfg;
+ sec_baton.sections = apr_array_make(pool, 0, sizeof(const char *));
+
+ /* First try repos specific */
+ qualified_repos_path = apr_pstrcat(pool, repos_name, ":", repos_path,
+ NULL);
+
+ sec_baton.prefix = qualified_repos_path;
+ svn_config_enumerate_sections(cfg, find_sections_matching_prefix,
+ &sec_baton);
+
+ authz_baton.pool = pool;
+ authz_baton.config = cfg;
+ authz_baton.user = user;
+
+ list = sec_baton.sections;
+ for (i = 0; i < list->nelts; ++i) {
+ section = ((const char **) list->elts)[i];
+ authz_baton.allow = authz_baton.deny = 0;
+
+ svn_config_enumerate(cfg, section, parse_authz_line, &authz_baton);
+ *access = !(authz_baton.deny & required_access)
+ || ((authz_baton.allow & required_access) != 0);
+
+ if (!*access)
+ return TRUE;
+ }
+
+ /* Now try with no repos specified */
+ sec_baton.prefix = repos_path;
+ svn_config_enumerate_sections(cfg, find_sections_matching_prefix,
+ &sec_baton);
+
+ list = sec_baton.sections;
+ for (i = 0; i < list->nelts; ++i) {
+ section = ((const char **) list->elts)[i];
+ authz_baton.allow = authz_baton.deny = 0;
+
+ svn_config_enumerate(cfg, section, parse_authz_line, &authz_baton);
+ *access = !(authz_baton.deny & required_access)
+ || ((authz_baton.allow & required_access) != 0);
+
+ if (!*access)
+ return TRUE;
+ }
+
+ return TRUE;
+}
+
+
+static int check_access(svn_config_t *cfg, const char *repos_name,
+ const char *repos_path, const char *user,
+ int required_access, apr_pool_t *pool)
+{
     const char *base_name;
+ const char *orig_repos_path;
+ int require_subtree = FALSE;
     int access;
 
     if (!repos_path) {
@@ -209,13 +307,17 @@
         return 1;
     }
 
- if (parse_authz_lines(cfg, repos_name, repos_path,
- user, required_access, &access,
- pool))
- return access;
+ /* Check regular access first, check subtree access after that */
+ if (required_access == AUTHZ_SVN_READ_TREE) {
+ require_subtree = TRUE;
+ required_access = AUTHZ_SVN_READ;
+ }
 
     base_name = repos_path;
- do {
+ orig_repos_path = repos_path;
+ while (!parse_authz_lines(cfg, repos_name, repos_path,
+ user, required_access, &access,
+ pool)) {
         if (base_name[0] == '/' && base_name[1] == '\0') {
             /* By default, deny access */
             return 0;
@@ -223,10 +325,13 @@
 
         svn_path_split(repos_path, &repos_path, &base_name, pool);
     }
- while (!parse_authz_lines(cfg, repos_name, repos_path,
- user, required_access, &access,
- pool));
 
+ /* Access is OK for this directory and parent directories, now see whether
+ * access is OK for subdirectories */
+ if (access && require_subtree)
+ check_access_subtree(cfg, repos_name, orig_repos_path, user,
+ required_access, &access, pool);
+
     return access;
 }
 
@@ -262,12 +367,16 @@
     /* All methods requiring read access to r->uri */
     case M_OPTIONS:
     case M_GET:
- case M_COPY:
     case M_PROPFIND:
     case M_REPORT:
         authz_svn_type = AUTHZ_SVN_READ;
         break;
 
+ /* All methods requiring read access to all subtrees of r->uri */
+ case M_COPY:
+ authz_svn_type = AUTHZ_SVN_READ_TREE;
+ break;
+
     /* All methods requiring write access to r->uri */
     case M_MOVE:
     case M_MKCOL:
Index: subversion/include/svn_config.h
===================================================================
--- subversion/include/svn_config.h (revision 10171)
+++ subversion/include/svn_config.h (working copy)
@@ -186,7 +186,27 @@
                           const char *section, const char *option,
                           svn_boolean_t value);
 
+/** A callback function used in enumerating config sections.
+ *
+ * See @c svn_config_enumerate_sections for the details of this type.
+ */
+typedef svn_boolean_t (*svn_config_section_enumerator_t)
+ (const char *name, void *baton);
 
+/** Enumerate the sections, passing @a baton and the current section's name to
+ * @a callback. Continue the enumeration if @a callback returns @c TRUE.
+ * Return the number of times @a callback was called.
+ *
+ * ### See kff's comment to @c svn_config_enumerate. It applies to this
+ * function, too. ###
+ *
+ * @a callback's @a name and @a name parameters are only valid for the
+ * duration of the call.
+ */
+int svn_config_enumerate_sections (svn_config_t *cfg,
+ svn_config_section_enumerator_t callback,
+ void *baton);
+
 /** A callback function used in enumerating config options.
  *
  * See @c svn_config_enumerate for the details of this type.
@@ -207,7 +227,7 @@
  * of an enumeration early, with no error, than an invocation of
  * @a callback is likely to need to return an error? ###
  *
- * @a callback's @a name and @a name parameters are only valid for the
+ * @a callback's @a name and @a value parameters are only valid for the
  * duration of the call.
  */
 int svn_config_enumerate (svn_config_t *cfg, const char *section,
Index: subversion/libsvn_subr/config.c
===================================================================
--- subversion/libsvn_subr/config.c (revision 10171)
+++ subversion/libsvn_subr/config.c (working copy)
@@ -645,7 +645,33 @@
 }
 
 
+
+int
+svn_config_enumerate_sections (svn_config_t *cfg,
+ svn_config_section_enumerator_t callback,
+ void *baton)
+{
+ apr_hash_index_t *sec_ndx;
+ int count = 0;
 
+ for (sec_ndx = apr_hash_first (cfg->x_pool, cfg->sections);
+ sec_ndx != NULL;
+ sec_ndx = apr_hash_next (sec_ndx))
+ {
+ void *sec_ptr;
+ cfg_section_t *sec;
+
+ apr_hash_this (sec_ndx, NULL, NULL, &sec_ptr);
+ sec = sec_ptr;
+ ++count;
+ if (!callback (sec->name, baton))
+ break;
+ }
+
+ return count;
+}
+
+
 
 int
 svn_config_enumerate (svn_config_t *cfg, const char *section,

-- Mike

-- 
Michael W. Thelen
Many would be cowards if they had courage enough.
                -- Thomas Fuller