Closed
Bug 1211000
Opened 9 years ago
Closed 9 years ago
Move CORS preflight logic from nsCORSListenerProxy to nsCORSPreflightListener
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: sicking, Assigned: sicking)
References
Details
Attachments
(1 file, 2 obsolete files)
23.30 KB,
patch
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Right now the nsCORSListenerProxy class handles much of the logic for preflights. This has worked fine so far. However it is turning into a problem as we are moving to AsyncOpen2. The problem is that when we call AsyncOpen2 on the preflight channel, the preflight channel will create a "normal" nsCORSListenerProxy. I.e. one with mIsPreflight set to false and thus doesn't run the preflight-logic. There are a few ways that we could fix this: 1. We could simply add another nsCORSListenerProxy which *does* run the preflight logic. But that would mean having *two* CORS listeners which is wasteful, and possibly causes other problems. 2. We could reach into the httpchannel and provide enough logic that when it creates a CORS listener, then it runs the appropriate nsCORSListenerProxy constructor. However this means adding even more CORS API to the httpchannel API, and more CORS logic into the httpchannel. 3. We could modify the code that creates the preflight channel and make it not set the SEC_REQUIRE_CORS_DATA_INHERITS flag. But that makes the channel creation code messier since we can't copy the loadinfo. 4. We could move the preflight logic from nsCORSListenerProxy to nsCORSPreflightListener. This is a bigger change, but creates a cleaner separation between CORS checks and preflight logic. Attaching a patch which does #4
Attachment #8669158 -
Flags: review?(ehsan)
Updated•9 years ago
|
Attachment #8669158 -
Attachment is patch: true
Comment 1•9 years ago
|
||
Comment on attachment 8669158 [details] [diff] [review] Implement approach 4 Review of attachment 8669158 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsCORSListenerProxy.cpp @@ +1171,5 @@ > } > > +nsresult > +nsCORSPreflightListener::CheckPreflightRequestApproved(nsIRequest* aRequest) > +{ I think you should handle gDisableCORS here similar to nsCORSListenerProxy::CheckRequestApproved(). @@ +1192,5 @@ > + > + nsAutoCString headerVal; > + // The "Access-Control-Allow-Methods" header contains a comma separated > + // list of method names. > + http->GetResponseHeader(NS_LITERAL_CSTRING("Access-Control-Allow-Methods"), Hmm, what about checking Access-Control-Allow-Origin? IINM with this patch we no longer check that on preflight requests, which is wrong. In case I'm right here, can you also check to see if we have no tests that test a preflight response being rejected because of the Access-Control-Allow-Origin header? If we don't have such a test, we should add one.
Attachment #8669158 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 2•9 years ago
|
||
> > +nsresult > > +nsCORSPreflightListener::CheckPreflightRequestApproved(nsIRequest* aRequest) > > +{ > > I think you should handle gDisableCORS here similar to > nsCORSListenerProxy::CheckRequestApproved(). Good catch! I added it to nsCORSListenerProxy::StartCORSPreflight instead though, so that we don't even do the OPTIONS request, and so that we also prevent codepaths that would otherwise have gone through the prefight cache. > Hmm, what about checking Access-Control-Allow-Origin? There's still a nsCORSListenerProxy (sometimes provided by AsyncOpen2) which checks this. Even on the OPTIONS request. I just move the preflight-specific checks to the preflight-listener.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jonas
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8669158 -
Attachment is obsolete: true
Attachment #8669926 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
Oops! Added the check to the wrong patch in my queue. Here's a correct patch.
Attachment #8669926 -
Attachment is obsolete: true
Attachment #8669926 -
Flags: review?(ehsan)
Attachment #8669932 -
Flags: review?(ehsan)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8669932 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 5•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/088475a1642e27ed625b545f98682b769737e87d Bug 1211000: Move CORS preflight logic from nsCORSListenerProxy to nsCORSPreflightListener. r=ehsan
https://hg.mozilla.org/mozilla-central/rev/088475a1642e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8669932 [details] [diff] [review] Fix review comment Approval Request Comment [Feature/regressing bug #]: bug 1182537. We considered backing the patch out from gecko 43 (it was already backed out from 42), but the merge conflicts makes this more risky than landing the patch in bug 1211000 (this bug) [User impact if declined]: We don't do certain CORS-related security checks correctly when the sendBeacon API is used. [Describe test coverage new/current, TreeHerder]: Our CORS code is very well tested when used through XMLHttpRequest and fetch(). So the affected code is very well tested in CI. [Risks and why]: The risks are pretty low given how well tested this code is in CI. The patch has also been landed on trunk for a a few weeks with no known issues. Additionally, the logic changes in this patch are pretty simple and should have no behavioral changes other than for sendBeacon. While the patch isn't small, it mainly consists of moving code around. [String/UUID change made/needed]: None
Attachment #8669932 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
status-firefox43:
--- → affected
Comment 8•9 years ago
|
||
Comment on attachment 8669932 [details] [diff] [review] Fix review comment Corrects a security check, has some test coverage. OK to uplift to beta. Also, if we break this I'd rather know earlier than later.
Attachment #8669932 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 9•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9576cf65c2b5
You need to log in
before you can comment on or make changes to this bug.
Description
•