761655 - (CVE-2012-1961) Firefox ignores X-Frame-Options when set to SAMEORIGIN, SAMEORIGIN (duplicated header)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Frédéric Buclin]

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

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

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

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

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

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

(In reply to Johnny Stenback (:jst, [e-mail]) 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]]

Attachment: fix

Addressed review comments, worked fine on Try.  Carrying over r=jst.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Pushed to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b3098ddd9b

Comment 9

[Frédéric Buclin]

Is there a chance to see this bug fixed into Fx 14 and 15, as it's security related?

Comment 10

[Reed Loden [:reed]]

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

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

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

https://hg.mozilla.org/mozilla-central/rev/62b3098ddd9b

Comment 14

[Daniel Veditz [:dveditz]]

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

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

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

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]

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

Thanks Lukas.  Landed on all three branches.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 21

[u279076]

Is there something QA can do to verify this fix?