243764 - Trailing slashes cause unexpected errors

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[timeless]

1. load http://bugzilla.mozilla.org/enter_bug.cgi/
2. select bugzilla
3. select creating/changing bugs
4. enter the url from 1.
5. enter the summary "Trailing slashes cause ie to get very unhappy"
6. enter this comment.
7. include comments 7, 8 and 9
8. enter expected results
9. click submit

expected results:
bug filed

actual results:
http://bugzilla.mozilla.org/enter_bug.cgi/post_bug.cgi
complete with the currently almost filed bug.

note that at this point you can continue making changes and clicking submit. 
your bug will never be filed. when you're tired. open a normal bug reporting 
page and copy all the values.

Comment 1

[Dave Miller [:justdave]]

Every browser I know will do that, not just IE.  That's part of the HTTP
standard. That's user error for putting a slash on the end of the URL.  It'd be
a pain in the butt to detect it and redirect you out of it, but I suppose it
could be done. Is it worth it?

Comment 2

[Joel Peshkin]

How did you get a URL with a trailing slash?

Comment 3

[Gervase Markham [:gerv]]

Arguably, the error is the web-server's - the URL timeless gives means "the
default resource in a directory called enter_bug.cgi", and such a thing doesn't
exist.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

The real issue is why did http://bugzilla.mozilla.org/enter_bug.cgi/ load a page
in the first place?

Comment 5

[Dave Miller [:justdave]]

That's how CGI works.  It's not the webserver's fault.  An executable CGI file
is treated as a directory to the end user, if there is additional pathname
tacked on the end of it.  The additional path beyond the name of the CGI itself
is passed in on the PATH_INFO environment variable.  The entire concept of how
LXR works is built around this. :)  Go take a look. :)

The easiest thing for us to do to short-circuit this from our end would be to
see if PATH_INFO contains anything in Bugzilla/CGI.pm and issue a 302 redirect
back to the same URL without the extra path on the end if there's stuff in it.

Comment 6

[Bradley Baetz (:bbaetz)]

I thought that there was extra apache config stuff for LXR. Can we turn this off
via apache, or add a rewrite rule?

Comment 7

[Dave Miller [:justdave]]

The only reason Apache needs extra config for Tinderbox is because the filename
of the script doesn't end in .cgi, so we have to tell Apache to override that
and treat it as a cgi anyway.

Comment 8

[Frédéric Buclin]

*** Bug 342842 has been marked as a duplicate of this bug. ***

Comment 12

[Frédéric Buclin]

Attachment: patch, v1

$ENV{PATH_INFO} can only be used to try to abuse Bugzilla or the browser as Bugzilla doesn't use it at all. To avoid unexpected behaviors or attacks, we should remove it from the URL entirely and resubmit the stripped URL.

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 633798 [details] [diff] [review]
patch, v1

Review of attachment 633798 [details] [diff] [review]:
-----------------------------------------------------------------

This patch breaks the REST extension I am working on which relies on path_info() to work properly. Is it possible to 
rework this patch to only redirect if path_info eq '/' or will that not fix the possible abuses originally proposed?

dkl

Comment 14

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #13)
> This patch breaks the REST extension I am working on which relies on
> path_info() to work properly. Is it possible to 
> rework this patch to only redirect if path_info eq '/' or will that not fix
> the possible abuses originally proposed?

This will not fix the abuses, see bugs marked as duplicates of this one.

Comment 15

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #12)
> $ENV{PATH_INFO} can only be used to try to abuse Bugzilla or the browser as
> Bugzilla doesn't use it at all. To avoid unexpected behaviors or attacks, we
> should remove it from the URL entirely and resubmit the stripped URL.

I've looked at the duplicate bugs and can't see any attacks. Do you have details on what security problems this may cause?

We shouldn't break this without good cause. I can certainly see how it would be useful to the REST extension.

Gerv

Comment 16

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #15)
> I've looked at the duplicate bugs and can't see any attacks. Do you have
> details on what security problems this may cause?

Some investigation is needed, but it seems this causes bug 765430. Also, http://search.cpan.org/~markstos/CGI/lib/CGI.pm#path_info reports that:

"NOTE: The Microsoft Internet Information Server is broken with respect to additional path information. If you use the Perl DLL library, the IIS server will attempt to execute the additional path information as a Perl script."

The "additional path information as a Perl script" seems dangerous as I'm not sure how this could be used.

