Closed Bug 1041180 Opened 6 years ago Closed 5 years ago

Remove deprecated nsIChannelPolicy

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 1006868
Depends on: 1038756
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: nobody → mozilla
Status: NEW → ASSIGNED
Attached patch bug_1041180_use_loadinfo.patch (obsolete) — Splinter Review
Attachment #8486743 - Flags: review?(sstamm)
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+
(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+
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 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 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 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+
(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)
> --- 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
(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
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.
Keywords: leave-open
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
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
Patches are slightly bitroted, putting up new ones.
Attachment #8486742 - Attachment is obsolete: true
Attachment #8488068 - Attachment is obsolete: true
Attachment #8499486 - Flags: review+
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)
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)
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)
(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!
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)
(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!
Depends on: 1083941
You need to log in before you can comment on or make changes to this bug.