Re: Multiple matching lines in authz file

Stefan Sperling <stsp_at_elego.de> writes:

> This is quite a dangerous bug isn't it?
> Anyone relying on authz for confidentially would rightfully regard
> this as a security issue. I think we should fix the code.
> Most people have probably written their authz rules according to the
> book, so their authz rules may not work as expected.

Do they? Perhaps they write them by trial-and-error, testing to see
if they get the behaviour they want. In that case we will break authz
files that work.

> Can you test if the diff below fixes this?
>
> Index: subversion/libsvn_repos/authz.c
> ===================================================================
> --- subversion/libsvn_repos/authz.c (revision 899439)
> +++ subversion/libsvn_repos/authz.c (working copy)
> @@ -271,7 +271,7 @@ authz_parse_line(const char *name, const char *val
> else
> b->deny |= svn_authz_write;
>
> - return TRUE;
> + return FALSE;
> }

It doesn't appear to. I wrote the following test to verify the
current behaviour, it continues to PASS with your patch.

Index: subversion/tests/cmdline/authz_tests.py
===================================================================
--- subversion/tests/cmdline/authz_tests.py (revision 901125)
+++ subversion/tests/cmdline/authz_tests.py (working copy)
@@ -903,6 +903,39 @@
                        root_url + '/A-copy/B/E/beta',
                        root_url + '/A-copy/C')
 
+
+def multiple_matches(sbox):
+ "multiple lines matching a user"
+
+ sbox.build(create_wc = False)
+ root_url = sbox.repo_url
+ write_restrictive_svnserve_conf(sbox.repo_dir)
+ if sbox.repo_url.startswith("http"):
+ expected_err = ".*[Ff]orbidden.*"
+ else:
+ expected_err = ".*svn: Authorization failed.*"
+
+ # Disable access and commit fails
+ write_authz_file(sbox, {'/': 'jrandom ='})
+ svntest.actions.run_and_verify_svn(None, None, expected_err,
+ 'cp', '-m', 'fail copy',
+ root_url, root_url + '/fail')
+
+ # At present if multiple lines match the permissions of all the
+ # matching lines are amalgamated. So jrandom gets access regardless
+ # of the line excluding access and regardless of the order of the
+ # lines. This might be a bug.
+
+ write_authz_file(sbox, {'/': 'jrandom =' + '\n' + '* = rw'})
+ svntest.main.run_svn(None, 'cp',
+ '-m', 'first copy',
+ root_url, root_url + '/first')
+
+ write_authz_file(sbox, {'/': '* = rw' + '\n' + 'jrandom ='})
+ svntest.main.run_svn(None, 'cp',
+ '-m', 'second copy',
+ root_url, root_url + '/second')
+
 ########################################################################
 # Run the tests
 
@@ -928,6 +961,7 @@
                          svntest.main.is_ra_type_file)),
               Skip(authz_access_required_at_repo_root,
                    svntest.main.is_ra_type_file),
+ Skip(multiple_matches, svntest.main.is_ra_type_file),
              ]
 
 if __name__ == '__main__':

-- 
Philip