633691 - (CVE-2012-1964) Mitigate clickjacking of about:certerror

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Matt McCutchen]

A site colluding with a MITM attacker can show a fake about:certerror page with the "Add Exception" button of a real about:certerror page for another site showing through an iframe.  If the user judges the front site unimportant and fails to recheck the hostname in the "Add Security Exception" dialog, his/her communications with the back site are compromised.

I have linked a demo for Firefox 3.6 and 4 that targets www.cacert.org (so it will not work if you have the CAcert root enabled or already have a cert override for this site).  The demo assumes browser.xul.error_pages.expert_bad_cert is enabled.  In a real attack, I think one could avoid this assumption by supplying a cert with a related hostname so that the "Technical Details" section of the back page expands and then letting the entire "I Understand the Risks" section (including the toggle) show through.  The only problem is that the width of the errorPageContainer may change when this section opens, and I'm not sure how to make it change on both the front and back pages.

Please consider mitigating this attack.  One possibility: when about:certerror loads in a frame, it could say in place of the button, "To add an exception, you need to <a>open this page in a new tab</a>."

I'm marking the bug private as a precaution.  Please feel free to open it if you judge that it does not need this protection.

Comment 1

[Justin Dolske [:Dolske]]

How about we just block all invalid-cert loads in iframes, and only show the about:certerror warning for top-level documents? Frames are not terribly common, so it seems like this would be enough. Alternatively, we could sho some kind of error pattern (ala blocked plugins), and when clicked make that zoom to full-tab as if you were viewing just the iframe (and, then show the normal cert error UI).

Comment 2

[Justin Dolske [:Dolske]]

Ok, the more I think about it the less this bug makes sense...

1) If you've deliberately flipped the "expert_bad_cert" pref and are ignoring the actual text of the dialog... Sorry. You are not an expert. ;-) AKA "Doctor, it hurts when I do this!"

2) The default-pref case you describe doesn't seem useful. The attacker is being limited to a similar-sounding domain, you have to trick the user into clicking a couple different things, and the resulting dialog still has warnings and the real host the exception is for. And there's a MITM going on anyway, so I'm not sure why they'd resort to such subtle and limited measures.

So I'm WONTFIXing this for the specific attack.

But I _do_ think there's significant value in having the various in-content UI we have display different when in an iframe. EG when a image fails to load we just show a broken-image icon, so an iframe with a problem could just do something similar... Giving the user instruction on fixing the problem (irrelevantly, in the URL bar) isn't useful for a web page!

Filing that momentarily, and it would also address the problem in this bug.

Comment 3

[Justin Dolske [:Dolske]]

Filed bug 688658.

Comment 4

[Matt McCutchen]

(In reply to Justin Dolske [:Dolske] from comment #2)
> 1) If you've deliberately flipped the "expert_bad_cert" pref and are
> ignoring the actual text of the dialog... Sorry. You are not an expert. ;-)
> AKA "Doctor, it hurts when I do this!"

I disagree.  "Expert" in that context just means the user understands the implications of cert overrides and wants a faster path to add them when they are appropriate.  Even the average cert override "expert" is likely to assume that when they click "Add exception" and the dialog appears, it is for the same hostname they tried to visit; he/she is unlikely to anticipate this kind of clickjacking.

> 2) The default-pref case you describe doesn't seem useful. The attacker is
> being limited to a similar-sounding domain,

No, the cert only has to contain a domain similar to the one in the back frame, it does not have to be valid for that domain.  So this is not a limitation.

> you have to trick the user into
> clicking a couple different things,

Just "I Understand the Risks" and "Add Exception", as the user normally would.

> and the resulting dialog still has
> warnings and the real host the exception is for.

The dangerous case is when the front site is one the user really doesn't care about authenticating, and he/she is ready to click right through what he/she believes is the exception process for that site.  *Wag finger*... but such is the state of the web today.

