Last Comment Bug 824616 - The urlbase field in global/header.html.tmpl must be filtered
: The urlbase field in global/header.html.tmpl must be filtered
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Administration (show other bugs)
: unspecified
: All All
: -- minor (vote)
: Bugzilla 4.0
Assigned To: Matt Selsky [:selsky]
: default-qa
:
Mentors:
Depends on: 224242
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-25 03:57 PST by Mario Gomes
Modified: 2013-01-03 04:28 PST (History)
1 user (show)
LpSolit: approval+
LpSolit: approval4.4+
LpSolit: approval4.2+
LpSolit: approval4.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (643 bytes, patch)
2013-01-02 14:05 PST, Matt Selsky [:selsky]
LpSolit: review+
Details | Diff | Splinter Review

Description Mario Gomes 2012-12-25 03:57:55 PST
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.22 (KHTML, like Gecko) Chrome/25.0.1364.5 Safari/537.22

Steps to reproduce:

Hi,

There's a persistent xss in /editparams.cgi while editing the "sslbase" that allows attackers to steal user cookies. That change can be done only by admins, this can be used by ADMIN1 to get ADMIN2'S cookies.  

Reproduce:
1. Go to  https://localhost/bugzilla/editparams.cgi while logged as a admin
2. Change "sslbase" values to "'><img src=x onerror=confirm(3);>.
3. Save changes.
4. Go back to https://localhost/bugzilla/
5. See the alert.

That could be used to elevate privileges from a user that has power to edit "tweakparams" so the attacker can inject
the xss to the admin visit so possible elevate attacker's privilege or steal admin's cookies.

PoC: https://j6j9djxoqkc0.demo.bugzilla.org/

Cheers,
Mario
Comment 1 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-12-25 05:35:56 PST
assigned to rforbes for verification
Comment 2 Reed Loden [:reed] (use needinfo?) 2012-12-25 08:54:22 PST
While we should definitely add better validation to sslbase (and lots of other options), I think it's reasonable to say that if a user has tweakparams, he/she effectively has admin access already. Similar to how editusers is effectively admin, even without the bit explicitly set. As such, I don't consider this a valid security issue.
Comment 3 Frédéric Buclin 2012-12-25 10:20:26 PST
The string entered for the sslbase parameter must start with https:// and so you cannot type the string specified in point 2.

