Last Comment Bug 619588 - (CVE-2010-4567) [SECURITY] Safety checks that disallow clicking for javascript: or data: URLs in the URL field can be evaded with prefixed whitespace
(CVE-2010-4567)
: [SECURITY] Safety checks that disallow clicking for javascript: or data: URLs...
Status: RESOLVED FIXED
[infrasec:input][ws:moderate]
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.10
: All All
: -- normal (vote)
: Bugzilla 3.2
Assigned To: Frédéric Buclin
: default-qa
Mentors:
javascript:this.length=4444444444444...
: 709573 (view as bug list)
Depends on:
Blocks: 835424 619566 620540 CVE-2011-0048
  Show dependency treegraph
 
Reported: 2010-12-15 18:05 PST by Alex Miller
Modified: 2013-01-28 10:06 PST (History)
9 users (show)
LpSolit: approval+
LpSolit: approval4.0+
LpSolit: blocking4.0+
LpSolit: approval3.6+
LpSolit: blocking3.6.4+
LpSolit: approval3.4+
LpSolit: blocking3.4.10+
LpSolit: approval3.2+
LpSolit: blocking3.2.10+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (1.69 KB, patch)
2010-12-16 00:15 PST, Reed Loden [:reed] (use needinfo?)
mkanat: review+
LpSolit: review-
Details | Diff | Review
Alternative patch v.1 (2.55 KB, patch)
2010-12-16 10:04 PST, Gervase Markham [:gerv]
no flags Details | Diff | Review
patch, v2 (6.41 KB, patch)
2010-12-18 06:44 PST, Frédéric Buclin
dkl: review+
Details | Diff | Review
patch for 3.2 - 4.0, v1 (3.22 KB, patch)
2011-01-22 17:32 PST, Frédéric Buclin
dkl: review+
Details | Diff | Review
patch for 4.1, v3 (6.30 KB, patch)
2011-01-22 17:56 PST, Frédéric Buclin
dkl: review+
Details | Diff | Review

Description Alex Miller 2010-12-15 18:05:32 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.10 (maverick) Firefox/3.6.13
Build Identifier: 