> And there's a MITM going on
> anyway, so I'm not sure why they'd resort to such subtle and limited
> measures.

Because MITM-ing https://www.google.com (as an example of a non-HSTS high-value site) straight out is unlikely to fool anyone?

> But I _do_ think there's significant value in having the various in-content
> UI we have display different when in an iframe. [...]
> 
> Filing that momentarily, and it would also address the problem in this bug.

Provided that the iframe about:certerror does not contain the "Add Exception" button, yes.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Just to double-check my understanding of the fix we want here: If the certerror appears in an iframe then we should remove the "Add Exception" button?

Comment 6

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jwein and :jaws] from comment #5)
> Just to double-check my understanding of the fix we want here: If the
> certerror appears in an iframe then we should remove the "Add Exception"
> button?

(Quote from Matt McCutchen in comment #0)
> One possibility: when
> about:certerror loads in a frame, it could say in place of the button, "To
> add an exception, you need to <a>open this page in a new tab</a>."

I think we want to give the user the ability to open the frame in a new tab as suggested in comment 0 so they are not left at a dead-end.  ISTR that we do this for X-Frame-Options or some other warning dialog when shown in frames but I can't find any reference to it so maybe it was another browser. Then again, maybe that link would be a footgun since they may not check the domain of that new tab and assume it's the same as the parent page.

Comment 7

[Matt McCutchen]

(In reply to Matthew N. [:MattN] from comment #6)
> Then again, maybe that link would be a footgun since they may not check the
> domain of that new tab and assume it's the same as the parent page.

That would be a silly assumption to make.  Given that the top-level page loaded successfully, it's highly unlikely that a frame from the same site would hit a cert error: this could only happen if the frame makes a new SSL connection and the server cert has just changed or become invalid.  I have very little sympathy for a user who makes that assumption and ignores the domain in the URL bar and text of a full-tab cert error.  Compare to the original attack, where even I might have uncritically assumed the exception dialog would be launched with the same domain I saw in the URL bar and the fake cert error page.

This is not to say that I would object to leaving a dead end in the frame case, or for that matter removing "Add Exception" from all cert errors in keeping with Nelson Bolyard's preference in bug 327181 comment 24.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 633691

This patch hides the ability to add the override if the certificate error occurs in a frame.

Users can still right click on the page and use the context menu "This Frame -> Show Only This Frame" if they really need to override the certificate error. This also gives users a better chance to see that the domains are different.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

This can be tested by using the following URLs:

With override ability removed:
data:text/html,<iframe width="700" height="700" src="about:certerror"></iframe>

Without override ability:
about:certerror

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Here is a non-demo page as an example:

data:text/html,<iframe width="700" height="700" src="https://access.techsmith.com"></iframe>

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 578412 [details] [diff] [review]
Patch for bug 633691

window != top is the canonical way to check this, I think, but it doesn't really matter (window.self is a getter on nsGlobalWindow and so is trivially more expensive to retrieve).

Another nit I noticed in passing that you should also feel free to ignore since it's not directly related to this bug: the existing code could use ec.parentNode.removeChild instead of another getElementById() call.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/290d329672e5

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/290d329672e5

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Looks like this should have gone out with 13, but we'll have to settle for getting it in the next ESR (alongside FF14).  Please nominate for ESR landing approval.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Lukas Blakk [:lsblakk] from comment #15)
> Please nominate for ESR landing approval.

I'm sorry, but I'm not sure how I should be reading this. Are you asking that I:

A) request ESR landing for this bug now? or
B) request ESR landing approval for other security-related bugs in the future?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

A)

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 578803 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Click-jacking attack vector on the about:certerror page.
Fix Landed on Version: Firefox 11
Risk to taking this patch (and alternatives if risky): no risk anticipated
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 578803 [details] [diff] [review]
Patch for checkin

