dulwich-users team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

On Mon, Dec 27, 2010 at 02:07:18AM +0800, Tay Ray Chuan wrote:
> On Mon, Dec 27, 2010 at 1:56 AM, Augie Fackler <durin42@xxxxxxxxx> wrote:
> > On Dec 26, 2010, at 12:45 PM, Jelmer Vernooij wrote:

> >> On Mon, Dec 27, 2010 at 12:45:14AM +0800, Tay Ray Chuan wrote:
> >>> On Mon, Dec 27, 2010 at 12:36 AM, Jelmer Vernooij <jelmer@xxxxxxxxx> wrote:
> >>>> On Mon, Dec 27, 2010 at 12:15:45AM +0800, Tay Ray Chuan wrote:
> >>> [snip]
> >>>>> +def _norm_path(path):
> >>>>> + ? ?return os.path.normcase(os.path.realpath(path))
> >>>> Thanks for the patches.

> >>>> I'm not sure this is a useful thing to factor out.

> >>> It makes things neater. In the next patch (#4), we go through the
> >>> whole gamut again for the parent directory.

> >>>> Also, why the os.path.realpath? We're just going to open these files, why do we care
> >>>> about their canonical location?

> >>> A malicious user could ask for an path like

> >>>  /../some/file

> >>> realpath "escapes" these for us.
> >> dulwich.rpeo is the wrong place for server-side permission checks IMHO. At the
> >> very least we should raise an exception if the path is outside of the repository rather than returning None.

> >> In genera though, I think we should put checks like this into the server rather than here.

> > +1 on both points.

> (Strictly speaking, looking at the http server urls allowed, it's
> close to impossible to get ".." in paths from the server.) In any
> case, outside of the web server, do want to allow such "bad" file
> paths to be accessed? I think not. By having this at the repo-level,
> we can be sure of "safe" file accesses, regardless of who's requesting
> the file - the server, or elsewhere.
I'm worried about mixing code that doesn't do permission checking with
code that does, because it makes it unclear who is responsible for
doing these checks. None of the other Repo functions do these kinds of
checks, it should be the responsibility of the server layer.
Also, security checks are not without performance costs. E.g. realpath
resolves symlinks so might have to touch the disk so we should avoid
it if we can (e.g. local user access).

> Implementation-wise, like patch #2, putting it on the server-side
> makes things complicated because such "escaping" shouldn't be done for
> in-memory repos.
It would indeed make the server side more complicated, but it would
also make the code in dulwich.repo simpler. I don't really see how
this is an issue.

Cheers,

Jelmer