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

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Alex Miller]

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]]

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]

(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]

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

Comment 4

[Max Kanat-Alexander]

And yeah, that should be an easy fix.

Comment 5

[Alex Miller]

(In reply to comment #3)
> Note that no theft of user credentials is possible here. The user credentials
> are httponly.

Comment 6

[Alex Miller]

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

Comment 7

[Max Kanat-Alexander]

Actually, even the session ID is httponly.

Comment 8

[Max Kanat-Alexander]

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]

(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]

(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]

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]]

Attachment: 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]

Comment on attachment 498049 [details] [diff] [review]
patch - v1

Looks good to me.

Comment 15

[Reed Loden [:reed]]

Mr. [e-mail]... 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

[[e-mail]]

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:
javascriFt:

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]

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]

(In reply to comment #16)
> javascriFt:

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]]

Attachment: 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]

Attachment: 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]]

(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]]

CVE-2010-4567

Comment 23

[Gervase Markham [:gerv]]

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]

(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]]

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]]

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]

Attachment: patch for 3.2 - 4.0, v1

Here is the backport for 3.6.

Comment 28

[Frédéric Buclin]

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]

Attachment: 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]

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]]

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]

(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]]

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]]

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]

Committing to: bzr+ssh://[e-mail]/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://[e-mail]/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://[e-mail]/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://[e-mail]/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://[e-mail]/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]

Security advisory sent. Removing the security flag.