Last Comment Bug 716283 - Clickjacking in the attachment "Details" page allows to bypass token checks
: Clickjacking in the attachment "Details" page allows to bypass token checks
Status: RESOLVED FIXED
[infrasec:bestpractice][ws:low]
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 4.3
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 14:30 PST by Mario Gomes
Modified: 2012-03-13 02:36 PDT (History)
8 users (show)
LpSolit: approval+
LpSolit: approval4.2+
LpSolit: blocking4.2+
LpSolit: approval4.0+
LpSolit: blocking4.0.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Exploit used in landfill. (940 bytes, text/plain)
2012-01-07 14:33 PST, Mario Gomes
no flags Details
patch, v1 (2.61 KB, patch)
2012-01-09 14:33 PST, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review
patch for 4.0, v1 (2.89 KB, patch)
2012-01-09 16:11 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review

Description Mario Gomes 2012-01-07 14:30:13 PST
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.75 Safari/535.7

Steps to reproduce:

Hello,

all hard test done in https://landfill.bugzilla.org/bugzilla-tip/show_bug.cgi?id=16947.

Reproduce:
1. Open this https://landfill.bugzilla.org/bugzilla-tip/attachment.cgi?id=2346&action=edit
2. Click in "Click Me!"
3. Back to https://landfill.bugzilla.org/bugzilla-tip/show_bug.cgi?id=16947 see the changes.

Tested using Chrome 16.
Windows 7 SP1

Cheers,
Mario.
Comment 1 Mario Gomes 2012-01-07 14:33:12 PST
Created attachment 586733 [details]
Exploit used in landfill.
Comment 2 Frédéric Buclin 2012-01-07 15:54:00 PST
Confirmed. Using attachment_base doesn't help.
Comment 3 Frédéric Buclin 2012-01-07 17:15:31 PST
Note that we don't exactly bypass the security token check as process_bug.cgi still asks you to confirm the changes. But the user doesn't realize that the "click me" button is to confirm the changes as the iframe is hidden.

This is not a critical security bug, because a) viewing the attachment directly cannot trigger this problem, due to the implementation of X-FRAME-OPTIONS: SAMEORIGIN in bug 475894 + the alternate attachment host in bug 38862; and b) it requires a user action from the Details page, and users should understand that there is a risk to interact with an HTML document. To mitigate this last problem, we could display a warning above our iframe when the MIME type is text/html, and if the alternate attachment host is available, we could also suggest to view the attachment directly, which would annihilate this clickjacking problem. I think that in most cases, this warning should be enough. If the user ignores this warning, there is nothing we can do for him.

This clickjacking problem has already been discussed in bug 26257 comment 128, bug 475894 comment 4, bug 475894 comment 7 and bug 600692 comment 9. So I'm removing the security flag as this discussion already took place there, and these bugs are public.
Comment 4 Frédéric Buclin 2012-01-07 17:37:05 PST
Hum, one easy way to fix this problem is to *always* display the source code of HTML files in the "Details" page, and ask the user to click "View" to view the HTML page itself. As I said, clickjacking is not possible from the alternate host as we use X-FRAME-OPTIONS: SAMEORIGIN. No need for the warning message I suggested in my previous comment, and no way to bypass this restriction (for users who never read warnings). I think that developers who play with HTML testcases don't do it from the Details page anyway, so they wouldn't be hurt if we display the source code instead of the HTML page itself.

Marking as a blocker for 4.2 for now, because if everybody agrees with my proposal (source code), it would be very easy to do and would really worth it.
Comment 5 Reed Loden [:reed] (use needinfo?) 2012-01-07 18:30:18 PST
+1 from me. I think it's a good idea.
Comment 6 Mario Gomes 2012-01-08 04:32:27 PST
I do not agree to let this vulnerability (or bug) in public. is an easy to be exploited, and is easy to be repaired. For example, in Firefox, I can report a vulnerability, and convince someone of safety and security team of Firefox click on "Click Me!" and remove the security restrictions in a real security bug.

Exemple of the report:
===========================
Hello,

Firefox crashes exploitable when manipulating IFRAMES. The failure appears to be exploited, but to be performed is necessary to be viewed by the "Details" in Bugzilla.

Reproduce:
1. Open with firefox https://landfill.bugzilla.org/bugzilla-tip/attachment.cgi?id=2346&action=edit
2. Click in "Click Me!"
3. See the CRASH

(To help convince, I can post a Stacktrace false Here.)

Regards,
Mario
========================

Using this, I have great chances of the exploit work and changes are made in a security bug.



