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

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
7 years ago
4 years ago

People

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

Tracking

(Blocks: 1 bug)

2.10
Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
blocking4.0 +
approval3.6 +
blocking3.6.4 +
approval3.4 +
blocking3.4.10 +
approval3.2 +
blocking3.2.10 +

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
(Reporter)

Updated

7 years ago
(Reporter)

Updated

7 years ago
(Reporter)

Updated

7 years ago
(Reporter)

Updated

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

Comment 2

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

7 years ago
Note that no theft of user credentials is possible here. The user credentials are httponly.

Comment 4

7 years ago
And yeah, that should be an easy fix.
(Reporter)

Comment 5

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

Comment 6

7 years ago
Changed the title accordingly. Presumably an XMLHttprequest in the javascript could post the credentials to evil.com...

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

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

Updated

7 years ago
OS: Linux → All
Hardware: x86 → All
(Reporter)

Comment 11

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

Updated

7 years ago
Whiteboard: [sg:high] → [ws:moderate]
(Reporter)

Updated

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

Updated

7 years ago
Whiteboard: [sg:high] → [ws:moderate]
Target Milestone: --- → Bugzilla 3.4
Version: unspecified → 3.6.3
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...
Assignee: create-and-change → reed
Status: NEW → ASSIGNED
Attachment #498049 - Flags: review?(mkanat)

Comment 14

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

Comment 17

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

Comment 18

7 years ago
(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
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
Whiteboard: [ws:moderate] → [infrasec:input][ws:moderate]
(Assignee)

Comment 20

7 years ago
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.
Assignee: reed → LpSolit
Attachment #498049 - Attachment is obsolete: true
Attachment #498136 - Attachment is obsolete: true
Attachment #498514 - Flags: review?(mkanat)
(Assignee)

Updated

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

Comment 24

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

Updated

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

Updated

7 years ago
Flags: approval?
Flags: approval4.0?
Blocks: 619566
Blocks: 628034
(Assignee)

Updated

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

Comment 27

6 years ago
Created attachment 506172 [details] [diff] [review]
patch for 3.2 - 4.0, v1

Here is the backport for 3.6.
Attachment #506172 - Flags: review?(dkl)
(Assignee)

Comment 28

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

Comment 29

6 years ago
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.
Attachment #498514 - Attachment is obsolete: true
Attachment #506176 - Flags: review?(dkl)
(Assignee)

Comment 30

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

Updated

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

Comment 32

6 years ago
(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+

Updated

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

Updated

6 years ago
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
(Assignee)

Updated

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

Comment 35

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 36

6 years ago
Security advisory sent. Removing the security flag.
Group: bugzilla-security
(Assignee)

Updated

6 years ago
Duplicate of this bug: 709573

Updated

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