Closed
Bug 1041180
Opened 10 years ago
Closed 10 years ago
Remove deprecated nsIChannelPolicy
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: ckerschb, Assigned: ckerschb)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 3 obsolete files)
113.47 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
6.98 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
In bug 1038756, we store node/principal/contentPolicyType on all channels relevant for evaluating CSP (also MCB) even after redirects. We store the needed information at construction time of every channel going through nsNetUtil.h. Hence the is no need to store the channelPolicy in addition on every channel because the needed information can be requested from the loadinfo.
Assignee | ||
Updated•10 years ago
|
Comment 1•10 years ago
|
||
Since there is a csp directive for websockets and since websockets don't have loadinfo yet, we probably need to add loadinfo to them before we can remove channelPolicy. However, do websocket channels today have a channelPolicy? If they don't, then on redirect we fail open and allow the load: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#243
Depends on: 1037669
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8486742 -
Flags: review?(sstamm)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8486743 -
Flags: review?(sstamm)
Comment 4•10 years ago
|
||
Comment on attachment 8486743 [details] [diff] [review] bug_1041180_use_loadinfo.patch Review of attachment 8486743 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you address these minor comments. Looks good (once bug 1038756 lands it should work). ::: content/base/src/nsCSPService.cpp @@ +15,3 @@ > #include "nsIChannelEventSink.h" > #include "nsIPropertyBag2.h" > #include "nsIWritablePropertyBag2.h" Can you remove nsIPropertyBag2.h and nsIWritablePropertyBag2.h from the includes (or did I miss a place where they're used)? Also consider removing them from nsCSPContext.cpp if we don't need them there either. @@ +245,2 @@ > > + // The Node's Principal and the Principal set loadInfo do *not* necessarily match Nit: "Principal-set" (hyphenate it please or change to "loadInfo's principal") @@ +266,5 @@ > + * Content Policy implementation directly when redirects occur using the > + * information set in the LoadInfo when channels are created. > + * > + * We check if the CSP permits this host for this type of load, if not, > + * we cance the load now. Nit: "cance" needs an l (cancel) @@ +276,5 @@ > nsCOMPtr<nsIURI> originalUri; > oldChannel->GetOriginalURI(getter_AddRefs(originalUri)); > + NS_ENSURE_SUCCESS(rv, rv); > + nsContentPolicyType policyType = loadInfo->GetContentPolicyType(); > + Need to assign rv on the GetOriginalURI call if you want to check for success afterwards. @@ +283,5 @@ > + newUri, > + nullptr, // aRequestOrigin > + nullptr, // aRequestContext > + EmptyCString(), // aMimeTypeGuess > + originalUri, Please put the argument label comments back to make it more obvious what we're passing and what role the content policy puts them into. @@ +303,5 @@ > ("CSPService::AsyncOnChannelRedirect CANCELLING request.")); > #endif > > // if ShouldLoad doesn't accept the load, cancel the request > + if (aDecision != nsIContentPolicy::ACCEPT) { Consider using NS_CP_ACCEPTED(aDecision). http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentPolicyUtils.h#41 @@ -329,5 @@ > - nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > - NS_LITERAL_CSTRING("Redirect Error"), nullptr, > - nsContentUtils::eDOM_PROPERTIES, > - "InvalidRedirectChannelWarning", > - formatParams, 1); Is this error no longer necessary to put in the console, or have we eradicated this error case? Seems to me it's okay to delete since we're not dealing with nsIWritablePropertyBag2 anymore...
Attachment #8486743 -
Flags: review?(sstamm) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #4) > r=me if you address these minor comments. Looks good (once bug 1038756 > lands it should work). Thanks Sid, addressed all of your nits and updated the patch. > > - nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > > - NS_LITERAL_CSTRING("Redirect Error"), nullptr, > > - nsContentUtils::eDOM_PROPERTIES, > > - "InvalidRedirectChannelWarning", > > - formatParams, 1); > > Is this error no longer necessary to put in the console, or have we > eradicated this error case? Seems to me it's okay to delete since we're not > dealing with nsIWritablePropertyBag2 anymore... Exactly, I think that warning is outdated since we are no longer make us of the propertybag. Bug 1038756 makes sure that the information formerly stored in nsIChannelPolicy remains on the channel for the entire lifetime of a channel.
Attachment #8486743 -
Attachment is obsolete: true
Attachment #8488068 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8486742 [details] [diff] [review] bug_1041180_remove_channelPolicy_removal_part.patch Just chatted with Sid on IRC, can you please also take a look at this?
Attachment #8486742 -
Flags: review?(jst)
Comment 7•10 years ago
|
||
Comment on attachment 8486742 [details] [diff] [review] bug_1041180_remove_channelPolicy_removal_part.patch r=jst, but please have a necko guy look at the necko changes here. And yes, it seems nsNetStrings.cpp can be deleted, unless there's work underway already to add more there.
Attachment #8486742 -
Flags: review?(jst) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8486742 [details] [diff] [review] bug_1041180_remove_channelPolicy_removal_part.patch Review of attachment 8486742 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: netwerk/base/src/nsNetStrings.cpp @@ +4,5 @@ > > #include "nsNetStrings.h" > #include "nsChannelProperties.h" > > +// XXXckerschb: can we delete that file comletely? Looks like it. Go for it. No need to re-review for that.
Attachment #8486742 -
Flags: review+
Comment 9•10 years ago
|
||
Comment on attachment 8486742 [details] [diff] [review] bug_1041180_remove_channelPolicy_removal_part.patch Review of attachment 8486742 [details] [diff] [review]: ----------------------------------------------------------------- looks good.
Attachment #8486742 -
Flags: review?(sstamm) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc9903ff28f https://hg.mozilla.org/integration/mozilla-inbound/rev/1cfb645267a4
Target Milestone: --- → mozilla35
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/709cc1dcd23d for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=48720379&tree=Mozilla-Inbound
Flags: needinfo?(mozilla)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #11) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/709cc1dcd23d for > build bustage: > > > https://tbpl.mozilla.org/php/getParsedLog.php?id=48720379&tree=Mozilla- > Inbound Thanks for backing it out; had it on try earlier: https://tbpl.mozilla.org/?tree=Try&rev=b9003de6b3a5 but then uploaded the wrong patch which does not have the include: > +#include "nsContentPolicyUtils.h" where NS_CP_ACCEPTED is defined. Will push again tomorrow.
Flags: needinfo?(mozilla)
Assignee | ||
Comment 13•10 years ago
|
||
Here we go: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb508b6f0c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/c6fe8e2c41db
Comment 14•10 years ago
|
||
sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48762465&tree=Mozilla-Inbound
Assignee | ||
Comment 15•10 years ago
|
||
> --- a/image/test/unit/async_load_tests.js > +++ b/image/test/unit/async_load_tests.js > > - requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, null, null, outer, null, 0, null, null)); > + requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, null, null, outer, null, 0, null)); It seems that JS is calling LoadImageXPCOM, which would also align with the error we are getting: https://tbpl.mozilla.org/php/getParsedLog.php?id=48762465&tree=Mozilla-Inbound#error0 Pushing to try again to make sure this was causing the problem: https://bugzilla.mozilla.org/show_bug.cgi?id=1041180
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15) > > --- a/image/test/unit/async_load_tests.js > > +++ b/image/test/unit/async_load_tests.js > > > > - requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, null, null, outer, null, 0, null, null)); > > + requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, null, null, outer, null, 0, null)); > > It seems that JS is calling LoadImageXPCOM, which would also align with the > error we are getting: > https://tbpl.mozilla.org/php/getParsedLog.php?id=48762465&tree=Mozilla- > Inbound#error0 That's what caused the error, try is green now: https://tbpl.mozilla.org/?tree=Try&rev=eb5bd75ec2d5
Assignee | ||
Comment 17•10 years ago
|
||
Alright, now everything should be fine: https://hg.mozilla.org/integration/mozilla-inbound/rev/693507b38116 https://hg.mozilla.org/integration/mozilla-inbound/rev/11fc11a90d6b
Comment 18•10 years ago
|
||
Try was green, inbound not so much. Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/11fc11a90d6b in https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c01d77aee0 for a bunch of the same sort of crashes again.
Updated•10 years ago
|
Keywords: leave-open
Comment 19•10 years ago
|
||
And backed out 693507b38116 in https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5e78b0cc56 since it had absolutely no desire to build without its friend around for company.
Keywords: leave-open
Assignee | ||
Comment 20•10 years ago
|
||
One more time; fixed the problem caused by a incorrect argument for > loadImageXPCOM() pushed to try to verify that everything is fine now: https://tbpl.mozilla.org/?tree=Try&rev=8c5911adab3c That should work now: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2192165100c https://hg.mozilla.org/integration/mozilla-inbound/rev/42614739736d
Comment 21•10 years ago
|
||
Reverted for xpcshell failures: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2734441&repo=mozilla-inbound https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2733642&repo=mozilla-inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c107ce32cbc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5ced212c7d
Assignee | ||
Comment 22•10 years ago
|
||
Patches are slightly bitroted, putting up new ones.
Attachment #8486742 -
Attachment is obsolete: true
Attachment #8488068 -
Attachment is obsolete: true
Attachment #8499486 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8499488 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
I can't imagine what's causing the discrepancy here. The tests, in particular: > image/test/unit/test_async_notification.js passes locally on linux as well as mac. I pushed to try (See Comment 20) and it worked just fine. Manual inspection of the code in question also lets me conclude that the code should be correct. Tanvi, since I am on PTO next week, any chance you could take a look at this and probably land?
Flags: needinfo?(tanvi)
Comment 25•10 years ago
|
||
I've been busy with a couple other bugs this week, so haven't had a chance to look at the xpcshell failure. Removing needinfo and will discuss with Christoph when he is back next week.
Flags: needinfo?(tanvi)
Assignee | ||
Comment 26•10 years ago
|
||
Ryan, patches for this bug got backed out twice; reason is: > https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2733642&repo=mozilla-inbound I manually verified that we are passing the right number of arguments and pushed to try again: > https://tbpl.mozilla.org/?tree=Try&rev=029086fe2bdb which shows no failures for xpcshell tests. Mrbkap brought to my attention that I haven't pumped the uuid in public/imgILoader.idl initially. I am not convinced that solves the issue on inbound. Should/can I try to push to inbound again? Or do you have any other suggestions I could try before trying to re-land the patches?
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26) > Ryan, patches for this bug got backed out twice; reason is: > > https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2733642&repo=mozilla-inbound > > I manually verified that we are passing the right number of arguments and > pushed to try again: > > https://tbpl.mozilla.org/?tree=Try&rev=029086fe2bdb > which shows no failures for xpcshell tests. > > Mrbkap brought to my attention that I haven't pumped the uuid in > public/imgILoader.idl initially. I am not convinced that solves the issue on > inbound. Should/can I try to push to inbound again? > Or do you have any other suggestions I could try before trying to re-land > the patches? To be more precise, try is green, but inbound shows failures for xpcshell tests. I am pretty sure the code in the patches works correctly (no errors on try and also passes xpcshell tests locally). My question is, what is different on inbound? I am trying to avoid another back-out. Thanks!
Comment 28•10 years ago
|
||
The difference is that try always does clobber builds, and inbound does not. If you don't rev a uuid while making an IDL change, then a clobber build will successfully build with all your changes, and a non-clobber build might build with all your changes, or might just build with some but not all, and might then fail to have your newly-added feature working, or might crash on startup, or might not be visibly broken, only invisibly broken.
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #28) > The difference is that try always does clobber builds, and inbound does not. > If you don't rev a uuid while making an IDL change, then a clobber build > will successfully build with all your changes, and a non-clobber build might > build with all your changes, or might just build with some but not all, and > might then fail to have your newly-added feature working, or might crash on > startup, or might not be visibly broken, only invisibly broken. Thanks!
Assignee | ||
Comment 30•10 years ago
|
||
Changing the uuid in imgILoader.idl should fix this problem, here we go again: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8328e5cbb3 https://hg.mozilla.org/integration/mozilla-inbound/rev/85d7bc9937ef
Assignee | ||
Comment 31•10 years ago
|
||
Changing the uuid in imgILoader.idl should fix this problem, here we go again: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8328e5cbb3 https://hg.mozilla.org/integration/mozilla-inbound/rev/85d7bc9937ef
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed8328e5cbb3 https://hg.mozilla.org/mozilla-central/rev/85d7bc9937ef
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Blocks: CVE-2015-0809
You need to log in
before you can comment on or make changes to this bug.
Description
•