Closed Bug 325079 Opened 18 years ago Closed 18 years ago

[SECURITY] The login form on the Bugzilla home page may redirect your login and password to another site

Categories

(Bugzilla :: User Accounts, defect)

2.19.3
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: LpSolit, Assigned: LpSolit)

References

()

Details

(Whiteboard: [doesn't affect 2.16][doesn't affect 2.18][ready for 2.20.1][ready for 2.22rc1])

Attachments

(1 file, 3 obsolete files)

Most of the discussion is in bug 325058 already, but here is a short summary:

bug 325058 comment 0 (LpSolit):

This URL ( http://landfill.bugzilla.org//paul/ ) contains a double slash // and the login form on Bugzilla redirects me outside bugzilla.org, in my case paul.fr (but I suspect it depends on your locale, in my case: french).

Discussing with CTho on #developers, it looks like the login and password [may] have been sent to paul.fr instead of landfill.bugzilla.org, meaning that they have now my admin login and password for our QA installations.


bug 325058 comment 6 (dveditz):

Is this a landfill bug in the way it handles multiple installations? The form
action is derived from the URL. If you don't specify the double-slash and visit
the intended http://landfill.bugzilla.org/paul/ then the form action is
"/paul/index.cgi" which logs into landfill. If you use the double-slash it's
copied into the form action making 'paul' a hostname. Landfill should do some
sanity-checking when generating the form action, or specify the complete url.

Why is this filed as a networking bug? It looks like Gecko is doing exactly
what the page is telling it to do (submit the form to "paul").


bug 325058 comment 8 (LpSolit):

The problem is not specific to landfill, it affects *all* Bugzilla
installations.


The problem is that template/en/default/account/auth/login-small.html.tmpl
uses:

<form name="login" action="[% cgi.script_name FILTER html %]" method="POST">

so when you enter //foo, you get action="//foo/index.cgi" which redirects you
outside your Bugzilla installation with your login and password.

It could as well be a bug in CGI.pm itself. Anyway, Bugzilla should probably
use Param('urlbase').


-----

The template/en/default/account/auth/login-small.html.tmpl template was introduced in Bugzilla 2.20 in bug 165075. This means that only the home page is affected due to the use of 'cgi.script_name' which returns the full path, but you are not affected when you have to log in due to GoAheadAndLogIn=1 which is correctly handled by Auth::Login::WWW::CGI::login(). This routine indeed calls $cgi->url(-relative=>1) and so you cannot be redirected outside Param('urlbase') or Param('sslbase').

Probably the right fix is to do something similar for login-small.html.tmpl.
I don't know if we want to block 2.20.1 for it.
Flags: blocking2.22?
Flags: blocking2.20.2?
Flags: blocking2.20.1?
It should be noted that this is only an issue if the "foo" part of "//foo" is a valid URL - in the example given, no data is sent unless the address "paul" resolves to something. So the bugzilla install would have to be at an address like "www.domain.org/evil.com" for this to be exploitable, and it would be exploitable only by evil.com.
// is shorthand for a Windows network share, isn't it?  So this would look for an smb:// host on your local network named "paul" instead of http: if it's interpreting it as a hostname.
*** Bug 325058 has been marked as a duplicate of this bug. ***
I don't particularly see this as major. A user has to type a double slash, or be redirected to the site with a double slash. Now, granted, somebody could maliciously link to the site with a double slash, so that's exploitable. But for the most part, this would only be exploited by accident. After all, this bug has obviously been around for a long time, and we only just now discovered it.

I'm hesitant to block anything on it, but as it is a valid security bug, we should fix it before our *next* (not 2.20.1) release.
Severity: major → normal
Flags: blocking2.22?
Flags: blocking2.22+
Flags: blocking2.20.2?
Flags: blocking2.20.2+
Flags: blocking2.20.1?
Flags: blocking2.20.1-
Oh, by the way, I think the obvious simple fix is to modify the URL filter so that it strips off any slashes near the beginning of a URL other than the first one. It means that we won't be able to insert SMB network paths there, but... um... oh well. :-) I don't think we had any plans on that.
(In reply to comment #6)
> Oh, by the way, I think the obvious simple fix is to modify the URL filter so
> that it strips off any slashes near the beginning of a URL other than the first
> one. It means that we won't be able to insert SMB network paths there, but...
> um... oh well. :-) I don't think we had any plans on that.
> 

No need to change the URL filter. The right fix is to replace cgi.script_name by something like $cgi->url(-relative => 1), as done by Auth::Login::WWW::CGI::login().
(In reply to comment #5)
> for the most part, this would only be exploited by accident. After all, this
> bug has obviously been around for a long time, and we only just now discovered
> it.

"for a long time" isn't true. Only versions 2.19.3 and above are affected, with 2.20 being released on Sept 30, 2005; that's pretty recent. 2.19.2 and below are not affected as you couldn't log in from the home page.
Attached patch possible fix, v1 (obsolete) — Splinter Review
cgi.script_name will redirect you outside your installation if the URL contains //. So I now let the .cgi script pass this param.
Attachment #211032 - Flags: review?(justdave)
Attached patch better patch, v2 (obsolete) — Splinter Review
No need to hack index.cgi anymore.
Assignee: user-accounts → LpSolit
Attachment #211032 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #211090 - Flags: review?(myk)
Attachment #211032 - Flags: review?(justdave)
Comment on attachment 211090 [details] [diff] [review]
better patch, v2

>+[% DEFAULT homepage = "index.cgi" %]

Nit: this should probably be set globally, and perhaps it should be an absolute URL via URLPATH.
Attachment #211090 - Flags: review?(myk) → review+
Attached patch patch, v3 (obsolete) — Splinter Review
Use an absolute path. Now, //paul and //paul/index.cgi both fall back to /paul/index.cgi.
Attachment #211090 - Attachment is obsolete: true
Attachment #211095 - Flags: review?(myk)
Comment on attachment 211095 [details] [diff] [review]
patch, v3

>Index: template/en/default/account/auth/login-small.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login-small.html.tmpl,v
>retrieving revision 1.1
>diff -u -r1.1 login-small.html.tmpl
>--- template/en/default/account/auth/login-small.html.tmpl	12 Apr 2005 17:23:01 -0000	1.1
>+++ template/en/default/account/auth/login-small.html.tmpl	8 Feb 2006 02:21:24 -0000
>@@ -21,7 +21,20 @@
> 
> [% PROCESS global/variables.none.tmpl %]
> 
>-<form name="login" action="[% cgi.script_name FILTER html %]" method="POST">
>+[%# Use the current script name. If an empty name is retuned,
>+  # then we are accessing the home page. %]

Nit: retuned -> returned


>+[% DEFAULT homepage = "index.cgi" %]
>+[% script_name = cgi.url(Relative => 1) || homepage %]
>+
>+[%# If SSL is in use, use 'sslbase', else use 'urlbase'. %]
>+[% IF Param("sslbase") != "" && Param("ssl") != "never" %]
>+  [% script_name = Param("sslbase") _ script_name %]
>+[% ELSE %]
>+  [% script_name = Param("urlbase") _ script_name %]
>+[% END %]

I wouldn't think this particularly necessary unless cgi.url is empty, but I doubt it hurts anything, either.  It does seem like this stuff should happen globally, so all templates can take advantage of it.
Attachment #211095 - Flags: review?(myk) → review+
(In reply to comment #13)

> I wouldn't think this particularly necessary unless cgi.url is empty, but I
> doubt it hurts anything, either.  It does seem like this stuff should happen
> globally, so all templates can take advantage of it.
> 

Well, I could still write http://www.foo.com//paul/index.cgi. My patch force it to fall back to http://www.foo.com/paul/index.cgi in this case too.

About making it global, we could set <base href=""> in global/header.html.tmpl, but I'm not sure <form> really cares about <base>. Does it?
Blocks: 320312
Flags: approval?
Flags: approval2.20?
Whiteboard: [ready for 2.20.1][ready for 2.22rc1]
Attached patch patch, v3.1Splinter Review
Small improvement: do not hardcode 'index.cgi'. If we are accessing the homepage, the relative URL is empty, but as we are appending the full path to the installation (either urlbase or sslbase), we never end with an empty script_name and so we never have action="".
Attachment #211095 - Attachment is obsolete: true
Attachment #211115 - Flags: review?(myk)
Attachment #211115 - Flags: review?(myk) → review+
Whiteboard: [ready for 2.20.1][ready for 2.22rc1] → [doesn't affect 2.16][doesn't affect 2.18][ready for 2.20.1][ready for 2.22rc1]
Version: 2.20 → 2.19.3
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
tip:

Checking in template/en/default/account/auth/login-small.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login-small.html.tmpl,v  <--  login-small.html.tmpl
new revision: 1.2; previous revision: 1.1
done

2.20:

Checking in template/en/default/account/auth/login-small.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login-small.html.tmpl,v  <--  login-small.html.tmpl
new revision: 1.1.4.1; previous revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Security Advisory posted; unlocking bug.
Group: webtools-security
Blocks: 328108
Blocker regression caused by this bug at bug 328108 (maybe we should consider some of the previous versions, in order to avoid using urlbase in a pre-login stage)
Summary: The login form on the Bugzilla home page may redirect your login and password to another site → [SECURITY] The login form on the Bugzilla home page may redirect your login and password to another site
Blocks: 329638
Blocks: 765189
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: