You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Michael Sinz <Mi...@sinz.org> on 2005/11/03 17:35:49 UTC
Re: reminder: Issue 2423 - correctly escaping ":" characters in file names in the repository when accessed via HTTP
On 11/3/05, Julian Foad <ju...@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)
--
Michael Sinz Technology and Engineering Director/Consultant
"Starting Startups" mailto:Michael.Sinz@sinz.org
My place on the web http://www.sinz.org/Michael.Sinz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: reminder: Issue 2423 - correctly escaping ":" characters in file
names in the repository when accessed via HTTP
Posted by Michael Sinz <Mi...@sinz.org>.
David James wrote:
> On 11/3/05, Michael Sinz <Mi...@sinz.org> wrote:
>
>>On 11/3/05, Julian Foad <ju...@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.
Thanks. BTW - the same patch is also relevant in the 1.2.x line and may
be of interest for those who are worried about the potential hole.
> 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.
That was what I found too. And, just to make sure the list sees this,
the ap_escape_uri() is just aa #define to ap_os_escape_path() so it
is still the same routine, just with the correct flag to support
relative paths. (BTW - I think Apache mis-named the parameter, I had
to read the code twice to be sure of what they were intending it to do)
--
Michael Sinz Technology and Engineering Director/Consultant
"Starting Startups" mailto:michael.sinz@sinz.org
My place on the web http://www.sinz.org/Michael.Sinz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: reminder: Issue 2423 - correctly escaping ":" characters in file names in the repository when accessed via HTTP
Posted by David James <ja...@gmail.com>.
On 11/3/05, Michael Sinz <Mi...@sinz.org> wrote:
> On 11/3/05, Julian Foad <ju...@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