On 11/3/05, Michael Sinz <Michael.Sinz@sinz.org> wrote:
> On 11/3/05, Julian Foad <julianfoad@btopenworld.com> wrote:
> > Michael Sinz wrote:
> > > Patch is already attached to 2423 and I believe it to be correct.
> >
> > Please could you remind us what this issue is about by mentioning its subject
> > in the subject line? (Yes I know I could go to the issue and find out, but I'm
> > probably not interested in it so I'm not going to.)
>
> Sorry - I too want that in the subject.
>
> I have updated the subject.
>
> Also, more directly, the issue has to do with the fact that the escaping
> function called to make a valid URL does not deal with a ":" character in
> a relative path URL, which is what mod_dav_svn generates in the HTML
> and XML output. A simple change to call the same function with an
> argument telling it that the URL is relative and will not have a full path
> pre-pended to it fixes the problem. (Note that the call looks different
> since a #define is used to make the secondary parameter optional)
Thanks for your careful analysis, Michael! I agree, it is definitely a
good idea to use ap_os_escape_path instead of ap_escape_uri for
relative filename paths in mod_dav_svn. I would definitely like to see
your patch committed to both trunk and the 1.3.x line.
Looking at the code for mod_dav_svn, I see there are three uses of
"apr_escape_uri":
1) ap_escape_uri(r->pool, r->uri) (in get_parentpath_resource)
2) ap_escape_uri(r->pool, r->uri) (in dav_svn_get_resource)
3) ap_escape_uri(entry_pool, href) (in dav_svn_deliver)
In situations (1) and (2), we call "ap_escape_uri" on full URLs, so
this is fine. (Right?) In situation (3), we call "apr_escape_uri" on a
relative path, and thus we are vulnerable to malicious
cross-site-links. Your patch fixes situation (3) to use
ap_os_escape_path, which properly escapes colons in filenames. Looks
good to me.
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Received on Thu Nov 3 22:17:54 2005