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

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
User Accounts
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

2.19.3
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +
blocking2.22 +
approval2.20 +
blocking2.20.2 +
blocking2.20.1 -

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
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. ***

Comment 5

12 years ago
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-

Comment 6

12 years ago
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.
(Assignee)

Comment 7

12 years ago
(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().
(Assignee)

Comment 8

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
Created attachment 211032 [details] [diff] [review]
possible fix, v1

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)
(Assignee)

Comment 10

12 years ago
Created attachment 211090 [details] [diff] [review]
better patch, v2

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+
(Assignee)

Comment 12

12 years ago
Created attachment 211095 [details] [diff] [review]
patch, v3

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+
(Assignee)

Comment 14

12 years ago
(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?
(Assignee)

Updated

12 years ago
Whiteboard: [ready for 2.20.1][ready for 2.22rc1]
(Assignee)

Comment 15

12 years ago
Created attachment 211115 [details] [diff] [review]
patch, v3.1

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+
(Assignee)

Updated

12 years ago
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]
(Assignee)

Updated

12 years ago
Version: 2.20 → 2.19.3
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
(Assignee)

Comment 16

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 17

12 years ago
Security Advisory posted; unlocking bug.
Group: webtools-security

Updated

12 years ago
Blocks: 328108

Comment 18

12 years ago
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)
(Assignee)

Updated

12 years ago
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
(Assignee)

Updated

8 years ago
Blocks: 329638
(Assignee)

Updated

5 years ago
Blocks: 765189
You need to log in before you can comment on or make changes to this bug.