Approved. Looks like this got discovered a bit late and should have gone out with a previous ESR. Better late than never.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to esr-10:
https://hg.mozilla.org/releases/mozilla-esr10/rev/b7665eb6401c

Comment 21

[u279076]

Does this have a testcase?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #21)
> Does this have a testcase?

It doesn't have a testcase in the tree, but you can verify it by visiting this URL:
> data:text/html,<iframe width="700" height="700" src="https://msu.edu"></iframe>

Comment 23

[u279076]

Should this be given a testcase in the tree? Sorry, I'm a bit new as to what does and does not qualify for an in-testsuite testcase.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, this should be in the testsuite. I'm happy to help mentor someone who wants to write the test for this. I'm not sure though if the test should be worked on in a new bug though.

Comment 25

[u279076]

(In reply to Jared Wein [:jaws] from comment #24)
> Yeah, this should be in the testsuite. I'm happy to help mentor someone who
> wants to write the test for this. I'm not sure though if the test should be
> worked on in a new bug though.

What makes the most sense to me would be to have someone in QA assign themselves as the QA contact and to develop the test in this bug. That seems appropriate to me. Any objections?

Comment 26

[Ioana (away)]

Verified as fixed on the 08/22 Firefox 10.0.7 ESR build (20120822034501) on Ubuntu 12.04, Mac 10.7.4 and Windows XP. The whole "I Understand the Risks" section has been removed from iframe about:certerror.

Comment 27

[Ioana (away)]

(In reply to Jared Wein [:jaws] from comment #24)
> Yeah, this should be in the testsuite. I'm happy to help mentor someone who
> wants to write the test for this. I'm not sure though if the test should be
> worked on in a new bug though.

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #25)
> ... 
> What makes the most sense to me would be to have someone in QA assign
> themselves as the QA contact and to develop the test in this bug. That seems
> appropriate to me. Any objections?

If there are no objections, I would like to develop the test for this bug. I've already assigned myself as QA contact and will start working on it as soon as possible.

Comment 28

[Ioana (away)]

Attachment: Automated test for bug 633691

I ran the test locally several times and it never failed.

Not sure who I should request for review. Jared, perhaps you can help me with that.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 727287 [details] [diff] [review]
Automated test for bug 633691

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

Looks good!

::: browser/components/certerror/test/browser_bug633691.js
@@ +2,5 @@
> +  waitForExplicitFinish();
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  // Open a html page with about:certerror in an iframe
> +  window.content.addEventListener("load", testIframeCert, true);
> +  content.location = "data:text/html,<iframe width=\"700\" height=\"700\" src=\"about:certerror\"></iframe>";

The attributes inside of the string can use single quotes so you don't need to escape them. For example:
> content.location = "data:text/html,<iframe width='700' height='700' src='about:certerror'></iframe>";

@@ +14,5 @@
> +    var eC = doc.getElementById("expertContent");
> +    ok(eC, "Expert content should exist")
> +    ok(eC.hasAttribute("hidden"), "Expert content should be hidded by default");
> +  } catch (e) {
> +      ok(true, e);

You can remove the try/catch block here, but keep what you have inside of the try block.

Comment 30

[Ioana (away)]

Attachment: Automated test for bug 633691 - v2

Modified as per the previous comment.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 727565 [details] [diff] [review]
Automated test for bug 633691 - v2

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

::: browser/components/certerror/test/Makefile.in
@@ +12,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  include $(topsrcdir)/config/rules.mk
>  
>  _BROWSER_FILES = browser_bug431826.js \
> +		  browser_bug633691.js \

nit, replace tabs with spaces here.

::: browser/components/certerror/test/browser_bug633691.js
@@ +1,1 @@
> +function test() {

Please add the following license block to the top of the file,

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/
 */

Comment 32

[Ioana (away)]

Attachment: Automated test for bug 633691

Added the changes requested in comment 31.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to inbound, https://hg.mozilla.org/integration/mozilla-inbound/rev/559990932762

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/559990932762