1200869 - CORS preflight cache poisoning with a CORS header being mistaken with another CORS header

Status: RESOLVED FIXED

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

Description

[(no longer active)]

There is a bug in nsCORSPreflightListener::AddResultToCache() in that we call GetResponseHeader() without emptying out the out value passed to it, which causes us to mistake the value of a header that we have checked before with the next header if the response doesn't include the next header.  This hits us in three cases:

* If the preflight response includes Access-Control-Max-Age, but not Access-Control-Allow-Methods, when we check for Access-Control-Allow-Methods, we mistakenly use the value of the Access-Control-Max-Age header.  Even though Access-Control-Max-Age can only be a string parseable as a number, surprisingly nothing actually fails an attempt to XHR with a method equal to such a string!

* If the preflight response includes Access-Control-Allow-Methods but not Access-Control-Allow-Headers, we use the value of the Access-Control-Allow-Methods instead of the Access-Control-Allow-Headers.

* If the preflight response includes Access-Control-Max-Age but not Access-Control-Allow-Headers nor Access-Control-Allow-Methods, when we check for Access-Control-Allow-Headers, we mistakenly use the value of the Access-Control-Max-Age header.

I think the third bug is not possible to trigger from script since there is no way to force a preflight without using a non-simple method or header, and the CORS preflight will fail in that case without either an Access-Control-Allow-Headers or an Access-Control-Allow-Methods header.

Going back in the history, I think we have had this bug since forever.  It definitely predates bug 644476, similar to bug 1200856.  I didn't look back further.

Comment 1

[(no longer active)]

Attachment: Empty the header value for code hygiene

Comment 2

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ecd1ff453a

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

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

Arguably that's a pretty bad API on the nsIHttpChannel. Ideally I'd like to see this fixed there since I'd imagine this bites more callsites. But up to you and the necko peers.

r=me on this patch either way.

Comment 4

[(no longer active)]

I agree, this is a terrible API.  Filed bug 1201229 for fixing that.

Comment 5

[(no longer active)]

Comment on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  This requires knowledge of the semantics of GetResponseHeader, so I don't think a reader can immediately figure out what this patch does (I'm assuming that we don't check in the test at the same time.)

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  I think the commit message is sneaky enough, and I don't think we should land the tests before this fix gets shipped to the release channel.

Which older supported branches are affected by this flaw? All branches.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  The backports are trivial and very low risk.

How likely is this patch to cause regressions; how much testing does it need? I've tested it locally and on try and the problem is also quite well understood.  I'm not too much worried about regressions from this.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We should take this on Aurora, Beta, and ESR38 as well. ESR31 is at EOL so we don''t need it there.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

Also, this should land without tests until it is released publicly.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Liz, here is another esr38 nom.

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

Approved for ESR38

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/279325711b9f

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f836871f565

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/a76d4a1c7fcc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d3cb4f28c735
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/d3cb4f28c735
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/b2a5888625db

Comment 15

[(no longer active)]

Now that the fixes have been shipped, can I check in the tests (and those of bug 1200856)?  Given that I have seen a number of vulnerabilities in CORS preflights lately, I think this area of the code really benefits from having more test coverage.

Comment 16

[(no longer active)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #15)
> Now that the fixes have been shipped, can I check in the tests (and those of
> bug 1200856)?  Given that I have seen a number of vulnerabilities in CORS
> preflights lately, I think this area of the code really benefits from having
> more test coverage.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Ah yes. Please check in the tests!

Comment 18

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c80a34a1597e