dulwich-users team mailing list archive

[Tay Ray Chuan <rctay89@xxxxxxxxx>]

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.

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.

But I agree on the "return None" point - the failure is too mild.

-- 
Cheers,
Ray Chuan