The default bug view has changed. See this FAQ.
Bug 761655 (CVE-2012-1961)

Firefox ignores X-Frame-Options when set to SAMEORIGIN, SAMEORIGIN (duplicated header)

RESOLVED FIXED in Firefox 14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Assigned: geekboy)

Tracking

({sec-moderate})

13 Branch
mozilla16
sec-moderate
Points:
---

Firefox Tracking Flags

(firefox14+ fixed, firefox15+ fixed, firefox16+ fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [advisory-tracking+][qa-], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
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.
Blocks: 761046
Severity: enhancement → normal
Keywords: sec-moderate
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

5 years ago
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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #631201 - Flags: review?(jst)
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.
Attachment #631201 - Flags: review?(jst) → review+
(Assignee)

Comment 6

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

Comment 7

5 years ago
Created attachment 632300 [details] [diff] [review]
fix

Addressed review comments, worked fine on Try.  Carrying over r=jst.
Attachment #631201 - Attachment is obsolete: true
Attachment #632300 - Flags: review+
(Assignee)

Comment 8

5 years ago
Pushed to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b3098ddd9b
Target Milestone: --- → mozilla16
(Reporter)

Comment 9

5 years ago
Is there a chance to see this bug fixed into Fx 14 and 15, as it's security related?
(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.
(Reporter)

Comment 11

5 years ago
(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.
(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)?
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox16: --- → +
https://hg.mozilla.org/mozilla-central/rev/62b3098ddd9b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox16: affected → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → +
Group: core-security
Please request branch approvals if these are appropriate patches, or attach additional back-ported patches if necessary and request approvals on those. Thanks.
tracking-firefox-esr10: ? → 14+
tracking-firefox14: ? → +
(Assignee)

Comment 15

5 years ago
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.
Attachment #632300 - Flags: approval-mozilla-beta?
Attachment #632300 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

5 years ago
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+.
Attachment #639776 - Flags: review+
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.
Attachment #639776 - Flags: review+
Attachment #639776 - Flags: approval-mozilla-esr10?
Attachment #632300 - Flags: approval-mozilla-beta?
Attachment #632300 - Flags: approval-mozilla-beta+
Attachment #632300 - Flags: approval-mozilla-aurora?
Attachment #632300 - Flags: approval-mozilla-aurora+
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.
Attachment #639776 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 19

5 years ago
Thanks Lukas.  Landed on all three branches.
status-firefox-esr10: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/9a51cdd0e7a2
https://hg.mozilla.org/releases/mozilla-beta/rev/094f6bd2f953
https://hg.mozilla.org/releases/mozilla-aurora/rev/4f801d0a2d3f
Whiteboard: [advisory-tracking+]
Alias: CVE-2012-1961
Is there something QA can do to verify this fix?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
Group: core-security

Updated

5 years ago
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa-]
You need to log in before you can comment on or make changes to this bug.