non-canonical paths in authz sections

[David Glasser <gl...@mit.edu>]

It appears that if you accidentally put a trailing slash in a section
header in an authz file, like

[/some/path/]

it will never be matched.  (At least, that's what a bit of testing and
source diving reveals; there might be some cases where it does.)  This
happens for any sort of non-canonical path, but trailing slashes are
the easiest to do by accident.

This is really confusing and should be fixed.

Is the correct fix to allow authz file writers to write non-canonical
paths and canonicalize them when they're matched against, or to throw
an error if a non-canonical path is specified?

--dave

-- 
David Glasser | glasser@mit.edu | http://www.davidglasser.net/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: non-canonical paths in authz sections

[David Glasser <gl...@mit.edu>]

On 6/6/07, David Glasser <gl...@mit.edu> wrote:
>   (authz_get_path_access): Instead of directly looking up the sections
>    at "path" and "qualified_path", use authz_parse_exact_section to
>    enumerate over all sections and check whether the canonicalized
>    version of the section name is the section we're looking for.

David Anderson points out that we should try to keep this operation
constant time.

I'll do another patch (actually, pair of patches) which introduces a
section name canonicalizer to svn_config_read and svn_config_merge
instead.

--dave

-- 
David Glasser | glasser@mit.edu | http://www.davidglasser.net/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: non-canonical paths in authz sections

[David Glasser <gl...@mit.edu>]

On 6/5/07, C. Michael Pilato <cm...@collab.net> wrote:
> David Glasser wrote:
> > It appears that if you accidentally put a trailing slash in a section
> > header in an authz file, like
> >
> > [/some/path/]
> >
> > it will never be matched.  (At least, that's what a bit of testing and
> > source diving reveals; there might be some cases where it does.)  This
> > happens for any sort of non-canonical path, but trailing slashes are
> > the easiest to do by accident.
> >
> > This is really confusing and should be fixed.
> >
> > Is the correct fix to allow authz file writers to write non-canonical
> > paths and canonicalize them when they're matched against, or to throw
> > an error if a non-canonical path is specified?
>
> Bummer thing about an error in this situation is that the folks who see the
> error might not be able to fix it (users vs. admins).  Besides,
> canonicalizing the paths automatically seems the more user-friendly thing to do.

The following patch should fix this issue.  Since there are security
concerns here, I'd appreciate review before I commit.  Also, I only
ran tests against svnserve; if somebody could run tests against a DAV
setup that would be appreciated.

(There are two classes of security concern here.  First, I could
simply have introduced a bug.  Secondly, this technically isn't
backwards compatible.  There could be installations out there that
have non-canonical blocks that are being ignored which will suddenly
start taking effect --- does anyone think this is a problem?)

--dave

[[[
When parsing authz files, canonicalize path names in section headers.
Among other things, this means that a section name with a trailing
slash won't be silently ignored.

* subversion/libsvn_repos/authz.c
  (authz_parse_section, authz_global_parse_section): Canonicalize
   section name.
  (authz_parse_exact_section): New section enumerator callback for
   authz_get_path_access.
  (authz_get_path_access): Instead of directly looking up the sections
   at "path" and "qualified_path", use authz_parse_exact_section to
   enumerate over all sections and check whether the canonicalized
   version of the section name is the section we're looking for.

* subversion/tests/cmdline/authz_tests.py
  (authz_read_access): Add a trailing slash to one of the authz
   section names.  (In the absence of the other changes in this
   revision, this causes the test to fail.)
]]]

-- 
David Glasser | glasser@mit.edu | http://www.davidglasser.net/

Re: non-canonical paths in authz sections

["C. Michael Pilato" <cm...@collab.net>]

David Glasser wrote:
> It appears that if you accidentally put a trailing slash in a section
> header in an authz file, like
> 
> [/some/path/]
> 
> it will never be matched.  (At least, that's what a bit of testing and
> source diving reveals; there might be some cases where it does.)  This
> happens for any sort of non-canonical path, but trailing slashes are
> the easiest to do by accident.
> 
> This is really confusing and should be fixed.
> 
> Is the correct fix to allow authz file writers to write non-canonical
> paths and canonicalize them when they're matched against, or to throw
> an error if a non-canonical path is specified?

Bummer thing about an error in this situation is that the folks who see the
error might not be able to fix it (users vs. admins).  Besides,
canonicalizing the paths automatically seems the more user-friendly thing to do.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand