Closed
Bug 1193924
Opened 7 years ago
Closed 7 years ago
Add assertion to AsyncOpen to make sure AsyncOpen(2) was called first when securityflags in loadInfo are set
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
Attachments
(1 file, 1 obsolete file)
12.73 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
We should land assertions to make sure that asyncOpen(2) is always called before asyncOpen(). We can make sure that happenened by investigating the 'securtyMode' within the Loadinfo to determine if asyncOpen(2) should have been called. If so, we can check that 'initialSecurityChecksDone' flag is set. I think it's particularly relevant now that we are getting into callsites where channel creation and opening happens at different places.
Assignee | ||
Comment 1•7 years ago
|
||
Do you really have to assign an already_addrefed value to a temprorary first, before you can use it? E.g in the case of nsJSChannel::AsyncOpen, do I really have to assign the loadInfo to a tmp variable? It seems, all over the codebase, this is what we do :-(
Also, I think we may want to also check for mLoadInfo first, because if an addon calls newChannel instead of newChannel2, then the channel does not have a loadInfo. So the assertion looks something like this:
> MOZ_ASSERT(mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
> mLoadInfo->GetInitialSecurityCheckDone(),
> "security flags in loadInfo but asyncOpen(2) not called");
What do you think Jonas? Do we still care about the case where the channel might not have a loadInfo??
Attachment #8647121 -
Flags: review?(jonas)
Attachment #8647121 -
Flags: review?(jonas) → review+
Attachment #8647121 -
Flags: review+ → review-
> Do you really have to assign an already_addrefed value to a temprorary > first, before you can use it? E.g in the case of nsJSChannel::AsyncOpen, do > I really have to assign the loadInfo to a tmp variable? It seems, all over > the codebase, this is what we do :-( If you didn't assign to a temporary, you'd leak. So yes, you have to assign an already_AddRefed before you can use it. But I don't know why you are use nsCOMPtr<nsILoadInfo> loadInfo; GetLoadInfo(getter_AddRefs(loadInfo)); rather than nsCOMPtr<nsILoadInfo> loadInfo = GetLoadInfo(); ? > Also, I think we may want to also check for mLoadInfo first, because if an > addon calls newChannel instead of newChannel2, then the channel does not > have a loadInfo. So the assertion looks something like this: > > > MOZ_ASSERT(mLoadInfo || mLoadInfo->GetSecurityMode() == 0 || > > mLoadInfo->GetInitialSecurityCheckDone(), > > "security flags in loadInfo but asyncOpen(2) not called"); > > What do you think Jonas? Do we still care about the case where the channel > might not have a loadInfo?? Yes. We still care about addons creating channels. I don't know why anything would have changed here? However, your code above gets the nullcheck wrong. What you want is MOZ_ASSERT(!mLoadInfo || mLoadInfo->GetSecurityMode() == 0 || mLoadInfo->GetInitialSecurityCheckDone(), "security flags in loadInfo but asyncOpen(2) not called");
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #2) > > What do you think Jonas? Do we still care about the case where the channel > > might not have a loadInfo?? > > Yes. We still care about addons creating channels. I don't know why anything > would have changed here? I updated the assertion to account for a null loadInfo. What I meant is, do we still care about addon code for debug builds, because in release builds they would work just fine. Anyway, I think it makes the most sense to make the assertion account for a null loadInfo. Further, there are 2 cases where we have to use a slightly different assertion, namely in * nsJARChannel::AsyncOpen * nsViewSourceChannel::AsyncOpen In those two cases the outer channel does not perform security checks in AsyncOpen(2), but rather sets the 'EnforceSecurityFlag'. I think it still makes sense to assert on that in those two cases if the securityMode is set in the LoadInfo.
Attachment #8647121 -
Attachment is obsolete: true
Attachment #8647307 -
Flags: review?(jonas)
Comment on attachment 8647307 [details] [diff] [review] bug_1193924_asyncopen_assertions.patch Review of attachment 8647307 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed. ::: dom/jsurl/nsJSProtocolHandler.cpp @@ +569,5 @@ > + { > + nsCOMPtr<nsILoadInfo> loadInfo = nsIChannel::GetLoadInfo(); > + MOZ_ASSERT(!loadInfo || loadInfo->GetSecurityMode() == 0 || > + loadInfo->GetInitialSecurityCheckDone(), > + "security flags in loadInfo but asyncOpen(2) not called"); Err.. all of these should say asyncOpen2, right? asyncOpen was called.
Attachment #8647307 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 5•7 years ago
|
||
url: https://hg.mozilla.org/integration/mozilla-inbound/rev/f99b27e40987b982c17fa8271ad94754228f4e92 changeset: f99b27e40987b982c17fa8271ad94754228f4e92 user: Christoph Kerschbaumer <mozilla@christophkerschbaumer.com> date: Wed Aug 12 21:36:33 2015 -0700 description: Bug 1193924 - Add assertion to AsyncOpen to make sure asyncOpen2() was called first when securityflags in loadInfo are set (r=sicking)
Comment 6•7 years ago
|
||
Backed out for B2G emulator bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=12829829&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ad1b01b299
Make mLoadInfo protected rather than private
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57dc2cd0ff0
Assignee | ||
Comment 9•7 years ago
|
||
url: https://hg.mozilla.org/integration/mozilla-inbound/rev/276b3c44eff17b097d3cdc3c1f769597d2aaf62e changeset: 276b3c44eff17b097d3cdc3c1f769597d2aaf62e user: Christoph Kerschbaumer <mozilla@christophkerschbaumer.com> date: Wed Aug 12 21:36:33 2015 -0700 description: Bug 1193924 - Add assertion to AsyncOpen to make sure asyncOpen2() was called first when securityflags in loadInfo are set (r=sicking)
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/276b3c44eff1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•