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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({csectype-sop, sec-high})

unspecified
mozilla43
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ fixed, firefox42+ fixed, firefox43+ fixed, firefox-esr31 wontfix, firefox-esr3841+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

4 years ago
Attachment #8655726 - Flags: review?(jonas)
(Assignee)

Updated

4 years ago

Updated

4 years ago
Group: core-security → dom-core-security
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.
Attachment #8655726 - Flags: review?(jonas) → review+
(Assignee)

Comment 4

4 years ago
I agree, this is a terrible API.  Filed bug 1201229 for fixing that.
(Assignee)

Comment 5

4 years ago
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.
Attachment #8655726 - Flags: sec-approval?
Attachment #8655726 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8655726 - Flags: approval-mozilla-release?
Attachment #8655726 - Flags: approval-mozilla-esr38?
Attachment #8655726 - Flags: approval-mozilla-esr31?
Attachment #8655726 - Flags: approval-mozilla-beta?
Attachment #8655726 - Flags: approval-mozilla-b2g37?
Attachment #8655726 - Flags: approval-mozilla-aurora?
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 on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

Also, this should land without tests until it is released publicly.
Attachment #8655726 - Flags: sec-approval?
Attachment #8655726 - Flags: sec-approval+
Attachment #8655726 - Flags: approval-mozilla-release?
Attachment #8655726 - Flags: approval-mozilla-esr31?
Attachment #8655726 - Flags: approval-mozilla-beta?
Attachment #8655726 - Flags: approval-mozilla-beta+
Attachment #8655726 - Flags: approval-mozilla-aurora?
Attachment #8655726 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Flags: in-testsuite?
Liz, here is another esr38 nom.
Flags: needinfo?(lhenry)
https://hg.mozilla.org/mozilla-central/rev/df60b3019087
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8655726 [details] [diff] [review]
Empty the header value for code hygiene

Approved for ESR38
Flags: needinfo?(lhenry)
Attachment #8655726 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [post-critsmash-triage]
Group: dom-core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
(Assignee)

Comment 15

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

Comment 16

4 years ago
(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.
Flags: needinfo?(abillings)
Ah yes. Please check in the tests!
Flags: needinfo?(abillings) → needinfo?(ehsan)
(Assignee)

Updated

4 years ago
Flags: needinfo?(ehsan)
(Assignee)

Comment 18

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80a34a1597e
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.