Moreover, we trust parameters set by administrators. We don't do a full validation on purpose. If you cannot trust your admins, then you are in trouble. This is not a bug.
Comment 4 Reed Loden [:reed] (use needinfo?) 2012-12-25 10:23:43 PST
(In reply to Mario Gomes from comment #0)
> PoC: https://j6j9djxoqkc0.demo.bugzilla.org/

I don't seem to be getting any type of XSS warning on the above URL. Can you show me exactly how you were getting it?
Comment 5 Mario Gomes 2012-12-26 02:05:50 PST
try it again. Anyway it can be exploited by the old clickjacking in attachment "Details" page.

(In reply to Reed Loden [:reed] from comment #4)
> (In reply to Mario Gomes from comment #0)
> > PoC: https://j6j9djxoqkc0.demo.bugzilla.org/
> 
> I don't seem to be getting any type of XSS warning on the above URL. Can you
> show me exactly how you were getting it?
Comment 6 Reed Loden [:reed] (use needinfo?) 2012-12-26 11:16:02 PST
  <body onload=""
        class="j6j9djxoqkc0-demo-bugzilla-org-aaaaa"'><img src=x onerror=confirm('XSS');>- yui-skin-sam">
Comment 7 Mario Gomes 2012-12-26 11:21:13 PST
Yep, that's it.

(In reply to Reed Loden [:reed] from comment #6)
>   <body onload=""
>         class="j6j9djxoqkc0-demo-bugzilla-org-aaaaa"'><img src=x
> onerror=confirm('XSS');>- yui-skin-sam">
Comment 8 Reed Loden [:reed] (use needinfo?) 2012-12-26 11:31:46 PST
  <body onload="[% onload %]"
        class="[% urlbase.replace('^https?://','').replace('/$','').replace('[-~@:/.]+','-') %]
               [% FOREACH class = bodyclasses %]
                 [% ' ' %][% class FILTER css_class_quote %]
               [% END %] yui-skin-sam">

Doesn't look like we're filtering it.
Comment 9 Frédéric Buclin 2012-12-27 09:20:22 PST
(In reply to Reed Loden [:reed] from comment #8)
> urlbase.replace('^https?://','').replace('/$','').replace('[-~@:/.]+','-') %]
> 
> Doesn't look like we're filtering it.

t/008filter.t doesn't complain because the regexp contains the +, - and / characters, which are seen as mathematical operators:

    return 1 if $directive =~ /[+\-*\/]/;

This looks like the single place where urlbase is not filtered. As it's used to define a CSS class name, it should probably use FILTER css_class_quote (which is a ugly function, btw, see bug 474877).
Comment 10 Mario Gomes 2012-12-27 09:41:39 PST
"security issue" or not I can exploit it with only a click(using clickjacking on attachment details page). And nobody here takes this seriously. So, what do you think about a flaw that result in persistent xss with only a user click? Is it not a security issue? 

(In reply to Reed Loden [:reed] from comment #2)
> I don't consider this a valid security issue.
Comment 11 Mario Gomes 2012-12-27 09:46:58 PST
Yeah that isn't a bug, that's a "feature" + a "clickjacking" = that becomes persistent XSS. That's why I love you buddies. You always try minimize the bug that is easily exploitable. So, is it really "INVALID"?

(In reply to Frédéric Buclin from comment #3)
> Moreover, we trust parameters set by administrators. We don't do a full
> validation on purpose. If you cannot trust your admins, then you are in
> trouble. This is not a bug.
Comment 12 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-12-27 11:13:58 PST
(In reply to Frédéric Buclin from comment #9)
> This looks like the single place where urlbase is not filtered. As it's used
> to define a CSS class name, it should probably use FILTER css_class_quote
> (which is a ugly function, btw, see bug 474877).

Which makes this sound like a real bug which should be fixed, to me.  Failure to filter.

That said, I'm 100% in agreement here that the security impact is non-existant here, because it requires admin privs to exploit, and would have to be intentionally exploited by an admin (we have CSRF protection in place on the params page). If you have an admin that's willing to do something like that, you have much bigger problems than XSS on your hands.

Which of course, does not mean we shouldn't fix it.  I think resolving it invalid just because it's not a security issue when it was initially filed as one is a little hasty.
Comment 13 Frédéric Buclin 2012-12-27 11:40:34 PST
(In reply to Mario Gomes from comment #11)
> Yeah that isn't a bug, that's a "feature" + a "clickjacking" = that becomes
> persistent XSS. That's why I love you buddies. You always try minimize the
> bug that is easily exploitable. So, is it really "INVALID"?

Yes, your XSS issue is really invalid, because it's not exploitable, despite what you try to say. We *trust* parameters entered by admins, period! Also, no Javascript code can steal login cookies, because they are protected against such attacks, so your last comment in comment 0 is wrong too.
Comment 14 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-12-27 11:41:45 PST
There is also a degree of "minimizing attack surface" here.  There's always the possibility that an administrator's account could be compromised in some other way (someone steals their password via a keylogger or social-engineers someone to do a password reset, etc).  If you have illicit access to an admin account there are far worse things than this that you could do with that access.  But the more you can limit the damage to things that are easy to detect/easily noticed, the better.
Comment 15 Matt Selsky [:selsky] 2013-01-02 14:05:06 PST
Created attachment 697200 [details] [diff] [review]
v1
Comment 16 Frédéric Buclin 2013-01-03 04:18:23 PST
Comment on attachment 697200 [details] [diff] [review]
v1

r=LpSolit
Comment 17 Frédéric Buclin 2013-01-03 04:28:19 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/global/header.html.tmpl
Committed revision 8546.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified template/en/default/global/header.html.tmpl
Committed revision 8496.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified template/en/default/global/header.html.tmpl
Committed revision 8182.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified template/en/default/global/header.html.tmpl
Committed revision 7739.

Note You need to log in before you can comment on or make changes to this bug.