Comment 17

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #16)
> "NOTE: The Microsoft Internet Information Server is broken with respect to
> additional path information. If you use the Perl DLL library, the IIS server
> will attempt to execute the additional path information as a Perl script."

Can we then only do this cleansing if the webs server is detected to be IIS and otherwise leave it alone? I will need to override the Bugzilla/CGI.pm method in the REST extension if we go forward with this for all web servers and I would rather not do (and maintain) that.

dkl

Comment 18

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #17)
> method in the REST extension

Does your REST extension really need to have access to all .cgi scripts? How do you parse the URL?

Comment 19

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #18)
> (In reply to David Lawrence [:dkl] from comment #17)
> > method in the REST extension
> 
> Does your REST extension really need to have access to all .cgi scripts? How
> do you parse the URL?

No. The REST extension has it's own entry point (rest.cgi) that need to have full access to the actual path_info data. If your patch could somehow use a blacklist and wipe out the path_info only for those cgis that would help me out in that case.

dkl

Comment 20

[Frédéric Buclin]

A blacklist seems a bad idea as all scripts should be blacklisted by default. A whitelist would make more sense. But we cannot whitelist a script which is not in the source code by default.

@glob: do you remember why you introduced "path_info => 1" in your patch for bug 38862 as we don't use it?

Comment 21

[:glob ✱]

(In reply to Frédéric Buclin from comment #20)
> @glob: do you remember why you introduced "path_info => 1" in your patch for
> bug 38862 as we don't use it?

if you're talking about redirect_to_urlbase, it's there to ensure the redirect changes just the urlbase; that function shouldn't alter anything else about the path.

(In reply to Frédéric Buclin from comment #20)
> A blacklist seems a bad idea as all scripts should be blacklisted by
> default. A whitelist would make more sense. But we cannot whitelist a script
> which is not in the source code by default.

a whitelist with a hook.

Comment 22

[Gervase Markham [:gerv]]

The REST API will eventually be shipped with Bugzilla, I strongly suggest and recommend, and therefore this will be a short-term problem. Still, a hook also seems fine.

Gerv

Comment 23

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #22)
> The REST API will eventually be shipped with Bugzilla, I strongly suggest
> and recommend, and therefore this will be a short-term problem. Still, a
> hook also seems fine.

I don't see how the REST API would change anything. None of the .cgi scripts understand nor use the path info. AFAIK, the REST API will point to a unique .cgi script, right?

Comment 24

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #23)
> (In reply to Gervase Markham [:gerv] from comment #22)
> > The REST API will eventually be shipped with Bugzilla, I strongly suggest
> > and recommend, and therefore this will be a short-term problem. Still, a
> > hook also seems fine.
> 
> I don't see how the REST API would change anything. None of the .cgi scripts
> understand nor use the path info. AFAIK, the REST API will point to a unique
> .cgi script, right?

REST uses the path info to determine what resource to load or action to take. For example for a client to access information about Bug 12345, it woudl typically use

https://bugzilla.mozilla.org/rest.cgi/bug/12345

where here /bug/12345 is the path_info and is parsed to load bug 12345. Of course using Apaches mod_rewrite I clean it up some to be https://bugzilla.mozilla.org/rest/bug/12345 but the path_info is the same.

There are many other variations of path_info to perform different tasks based also on request methods such as GET, POST, PUT, and DELETE but you get the idea.

dkl

Comment 25

[David Lawrence [:dkl]]

Comment on attachment 633798 [details] [diff] [review]
patch, v1

Review of attachment 633798 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl. I will have the REST extension workaround this by either utilizing mod_rewrite or by sub classing Bugzilla/CGI and
overriding new().

dkl

Comment 26

[Reed Loden [:reed]]

Comment on attachment 633798 [details] [diff] [review]
patch, v1

Review of attachment 633798 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/CGI.pm
@@ +57,5 @@
>      # Make sure our outgoing cookie list is empty on each invocation
>      $self->{Bugzilla_cookie_list} = [];
>  
> +    # Path-Info is of no use for Bugzilla and interacts badly with IIS.
> +    # Moreover, it causes unexepected behaviors, such as totally breaking

unexpected*

Comment 27

[Frédéric Buclin]

Pushing it in the approval queue for now. I have 200 bugmails to read first. :) I will clear the approval queue later today.

Comment 28

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #25)
> I will have the REST extension workaround this [...]

Note that once REST is part of the core code, it will be trivial to append
|if $script_name ne 'rest.cgi'| to the redirect, or something similar.


Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/CGI.pm
Committed revision 8297.