Last Comment Bug 761655 - (CVE-2012-1961) Firefox ignores X-Frame-Options when set to SAMEORIGIN, SAMEORIGIN (duplicated header)
(CVE-2012-1961)
: Firefox ignores X-Frame-Options when set to SAMEORIGIN, SAMEORIGIN (duplicate...
Status: RESOLVED FIXED
[advisory-tracking+][qa-]
: sec-moderate
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Sid Stamm [:geekboy or :sstamm]
:
: Andrew Overholt [:overholt]
Mentors:
https://mxr.mozilla.org/mozilla-centr...
Depends on:
Blocks: 761046
  Show dependency treegraph
 
Reported: 2012-06-05 09:26 PDT by Frédéric Buclin
Modified: 2012-08-14 08:16 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
14+
fixed


Attachments
fix (12.62 KB, patch)
2012-06-07 16:16 PDT, Sid Stamm [:geekboy or :sstamm]
jst: review+
Details | Diff | Splinter Review
fix (13.96 KB, patch)
2012-06-12 09:52 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
fix for esr10 (13.83 KB, patch)
2012-07-06 13:46 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
dveditz: review+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Frédéric Buclin 2012-06-05 09:26:40 PDT
Firefox ignores the "X-Frame-Options: SAMEORIGIN, SAMEORIGIN" header when the SAMEORIGIN value is duplicated. Some websites incorrectly have this duplicated header for some unknown reasons, see e.g. bug 761046.

It would probably be fine to change the way nsDSURIContentListener::CheckFrameOptions parses this header to accept duplicated headers.
Comment 1 Daniel Veditz [:dveditz] 2012-06-05 17:12:31 PDT
Not an enhancement, a bug pure and simple. We need to enhance x-f-o support anyway, see bug 690168; maybe we can do both at the same time.
Comment 2 Sid Stamm [:geekboy or :sstamm] 2012-06-06 08:45:23 PDT
This is probably due to combining of multiple instances of the header (that's what the comma says to me).  AFAIK, the "spec" (working draft: http://tools.ietf.org/html/draft-gondrom-frame-options) doesn't say what to do about this.

There are two ways to fix this:
1. Split the header value and use the most restrictive policy
2. If there's a comma (multiple values), fall back to the policy "DENY"

I don't think we should ignore the header when there are multiple instances, since that would cater to header-injection-based bypasses.
Comment 3 Sid Stamm [:geekboy or :sstamm] 2012-06-06 09:35:41 PDT
I guess there's a third option too:
3. If there's a comma (multiple values), choose what's stated if there's only one and it is duplicated, choose "DENY" if there are different values.
Comment 4 Sid Stamm [:geekboy or :sstamm] 2012-06-07 16:16:03 PDT
Created attachment 631201 [details] [diff] [review]
fix

Here's a patch that does option 1: splits on commas, parses all the policies, and if the framing situation violates any of them, blocks the load.  Policies with unrecognized directives (i.e, not "deny" or "sameorigin") are ignored.  Patch includes two new tests for the bug.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2012-06-09 00:56:35 PDT
Comment on attachment 631201 [details] [diff] [review]
fix

- In content/base/test/file_x-frame-options_page.sjs

+  else if (query['xfo'] == "sameorigin2") {
+    response.setHeader("X-Frame-Options", "SAMEORIGIN, SAMEORIGIN", false);
+  }

Should we add tests here that test various white space combination around the comma as well, and maybe more than two values too while we're at it?

- In nsDSURIContentListener::CheckOneFrameOptionsPolicy():

+    printf("policy: %s\n", NS_ConvertUTF16toUTF8(policy).get());

Remove the printf.

+        if (policy.LowerCaseEqualsLiteral("deny")) {
+            NS_ASSERTION(policy.LowerCaseEqualsLiteral("deny"),
+                         "How did we get here with some random header value?");

That assertion will never fire, same condition as the if check we're in.

r=jst with that.
Comment 6 Sid Stamm [:geekboy or :sstamm] 2012-06-11 12:46:56 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #5)
> Should we add tests here that test various white space combination around
> the comma as well, and maybe more than two values too while we're at it?

Done.  Added a few more tests for two commas and for multiple values.

> +    printf("policy: %s\n", NS_ConvertUTF16toUTF8(policy).get());
> 
> Remove the printf.

Ack, yes, done.

> That assertion will never fire, same condition as the if check we're in.

Good catch -- artifact of code reorganization.  Deleted it. I pushed an updated patch to try, will attach it here after making sure it succeeds.
Comment 7 Sid Stamm [:geekboy or :sstamm] 2012-06-12 09:52:54 PDT
Created attachment 632300 [details] [diff] [review]
fix

Addressed review comments, worked fine on Try.  Carrying over r=jst.
Comment 8 Sid Stamm [:geekboy or :sstamm] 2012-06-12 09:58:00 PDT
Pushed to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b3098ddd9b
Comment 9 Frédéric Buclin 2012-06-12 09:59:36 PDT
Is there a chance to see this bug fixed into Fx 14 and 15, as it's security related?
Comment 10 Reed Loden [:reed] (use needinfo?) 2012-06-12 10:22:37 PDT
(In reply to Frédéric Buclin from comment #9)
> Is there a chance to see this bug fixed into Fx 14 and 15, as it's security
> related?

I'd be way more interested in finding and fixing whatever is causing Bugzilla to spit out multiple SAMEORIGIN headers.
Comment 11 Frédéric Buclin 2012-06-12 10:25:13 PDT
(In reply to Reed Loden [:reed] from comment #10)
> I'd be way more interested in finding and fixing whatever is causing
> Bugzilla to spit out multiple SAMEORIGIN headers.

Bugzilla is not the single website in the world. That's unrelated.
Comment 12 Reed Loden [:reed] (use needinfo?) 2012-06-12 10:39:05 PDT
(In reply to Frédéric Buclin from comment #11)

In what real world cases are we actually seeing this occur, aside from the issue found in bug 761046 (which has to do with duplication in another header as well)?
Comment 13 Matt Brubeck (:mbrubeck) 2012-06-12 18:33:18 PDT
https://hg.mozilla.org/mozilla-central/rev/62b3098ddd9b
Comment 14 Daniel Veditz [:dveditz] 2012-06-28 16:33:02 PDT
Please request branch approvals if these are appropriate patches, or attach additional back-ported patches if necessary and request approvals on those. Thanks.
Comment 15 Sid Stamm [:geekboy or :sstamm] 2012-07-06 13:42:16 PDT
Comment on attachment 632300 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): problem in initial implementation of feature.
User impact if declined: Users will not be protected from clickjacking on some sites.
Testing completed (on m-c, etc.): m-c, baked there for a while. 
Risk to taking this patch (and alternatives if risky): Not very risky. 
String or UUID changes made by this patch: None

This patch (in m-c) applies cleanly to aurora and beta, but not esr10.  New patch is coming for that.
Comment 16 Sid Stamm [:geekboy or :sstamm] 2012-07-06 13:46:59 PDT
Created attachment 639776 [details] [diff] [review]
fix for esr10

The patch that landed on m-c did not apply cleanly to esr10, so here's a patch that does.  Applies, compiles and x-frame-options tests succeed on esr10.  There are only trivial differences so I'm carrying over jst's r+.
Comment 17 Daniel Veditz [:dveditz] 2012-07-06 13:55:58 PDT
Comment on attachment 639776 [details] [diff] [review]
fix for esr10

r=dveditz on the backport

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Firefox will be sole modern browser to fail to protect users on some sites
User impact if declined: clickjacking attacks possible
Fix Landed on Version: Firefox 14 (soon)
Risk to taking this patch (and alternatives if risky): relatively minor
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-06 14:02:20 PDT
Comment on attachment 639776 [details] [diff] [review]
fix for esr10

low risk patch, go ahead and land to all three branches, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details on ESR landings.
Comment 19 Sid Stamm [:geekboy or :sstamm] 2012-07-06 14:20:14 PDT
Thanks Lukas.  Landed on all three branches.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-12 15:12:41 PDT
Is there something QA can do to verify this fix?

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