Closed Bug 1200869 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- wontfix
firefox41 + fixed
firefox42 + fixed
firefox43 + fixed
firefox-esr31 --- wontfix
firefox-esr38 41+ 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

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 file)

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.
Attachment #8655726 - Flags: review?(jonas)
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+
I agree, this is a terrible API.  Filed bug 1201229 for fixing that.
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+
Flags: in-testsuite?
Liz, here is another esr38 nom.
Flags: needinfo?(lhenry)
https://hg.mozilla.org/mozilla-central/rev/df60b3019087
Status: NEW → RESOLVED
Closed: 5 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+]
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.
(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)
Flags: needinfo?(ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80a34a1597e
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.