(In reply to Frédéric Buclin from comment #3)
> Note that we don't exactly bypass the security token check as
> process_bug.cgi still asks you to confirm the changes. But the user doesn't
> realize that the "click me" button is to confirm the changes as the iframe
> is hidden.
> 
> This is not a critical security bug, because a) viewing the attachment
> directly cannot trigger this problem, due to the implementation of
> X-FRAME-OPTIONS: SAMEORIGIN in bug 475894 + the alternate attachment host in
> bug 38862; and b) it requires a user action from the Details page, and users
> should understand that there is a risk to interact with an HTML document. To
> mitigate this last problem, we could display a warning above our iframe when
> the MIME type is text/html, and if the alternate attachment host is
> available, we could also suggest to view the attachment directly, which
> would annihilate this clickjacking problem. I think that in most cases, this
> warning should be enough. If the user ignores this warning, there is nothing
> we can do for him.
> 
> This clickjacking problem has already been discussed in bug 26257 comment
> 128, bug 475894 comment 4, bug 475894 comment 7 and bug 600692 comment 9. So
> I'm removing the security flag as this discussion already took place there,
> and these bugs are public.
Comment 7 Frédéric Buclin 2012-01-08 04:48:49 PST
(In reply to Mario Gomes from comment #6)
> I do not agree to let this vulnerability (or bug) in public.

This discussion already took place for a long time. There is no reason to keep this bug private.


> report a vulnerability, and convince someone of safety and security team of
> Firefox click on "Click Me!" and remove the security restrictions in a real
> security bug.

I doubt members of the security team are so easily abused by such statements. They are the ones who mentioned clickjacking problems in bugs I mentioned in comment 3.
Comment 8 Mario Gomes 2012-01-08 05:07:15 PST
So I think you can close this as "duplicate". Doubt, why not do a test?
Comment 9 Byron Jones ‹:glob› [PTO until 2016-10-10] 2012-01-09 12:04:52 PST
(In reply to Frédéric Buclin from comment #4)
> Hum, one easy way to fix this problem is to *always* display the source code
> of HTML files in the "Details" page, and ask the user to click "View" to
> view the HTML page itself.

that sounds sane.
Comment 10 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-01-09 12:05:49 PST
+1 on showing the source on the details page.  I've been pretty annoyed in the past at having to load HTML that did weird things when just trying to edit details about the attachment.
Comment 11 Frédéric Buclin 2012-01-09 12:13:17 PST
Do we want it for 3.6.8 too? Not sure the 8th minor release is the best moment to implement it. It may be confusing to some users. But if we decide to take it for 3.6.8, we should also take the X-FRAME-OPTIONS patch from bug 475894 for a complete fix.
Comment 12 David Lawrence [:dkl] 2012-01-09 12:32:02 PST
This solution works for as well.

dkl
Comment 13 Frédéric Buclin 2012-01-09 14:33:38 PST
Created attachment 587154 [details] [diff] [review]
patch, v1
Comment 14 David Lawrence [:dkl] 2012-01-09 15:05:36 PST
Comment on attachment 587154 [details] [diff] [review]
patch, v1

Review of attachment 587154 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine and works as expected. r=dkl
Comment 15 Frédéric Buclin 2012-01-09 15:08:10 PST
We should relnote it for 4.2 too, when releasing 4.2 final (or 4.2rc2, if there is one).
Comment 16 Frédéric Buclin 2012-01-09 16:05:04 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/global/textarea.html.tmpl
Committed revision 8067.

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

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/global/textarea.html.tmpl
Committed revision 7680.
Comment 17 Frédéric Buclin 2012-01-09 16:11:53 PST
Created attachment 587190 [details] [diff] [review]
patch for 4.0, v1

For the record, that's the patch for 4.0.4 with bitrot fixed. This patch doesn't apply to 3.6.7, so do not try to use it there.
Comment 18 Frédéric Buclin 2012-02-01 05:48:46 PST
Added to relnotes for 4.2rc2 and 4.0.4.
Comment 19 Mario Gomes 2012-02-19 05:33:28 PST
Remains vulnerable when you open SVG and XHTML.
Comment 20 Frédéric Buclin 2012-02-19 05:39:06 PST
(In reply to Mario Gomes from comment #19)
> Remains vulnerable when you open SVG and XHTML.

Do you have a PoC for SVG, please? :)
Comment 21 Mario Gomes 2012-02-19 08:33:25 PST
Yep, the PoC can be seen here https://landfill.bugzilla.org/bugzilla-tip/attachment.cgi?id=2466&action=edit.
Comment 22 Reed Loden [:reed] (use needinfo?) 2012-02-19 08:55:51 PST
Instead of a blacklist, I think it makes sense to go with a whitelist (text/plain, image/*, application/pdf [ugh]).
Comment 23 Jesse Ruderman 2012-02-19 09:58:36 PST
(In reply to Reed Loden [:reed] (very busy) from comment #22)
> Instead of a blacklist, I think it makes sense to go with a whitelist
> (text/plain, image/*, application/pdf [ugh]).

But not image/svg+xml? ;)
Comment 24 Frédéric Buclin 2012-02-19 10:49:58 PST
(In reply to Jesse Ruderman from comment #23)
> But not image/svg+xml? ;)

Heh, good point! :)

Please file a separate bug about the possible use of a whitelist, with Mario's URL from comment 21. But that's not something we will backport to branches. Clickjacking is not as bad as CSRF.

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