Inspired by bug 619566, I felt compelled to file a bug with a more believable testcase for an XSS flaw (as in, anyone who reads the bug URL realizes it's an XSS attempt).

Reproducible: Always

Steps to Reproduce:
1. Click the bug URL provided

Actual Results:  
It alerts my document cookies

Expected Results:  
Input such as that should be filtered before actually going into the database.
Comment 1 Gervase Markham [:gerv] 2010-12-15 18:11:22 PST
Yeah, our regexp is, er, sub-optimal. edit.html.tmpl:

        [% IF bug.bug_file_loc 
           AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
          <a href="[% bug.bug_file_loc FILTER html %]"><u>U</u>RL</a>
        [% ELSE %]
          <u>U</u>RL
        [% END %]

The space at the start of the URL here makes it not match, so the URL gets linked.

Gerv
Comment 2 Alex Miller 2010-12-15 18:14:02 PST
(In reply to comment #1)
> Yeah, our regexp is, er, sub-optimal. edit.html.tmpl:
> 
>         [% IF bug.bug_file_loc 
>            AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
>           <a href="[% bug.bug_file_loc FILTER html %]"><u>U</u>RL</a>
>         [% ELSE %]
>           <u>U</u>RL
>         [% END %]
> 
> The space at the start of the URL here makes it not match, so the URL gets
> linked.
> 
> Gerv
Easy fix?
Comment 3 Max Kanat-Alexander 2010-12-15 18:17:45 PST
Note that no theft of user credentials is possible here. The user credentials are httponly.
Comment 4 Max Kanat-Alexander 2010-12-15 18:18:31 PST
And yeah, that should be an easy fix.
Comment 5 Alex Miller 2010-12-15 18:18:44 PST
(In reply to comment #3)
> Note that no theft of user credentials is possible here. The user credentials
> are httponly.
Comment 6 Alex Miller 2010-12-15 18:19:38 PST
Changed the title accordingly. Presumably an XMLHttprequest in the javascript could post the credentials to evil.com...
Comment 7 Max Kanat-Alexander 2010-12-15 18:20:36 PST
Actually, even the session ID is httponly.
Comment 8 Max Kanat-Alexander 2010-12-15 18:21:12 PST
And yeah, and XMLHTTPRequest could access them, but not to evil.com, which is a different domain. So there's not really any significant risk there, except for sites that have multiple untrusted systems on the same domain name.
Comment 9 Alex Miller 2010-12-15 18:25:10 PST
(In reply to comment #8)
> And yeah, and XMLHTTPRequest could access them, but not to evil.com, which is a
> different domain. So there's not really any significant risk there, except for
> sites that have multiple untrusted systems on the same domain name.

Here's a long shot way of exploiting it, what if somebody modifies the javascript to post the session ID in a comment, and evil.com just searches bugzilla constantly for these session IDs?
Comment 10 Alex Miller 2010-12-15 18:25:50 PST
(In reply to comment #9)
> (In reply to comment #8)
> > And yeah, and XMLHTTPRequest could access them, but not to evil.com, which is a
> > different domain. So there's not really any significant risk there, except for
> > sites that have multiple untrusted systems on the same domain name.
> 
> Here's a long shot way of exploiting it, what if somebody modifies the
> javascript to post the session ID in a comment, and evil.com just searches
> bugzilla constantly for these session IDs?

Correction: Posts it to pastebin.mozilla.org?
Comment 11 Alex Miller 2010-12-15 20:06:22 PST
Also, you could forge actions on bugzilla with the victim's priveledges, which could be pretty bad for someone like Daniel Veditz or Jesse Ruderman.
Comment 13 Reed Loden [:reed] (use needinfo?) 2010-12-16 00:15:33 PST
Created attachment 498049 [details] [diff] [review]
patch - v1

I changed the regex from "^(javascript|data)" to "^\s*(javascript|data):".

This fixes two problems:
1) Allow any whitespace before the protocol.
2) Restricts to match protocols instead of anything that might start with "javascript" or "data".

I could possibly change it to be "^\b(javascript|data)\b" for simplification, if wanted...
Comment 14 Max Kanat-Alexander 2010-12-16 00:16:15 PST
Comment on attachment 498049 [details] [diff] [review]
patch - v1

Looks good to me.
Comment 15 Reed Loden [:reed] (use needinfo?) 2010-12-16 00:21:40 PST
Mr. mozilla11@mailinator.com... If you will provide your name, we will include you in the security advisory that will go out for this problem. However, you're free to remain anonymous, if you would like.
Comment 16 mozilla11@mailinator.com 2010-12-16 04:38:20 PST
Em...

so

1). Stealing credentials, it's impossible, but it's possible to do all kinds of interactions with bugzilla on behalf of user via XMLHttpRequest(), - i.e. stealing his username, profile, list of bugs etc.

2). No regexp will work. Some or all of the below will work in different browsers

javascr[nul_byte]ipt:
[tab]javascript:
javascri&#70;t:

The only true way of fixing this is checking for http:// https:// prefixes (i.e. using whitelists instead of blacklists).
Comment 17 Frédéric Buclin 2010-12-16 08:47:38 PST
Comment on attachment 498049 [details] [diff] [review]
patch - v1

Sorry, but this patch doesn't fix the problem for me. The URL is still linkified.
Comment 18 Frédéric Buclin 2010-12-16 08:58:34 PST
(In reply to comment #16)
> javascri&#70;t:

I tried this with Firefox 4.0b7, but this didn't trigger JS. Maybe is it correctly escaped?

Anyway, this bug exists since Bugzilla 1.x, as it was already present in bug_form.tcl.
Comment 19 Gervase Markham [:gerv] 2010-12-16 10:04:51 PST
Created attachment 498136 [details] [diff] [review]
Alternative patch v.1

This doesn't work, because TT can't see SAFE_PROTOCOLS. But it can see other constants, so I don't know why that is. Still, if we can make that work, I think this is a better approach. (Although it might also need a lc() called on the protocol at some point.)

Gerv
Comment 20 Frédéric Buclin 2010-12-18 06:44:42 PST
Created attachment 498514 [details] [diff] [review]
patch, v2

TT can read SAFE_PROTOCOLS, you just have to write it correctly. ;)

I refactored the code a bit to avoid code duplication between Template.pm, attachment/edit.html.tmpl and bug/*.html.tmpl. When handling security, it's better to centralize checks. My code now forbids colons in local file names. This could break e.g. links pointing to wiki, if they were in a subdirectory of bugzilla/, but this is usually not the case and users are free to paste absolute URLs to avoid this restriction.
Comment 21 Daniel Veditz [:dveditz] 2010-12-20 15:33:57 PST
(In reply to comment #11)
> could be pretty bad for someone like Daniel Veditz or Jesse Ruderman.

Except we're both going to run a javascript URL in a sandbox anyway and not rely on bugzilla mechanisms.
Comment 22 Daniel Veditz [:dveditz] 2010-12-21 15:29:27 PST
CVE-2010-4567
Comment 23 Gervase Markham [:gerv] 2011-01-07 05:45:05 PST
Comment on attachment 498514 [details] [diff] [review]
patch, v2

>+    my $safe_protocols = SAFE_URL_REGEXP();

Nit: rename this variable, as it is no longer about just protocols.

>+  [% safe_url = "" %]
>+  [% IF is_safe_url(bug.bug_file_loc) %]
>+    [% safe_url = bug.bug_file_loc %]
>+  [%# In case the protocol is not specified, and there is no colon or other
>+      unwanted characters, then we assume it's a local file and we prepend
>+      the urlbase to prevent abuse. %]
>+  [% ELSIF bug.bug_file_loc.match('^[^\s<>\":]+[\w\/]$') %]
>+    [% safe_url = urlbase _ bug.bug_file_loc %]
>+  [% END %]

SAFE_URL_REGEXP requires a protocol, so the comment and the ELSIF branch are redundant.

>     [% IF bug.bug_file_loc %]
>+      [% safe_url = "" %]
>+      [% IF is_safe_url(bug.bug_file_loc) %]
>+        [% safe_url = bug.bug_file_loc %]
>+      [% ELSIF bug.bug_file_loc.match('^[^\s<>\":]+[\w\/]$') %]
>+        [% safe_url = urlbase _ bug.bug_file_loc %]
>+      [% END %]

Same comment here (in show-multiple).

Gerv
Comment 24 Frédéric Buclin 2011-01-07 06:29:33 PST
(In reply to comment #23)
> SAFE_URL_REGEXP requires a protocol, so the comment and the ELSIF branch are
> redundant.

I'm not sure why they are redundant. I explain why we would want to prepend the urlbase. I don't think one can easy get it without the comment. Also, not all protocols are safe, in which case you don't want to populate $safe_url.
Comment 25 Gervase Markham [:gerv] 2011-01-07 07:40:26 PST
Sorry, yes, I misread the code as thinking we did that check only on safe URLs, but in fact it does it if the URL is not safe. :-|

Gerv
Comment 26 David Lawrence [:dkl] 2011-01-10 17:36:10 PST
Comment on attachment 498514 [details] [diff] [review]
patch, v2

Looks good and works as expected in my testing. r=dkl
Comment 27 Frédéric Buclin 2011-01-22 17:32:43 PST
Created attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

Here is the backport for 3.6.
Comment 28 Frédéric Buclin 2011-01-22 17:39:42 PST
Comment on attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

This patch works for 3.2 - 3.6.
Comment 29 Frédéric Buclin 2011-01-22 17:56:45 PST
Created attachment 506176 [details] [diff] [review]
patch for 4.1, v3

Better patch for 4.1. I don't think prepending the urlbase is necessary when pointing to a local file. This makes the code much cleaner.
Comment 30 Frédéric Buclin 2011-01-22 18:02:11 PST
Comment on attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

OK, let's take this one for 4.0 too. The new constant is not mandatory here, and the patch for 4.1 doesn't apply cleanly on this branch.
Comment 31 Reed Loden [:reed] (use needinfo?) 2011-01-22 22:00:53 PST
Comment on attachment 506176 [details] [diff] [review]
patch for 4.1, v3

>+        [% IF NOT bug.check_can_change_field("bug_file_loc", 0, 1)
>+              AND is_safe_url(bug.bug_file_loc) %]
>           <a href="[% bug.bug_file_loc FILTER html %]">[% url_output FILTER none %]</a>

Can we also add the 'target' and 'title' parameters to this <a> (at least on trunk)?

Basically:
<a href="[% bug.bug_file_loc FILTER html %]" target="_blank" title="[% bug.bug_file_loc FILTER html %]">[% url_output FILTER none %]</a>
Comment 32 Frédéric Buclin 2011-01-23 03:30:14 PST
(In reply to comment #31)
> Can we also add the 'target' and 'title' parameters to this <a> (at least on
> trunk)?

No, this is a security bug, not an enhancement bug. You can file a separate RFE once 4.0rc2 is out, though.
Comment 33 David Lawrence [:dkl] 2011-01-23 06:20:09 PST
Comment on attachment 506176 [details] [diff] [review]
patch for 4.1, v3

Looks correct and works as expected. r=dkl
Comment 34 David Lawrence [:dkl] 2011-01-23 20:16:24 PST
Comment on attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

Looks good and works on all test versions. r=dkl
Comment 35 Frédéric Buclin 2011-01-24 10:41:20 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Template.pm
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/show-multiple.html.tmpl
Committed revision 7674.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Template.pm
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/show-multiple.html.tmpl
Committed revision 7531.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Template.pm
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/show-multiple.html.tmpl
Committed revision 7224.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/Template.pm
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/show-multiple.html.tmpl
Committed revision 6791.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.2/
modified Bugzilla/Template.pm
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/show-multiple.html.tmpl
Committed revision 6412.
Comment 36 Frédéric Buclin 2011-01-24 17:20:03 PST
Security advisory sent. Removing the security flag.
Comment 37 Frédéric Buclin 2011-12-11 09:39:38 PST
*** Bug 709573 has been marked as a duplicate of this bug. ***

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