Closed Bug 619588 (CVE-2010-4567) Opened 13 years ago Closed 13 years ago

[SECURITY] Safety checks that disallow clicking for javascript: or data: URLs in the URL field can be evaded with prefixed whitespace

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

2.10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: alexander.miller, Assigned: LpSolit)

References

()

Details

(Whiteboard: [infrasec:input][ws:moderate])

Attachments

(2 files, 3 obsolete files)

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.
Whiteboard: [sg:high]
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
(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?
Note that no theft of user credentials is possible here. The user credentials are httponly.
And yeah, that should be an easy fix.
(In reply to comment #3)
> Note that no theft of user credentials is possible here. The user credentials
> are httponly.
Summary: XSS flaw in Bugzilla can lead to theft of user credentials → XSS flaw in Bugzilla can lead to theft of session ID and other cookie information
Changed the title accordingly. Presumably an XMLHttprequest in the javascript could post the credentials to evil.com...
Actually, even the session ID is httponly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: XSS flaw in Bugzilla can lead to theft of session ID and other cookie information → JavaScript and Data URLs can be made clickable even though we don't intend to allow that, on show_bug.cgi
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.
(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?
(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?
OS: Linux → All
Hardware: x86 → All
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.
Whiteboard: [sg:high] → [ws:moderate]
Summary: JavaScript and Data URLs can be made clickable even though we don't intend to allow that, on show_bug.cgi → JavaScript and Data URLs can be made clickable even though we don't intend to allow that in show_bug.cgi (possibly resulting in XSRF)
Whiteboard: [ws:moderate] → [sg:high]
Whiteboard: [sg:high] → [ws:moderate]
Target Milestone: --- → Bugzilla 3.4
Version: unspecified → 3.6.3
Attached patch patch - v1 (obsolete) — Splinter Review
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...
Assignee: create-and-change → reed
Status: NEW → ASSIGNED
Attachment #498049 - Flags: review?(mkanat)
Comment on attachment 498049 [details] [diff] [review]
patch - v1

Looks good to me.
Attachment #498049 - Flags: review?(mkanat) → review+
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
Target Milestone: Bugzilla 3.4 → Bugzilla 3.2
Summary: JavaScript and Data URLs can be made clickable even though we don't intend to allow that in show_bug.cgi (possibly resulting in XSRF) → [SECURITY] Safety checks that disallow clicking for javascript: or data: URLs in the URL field can be evaded with prefixed whitespace
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.
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 on attachment 498049 [details] [diff] [review]
patch - v1

Sorry, but this patch doesn't fix the problem for me. The URL is still linkified.
Attachment #498049 - Flags: review-
(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.
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
Version: 3.6.3 → 2.10
Attached patch Alternative patch v.1 (obsolete) — Splinter Review
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
Whiteboard: [ws:moderate] → [infrasec:input][ws:moderate]
Attached patch patch, v2 (obsolete) — Splinter Review
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.
Assignee: reed → LpSolit
Attachment #498049 - Attachment is obsolete: true
Attachment #498136 - Attachment is obsolete: true
Attachment #498514 - Flags: review?(mkanat)
Blocks: 620540
(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.
CVE-2010-4567
Alias: CVE-2010-4567
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
(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.
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
Attachment #498514 - Flags: review?(mkanat) → review?(dkl)
Flags: blocking4.0?
Flags: blocking3.6.4?
Flags: blocking3.4.10?
Flags: blocking3.2.10?
Comment on attachment 498514 [details] [diff] [review]
patch, v2

Looks good and works as expected in my testing. r=dkl
Attachment #498514 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.0?
Flags: blocking4.0?
Flags: blocking4.0+
Flags: blocking3.6.4?
Flags: blocking3.6.4+
Flags: blocking3.4.10?
Flags: blocking3.4.10+
Flags: blocking3.2.10?
Flags: blocking3.2.10+
Here is the backport for 3.6.
Attachment #506172 - Flags: review?(dkl)
Comment on attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

This patch works for 3.2 - 3.6.
Attachment #506172 - Attachment description: patch for 3.6, v1 → patch for 3.2 - 3.6, v1
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.
Attachment #498514 - Attachment is obsolete: true
Attachment #506176 - Flags: review?(dkl)
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.
Attachment #506172 - Attachment description: patch for 3.2 - 3.6, v1 → patch for 3.2 - 4.0, v1
Flags: approval?
Flags: approval4.0?
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>
(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 on attachment 506176 [details] [diff] [review]
patch for 4.1, v3

Looks correct and works as expected. r=dkl
Attachment #506176 - Flags: review?(dkl) → review+
Flags: approval?
Comment on attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

Looks good and works on all test versions. r=dkl
Attachment #506172 - Flags: review?(dkl) → review+
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Security advisory sent. Removing the security flag.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.