Closed
Bug 1038756
Opened 10 years ago
Closed 10 years ago
Extending LoadInfo and storing LoadInfo on all channels created through NS_NewChannel
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: ckerschb, Assigned: ckerschb)
References
(Blocks 4 open bugs)
Details
Attachments
(34 files, 66 obsolete files)
9.78 KB,
patch
|
ckerschb
:
review+
sicking
:
review+
|
Details | Diff | Splinter Review |
27.49 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
33.84 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
3.65 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
3.22 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.17 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
19.51 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
25.43 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
10.49 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.50 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
19.74 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
1.61 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
12.72 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.17 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
3.32 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
4.87 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.70 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.01 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
4.17 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
4.72 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.28 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.21 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
18.37 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
16.93 KB,
patch
|
bzbarsky
:
review+
peterv
:
review+
|
Details | Diff | Splinter Review |
1.14 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
Currently, the LoadInfo is only set on a sparse number of channels. We could extend the LoadInfo [1] with a contentPolicyType and also a loadingContext which would allow us to classify and handle redirects for Mixed Content Blocking [2] correctly if set on all channels. + interface nsILoadInfo : nsISupports + { + ... + readonly attribute nsContentPolicyType contentPolicyType; + readonly attribute nsISupports loadingContext; Further down the road, this would also allow us to replace the |channelPolicy| currently set on channels to handle redirects for Content Security Policy (CSP). Instead of setting the LoadInfo after creation of a channel, we could hand the needed information (principal, context, contentPolicyType), to NS_NewChannel, which would then create the LoadInfo and set it on every channel. Agreed, not on every channel, because not all the creations of channels go through NS_NewChannel in nsNetutil.h, but at least all the channels we do care about for CSP and MCB are created through NS_NewChannel. For this approach to work we would also have to set the LoadInfo on channels where the principal currently is null, e.g. in nsDocshell, which calls SetupChannelOwner [3] which currently does *not* create a LoadInfo in case the principal is null. Bz, would it cause any problems to have a Loadinfo on a channel where the principal is null? I think it's best if we extend NS_NewChannel with 5 arguments inline nsresult NS_NewChannel(nsIChannel **result, nsIURI *uri, nsIIOService *ioService = nullptr, // pass in nsIIOService to optimize callers nsILoadGroup *loadGroup = nullptr, nsIInterfaceRequestor *callbacks = nullptr, uint32_t loadFlags = nsIRequest::LOAD_NORMAL, nsIChannelPolicy *channelPolicy = nullptr, + nsContentPolicyType aType, + nsIPrincipal* aRequestingPrincipal, + nsINode* aRequestingNode, + InheritType aInheritPrincipal, + SandboxType aSandboxed); Please note, that we have to set both, aRequestingNode as well as aRequestingPrincipal, because we do not always have a RequestingNode available from which we could query the principal through node->RequestingPrincipal() when creating a channel through NS_NewChannel. In general, we should never end up with a channel without a principal, so we can always reason about security. Fixing this bug would also be a perfect start on the way to implement Jonas proposal [4,5] where we call contentPolicies after a channel is created. Bz, what's your opinion on this? [1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadInfo.idl#14 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=418354 [3] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#6501 [4] https://etherpad.mozilla.org/BetterNeckoSecurityHooks [5] https://bugzilla.mozilla.org/show_bug.cgi?id=1006881
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 1•10 years ago
|
||
WIP patch extending nsILoadInfo
Assignee | ||
Comment 2•10 years ago
|
||
WIP patch for extending NS_NewChannel API
Assignee | ||
Comment 3•10 years ago
|
||
Now we have to update all the callsites to pass in the required info to NS_NewChannel. The big question is: Can we use the systemPrincipal for cases like nsStringBundle [1]. In my opinion we can use the systemPrincipal, because right now that case does not even undergo a shouldLoad call and hence is not evaluated by contentPolcies anyway. Anyone knows? [1] http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#73
Comment 4•10 years ago
|
||
Using a system principal for stringbundle sounds fine to me.
Flags: needinfo?(bzbarsky)
Comment 5•10 years ago
|
||
> Bz, would it cause any problems to have a Loadinfo on a channel where the principal is > null? Yes: current consumers expect it to have a non-null principal and will crash if it's null. That said, we could just create a NullPrincipal in this situation. Note bug 1037271. > In general, we should never end up with a channel without a principal, so we can always > reason about security. Right. Which means we should figure out what cases currently pass null to SetupChannelOwner and see whether they should do something else.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8456333 -
Attachment is obsolete: true
Attachment #8456334 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Adding the first set of WIP patches; we still need to update callers of NS_NewInputStreamChannel. Boris, can you take a first look and provide some feedback? In general, I would really like to pass info about principal/node/contentPolicyType to NS_NewChannel and all other API functions in nsNetutil.h that end up calling NS_NewChannel. For this to work, we have to modify SetupChannelOwner (InheritChannelOwner) because we don't want to set the loadInfo after cration of the channel, but rather at creation time to make sure all channels end up having the loadInfo. The ultimate goal of Jonas proposal is to have the loadInfo on *all* channels, not only those created in nsNetutil.h, but that's further down the road - in a different patch.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 13•10 years ago
|
||
CC'ing smaug on this bug as well, because he was one of the reviewers for the initial LoadInfo patch - so he might have some input as well.
Comment 14•10 years ago
|
||
Mike, there's docshell stuff in this bug. ;)
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Comment 17•10 years ago
|
||
Those WIP patches is all the work I would like to happen in this bug so that all channels created through NS_NewChannel have the loadingInfo attached. We have to be really careful and evaluate carefully which node/principal/type goes into what channel. Probably we should replace all the systemPrincpals with NullPrincipals. I am not sure, probably it would be safer, but then, once we move all security checks (ContentPolicy) checks to AsyncOpen all of these channels will fail, so probably keeping the systemPrincipal is the better strategy.
Comment 18•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17) > Those WIP patches is all the work I would like to happen in this bug so that > all channels created through NS_NewChannel have the loadingInfo attached. We > have to be really careful and evaluate carefully which node/principal/type > goes into what channel. Probably we should replace all the systemPrincpals > with NullPrincipals. I am not sure, probably it would be safer, but then, > once we move all security checks (ContentPolicy) checks to AsyncOpen all of > these channels will fail, so probably keeping the systemPrincipal is the > better strategy. We should try to find the actual loading principal and loading node for as many places as possible. The loadinfo will likely be used by consumers besides the security checking code in the future.
Comment 19•10 years ago
|
||
Christoph, it seems like the obvious thing is to change SetupChannelOwner to return an nsILoadInfo (and probably a boolean for whether to reset the channel owner to null, or move that SetOwner(nullptr) logic into callers based on the sandbox flag in the loadinfo) which will then be used when the channel is actually created. Or even just the boolean for whether to inherit, I guess: that's the one part SetupChannelOwner seriously computes, other than the actual channel-munging it does.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #19) > Christoph, it seems like the obvious thing is to change SetupChannelOwner to > return an nsILoadInfo (and probably a boolean for whether to reset the > channel owner to null, or move that SetOwner(nullptr) logic into callers > based on the sandbox flag in the loadinfo) which will then be used when the > channel is actually created. > > Or even just the boolean for whether to inherit, I guess: that's the one > part SetupChannelOwner seriously computes, other than the actual > channel-munging it does. If you look at patch 5 [1], we change SetupChannelOwner to InheritChannelOwner which returns a boolean whether to inherit the channel owner or not and resets the owner (SetOwner(null)) on the channel if needed right after channel creation. Please note that I moved SetupChannelOwner (InheritChannelOwner) to be called right before creation of the channel. If you look at patch 2 [2], we change the function signature for NS_NewChannel, which then internally creates the loadInfo, so that all channels created in nsNetUtil.h have the loadInfo attached at creation time. Hence I would rather have the former SetupChannelOwner only return a boolean whether to inherit or not and hand all the information needed to create a loadInfo to NS_NewChannel (patch 3) [3]. I applied the patches (slightly modified) and ran mochitests (locally), it seems to perform the same task and behaves the same way. I think the next step is to make sure that we use the correct node/principal in all cases. Any objections to this design? The ultimate goal is to have the loadInfo on all channels, so after landing this patch we would probably change *.idl files of ioService so that we can hand the loadInfo to 'newChannel', but that will break addons in turn, so I would like to do as much work (add loadInfo to as many channel as possible) before we have to break addons. Probably we can implement a shim, some kind of wrapper as an interim solution; but lets focus on what's important for this bug. [1] https://bug1038756.bugzilla.mozilla.org/attachment.cgi?id=8458134 [2] https://bugzilla.mozilla.org/attachment.cgi?id=8458131 [3] https://bugzilla.mozilla.org/attachment.cgi?id=8458132
Comment 21•10 years ago
|
||
> we change SetupChannelOwner to InheritChannelOwner which returns a boolean Right, that was my suggestion at the end of comment 19 as well. Looking at the patch, more carefully, that means we end up duplicating a bunch of logic (and in particular the conversion from booleans to loadinfo enums) and the SetOwner(nullptr) at the callsites, which is a bit unfortunate. We could fix the booleans bit via having InheritChannelOwner() (maybe call it ChannelShouldInheritPrincipal?) return the enum and by having an accessor on the document that returns the sandboxed enum instead of duplicating the GetSandboxFlags() & SANDBOXED_ORIGIN thing and its conversion to enums. We could also push the SetOwner(nullptr) bit down into NS_NewChannel, right? With that done, I think this would look reasonable. The SetOwner(nullptr) bit is definitely NOT a no-op, unless I'm seriously missing something, at least as long as we have protocol handler implementations that create channels with the owner already set.
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #21) > > we change SetupChannelOwner to InheritChannelOwner which returns a boolean > > Right, that was my suggestion at the end of comment 19 as well. > > Looking at the patch, more carefully, that means we end up duplicating a > bunch of logic (and in particular the conversion from booleans to loadinfo > enums) and the SetOwner(nullptr) at the callsites, which is a bit > unfortunate. I don't like that either, so my plan was to file a follow up and collapse all securityFlags into one unsigned long, where we 'or' all the different flags. There are more flags btw, see Jonas proposal at line 86: https://etherpad.mozilla.org/BetterNeckoSecurityHooks Anyway, as a first step I moved the enums into the idl, mostly because I couldn't access them in compiled code tests, but also as a first step towards having only *one* unsigned long that holds all the different flags. Similar to the loadFLags. > We could fix the booleans bit via having InheritChannelOwner() (maybe call > it ChannelShouldInheritPrincipal?) return the enum and by having an accessor > on the document that returns the sandboxed enum instead of duplicating the > GetSandboxFlags() & SANDBOXED_ORIGIN thing and its conversion to enums. > > We could also push the SetOwner(nullptr) bit down into NS_NewChannel, right? > With that done, I think this would look reasonable. That sounds like the best solution to me, we should have that part in NS_NewChannel where we can call SetOwner(nullptr) immediately after the channel was created by the ioservice. > The SetOwner(nullptr) bit is definitely NOT a no-op, unless I'm seriously > missing something, at least as long as we have protocol handler > implementations that create channels with the owner already set. Oh oh - my bad - facepalm. For some reason I had the impression the channel owner is a nullptr initially.
Assignee | ||
Comment 23•10 years ago
|
||
Since the patches here probably are going to change a lot I created a repo for our patches: http://hg.mozilla.org/users/mozilla_christophkerschbaumer.com/revamp-patches/ where we can track progress.
Assignee | ||
Comment 24•10 years ago
|
||
Boris: when storing the loadInfo on all channels that are created through NS_NewChannel we are going to run into a problem, which is best visualized when running test |test_CrossSiteXHR.html|. Especially relevant are the CORS tests (introduced/modified in this bug [1]) that cause problems - fail. Taking a closer look reveals that GetChannelPrincipal in the scriptSecurityManager returns the principal stored in the loadInfo [2] (because we have it on so many channels now). Since XMLHttpRequests use InheritPrincipal it returns too early; in fact it should return the CodeBasePrincipal [3]. In the end that causes the CorsListenerProxy to compare the wrong principals [4] and the tests to fail. What do you think is the best way to fix this problem? Thanks! [1] https://bugzilla.mozilla.org/show_bug.cgi?id=814141 [2] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#277 [3] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#294 [4] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCrossSiteListenerProxy.cpp#727
Flags: needinfo?(bzbarsky)
Comment 25•10 years ago
|
||
Hmm. So maybe the right answer is to not tell XHR (and probably various other callsites) to inherit the principal. I'm not sure why we're doing that anyway, since XHR then later stamps the principal on the document itself. Check the blame? The other option is to have two different GetChannelPrincipal functions, one of which considers inheritance and one of which does not, for answering the "what principal should I use for this data" and "where is this data coming from" questions. Sort of like for scripts we have distinct principals and originPrincipals...
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #25) > Hmm. > > So maybe the right answer is to not tell XHR (and probably various other > callsites) to inherit the principal. I think you mean *not* inherit the principal, because at the moment XML reqeuests inherit. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1978 I'm not sure why we're doing that > anyway, since XHR then later stamps the principal on the document itself. Anyway, as a first attempt I changed XHR to not inherit the principal, but that causes the test_XHR* test family to break :-( > The other option is to have two different GetChannelPrincipal functions, one > of which considers inheritance and one of which does not, for answering the > "what principal should I use for this data" and "where is this data coming > from" questions. Sort of like for scripts we have distinct principals and > originPrincipals... I would really like to have that separated as well. The current situation seems like a mix of channel-owner and principal in the loadInfo, where I am not sure when do use which one and sometimes they should be in sync and sometimes not. I have a meeting with Jonas in 30 minutes, will ask for more input from his side too.
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26) > (In reply to Boris Zbarsky [:bz] from comment #25) > > Hmm. > > > > So maybe the right answer is to not tell XHR (and probably various other > > callsites) to inherit the principal. > > I think you mean *not* inherit the principal, because at the moment XML > reqeuests inherit. > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsXMLHttpRequest.cpp#1978 Oh boy, my english :-( I think you meant the right thing. To be explicit, if I change XMLHttpRequests to *not* inherit the principal, then test_CrossSiteXHR.html passes, but test_XHR*.html tests are failing and vice versa.
Comment 28•10 years ago
|
||
> but that causes the test_XHR* test family to break Can you tell why? > The current situation seems like a mix of channel-owner and principal in the loadInfo Yes, but we should be phasing out usage of "owner" until we kill it off completely. The split I'm talking about is unrelated to whether we have "owner" or not.
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #28) > Yes, but we should be phasing out usage of "owner" until we kill it off > completely. The split I'm talking about is unrelated to whether we have > "owner" or not. Borisl: Jonas is on the same page with you. So we agreed to add a new function to the scriptSecurityManager. I am going to inline the hot part here: > NS_IMETHODIMP > +nsScriptSecurityManager::GetChannelURIPrincipal(nsIChannel* aChannel, > + nsIPrincipal** aPrincipal) > +{ > + NS_PRECONDITION(aChannel, "Must have channel!"); > + > + // Get the principal from the URI. Make sure this does the same thing > + // as nsDocument::Reset and XULDocument::StartDocumentLoad. > + nsCOMPtr<nsIURI> uri; > + nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCOMPtr<nsILoadContext> loadContext; > + NS_QueryNotificationCallbacks(aChannel, loadContext); > + > + if (loadContext) { > + return GetLoadContextCodebasePrincipal(uri, loadContext, aPrincipal); > + } > + > + return GetCodebasePrincipalInternal(uri, UNKNOWN_APP_ID, > + /* isInBrowserElement */ false, aPrincipal); > +} > + > + > +NS_IMETHODIMP > nsScriptSecurityManager::GetChannelPrincipal(nsIChannel* aChannel, > nsIPrincipal** aPrincipal) > { > NS_PRECONDITION(aChannel, "Must have channel!"); > nsCOMPtr<nsISupports> owner; > aChannel->GetOwner(getter_AddRefs(owner)); > if (owner) { > CallQueryInterface(owner, aPrincipal); > @@ -281,32 +305,17 @@ nsScriptSecurityManager::GetChannelPrinc > rv = loadInfo->GetForceInheritPrincipal(&inheritPrincipal); > NS_ENSURE_SUCCESS(rv, rv); > if (inheritPrincipal == nsILoadInfo::eInheritPrincipal) { > NS_ADDREF(*aPrincipal = loadInfo->LoadingPrincipal()); > return NS_OK; > } > } > > - // OK, get the principal from the URI. Make sure this does the same thing > - // as nsDocument::Reset and XULDocument::StartDocumentLoad. > - nsCOMPtr<nsIURI> uri; > - nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri)); > - NS_ENSURE_SUCCESS(rv, rv); > - > - > - nsCOMPtr<nsILoadContext> loadContext; > - NS_QueryNotificationCallbacks(aChannel, loadContext); > - > - if (loadContext) { > - return GetLoadContextCodebasePrincipal(uri, loadContext, aPrincipal); > - } > - > - return GetCodebasePrincipalInternal(uri, UNKNOWN_APP_ID, > - /* isInBrowserElement */ false, aPrincipal); > + return GetChannelURIPrincipal(aChannel, aPrincipal); > } Inside of nsCrossSiteListenerProxy::AsyncOnChannelRedirect we are now calling GetChannelURIPrincipal instead of GetChannelPrincipal which resolves the problem.
Assignee | ||
Comment 30•10 years ago
|
||
Boris, Jonas: Since we still got some failures when running mochis but also in respect to future patches it would be great to inspect all the callsites of 'GetChannelPrincipal' and update callsites to call GetChannel*URI*Principal. At the moment we might get lucky to fall through to GetChannelURIPrincipal when calling GetChannelPrincipal if there is no loadInfo set on the channel (see current codebase [1], but also Comment 29). In the future, once we have loadInfos and all channels, we are going to run into that problem again. It would be great to fix up all callsites with this patch! I don't feel confident updating callsites to call GetChannel*URI*Principal where I am not 100% sure this is the right thing to do. Could any of you browse over the callsites [2] and let me know where we need to call GetChannel*URI*Principal instead of GetChannelPrincipal? If it is of any help, here are testcases that are failing, that seem to closely related due to querying the wrong principal from the channel: > parser/htmlparser/tests/mochitest/test_bug715739.html > browser/base/content/test/general/browser_bug484315.js > browser/modules/test/browser_UITour_annotation_size_attributes.js > browser/modules/test/browser_UITour_detach_tab.js [1] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#269 [2] http://mxr.mozilla.org/mozilla-central/search?string=GetChannelPrincipal
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Comment 31•10 years ago
|
||
I think the right thing to do here is for you to go through this list and for each callsite say which function it should use and why. And if you're not sure, why you're not sure. Then Jonas or I can look at that list and help you resolve the ones you're not sure about.
Flags: needinfo?(bzbarsky)
Comment 32•10 years ago
|
||
Driveby glance, but I'm wondering if you can shove all of the new fields (or even all of the old ones) into a single idl so that one someone wants to extend loadinfo even more, they don't have to change all of the callsites of NS_NewChannel. The reason I ask is that I need to shove something in, too! (the URI of the toplevel window would be particularly helpful)
There's only 29 callsites of GetChannelPrincipal. http://mxr.mozilla.org/mozilla-central/search?string=GetChannelPrincipal Maybe create a patch that: 1. Adds GetChannelURIPrincipal 2. Renames GetChannelPrincipal to GetChannelResultPrincipal 3. Changes GetChannelPrincipal to GetChannelURIPrincipal or GetChannelResultPrincipal as you think is appropriate. This way bz and I can review and see if things look good.
Flags: needinfo?(jonas)
Monica, you should be able to calculate the top-window URI using the node that's already available from the LoadInfo. So you could either modify the LoadInfo constructor to calculate it, add a new function on LoadInfo to calculate it, or simply calculate it in your code externally from the channel/loadinfo.
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8458130 -
Attachment is obsolete: true
Attachment #8458131 -
Attachment is obsolete: true
Attachment #8458132 -
Attachment is obsolete: true
Attachment #8458133 -
Attachment is obsolete: true
Attachment #8458134 -
Attachment is obsolete: true
Attachment #8458135 -
Attachment is obsolete: true
Attachment #8458229 -
Attachment is obsolete: true
Attachment #8458230 -
Attachment is obsolete: true
Assignee | ||
Comment 36•10 years ago
|
||
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Comment 38•10 years ago
|
||
Assignee | ||
Comment 39•10 years ago
|
||
Boris, Jonas: I put four patches up, which show the transition from SetupChannelOwner to ShouldInheritPrincipal for docShell and imgLoader. The ImgLoader needs special attention, because all of the tests that are still failing on TBPL [1] seem to be closely related to the imgLoader. I can't tell for sure because tests are timing out, and as of now, I don't really know why. The last patch (f4_getChannelPrincipal.patch) is a diff of the changes of the separation of calling GetChannelResultPrincipal and GetChannelURIPrincipal. Any feedback, in particular on the imgLoader part is highly appreciated. Please note that I filed a follow up bug for collapsing inheritPrincipal and isSandboxed, so we do not need two separate bools, enums whatever, but only one unsigned long where we can 'or' the flags. [1] https://tbpl.mozilla.org/?tree=Try&rev=3b2d8fb5e213
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 40•10 years ago
|
||
Maybe one thing of interest worth sharing is the part in NS_NewChannel where we set the LoadInfo.
> +NS_NewChannel(nsIURI* aUri,
> + nsIIOService* aIoService, // pass in nsIIOService to optimize callers
> + nsILoadGroup* aLoadGroup,
> + nsIInterfaceRequestor* aCallbacks,
> + uint32_t aLoadFlags, // nsIRequest::LOAD_NORMAL,
> + nsIChannelPolicy* aChannelPolicy,
> + nsIPrincipal* aRequestingPrincipal,
> + nsINode* aRequestingNode,
> + InheritType aInheritPrincipal,
> + SandboxType aSandboxed,
> + nsContentPolicyType aContentPolicyType,
> + nsIChannel** outChannel)
> {
> ...
> +
> + nsCOMPtr<nsILoadInfo> loadInfo =
> + new mozilla::LoadInfo(aRequestingPrincipal,
> + aRequestingNode,
> + aInheritPrincipal,
> + aSandboxed,
> + aContentPolicyType);
> + channel->SetLoadInfo(loadInfo);
> +
> + // If we're sandboxed, make sure to clear any owner the channel
> + // might already have.
> + if (aSandboxed == nsILoadInfo::eSandboxed) {
> + channel->SetOwner(nullptr);
> + // Make sure we have set a NullPrincipal
> + MOZ_ASSERT(aRequestingPrincipal->GetIsNullPrincipal(), "sandboxed channel needs a nullPrincipal");
> + }
> +
> + channel.forget(outChannel);
> + return NS_OK;
> + }
Formely the LoadInfo was set individually on channels after creation, now we attach a loadInfo to every channel *if* created through NS_NewChannel in nsNetutil.h
Comment 41•10 years ago
|
||
> MOZ_ASSERT(aRequestingPrincipal->GetIsNullPrincipal(), "sandboxed channel needs a nullPrincipal"); That looks wrong to me. The whole point of having the sandboxed state in the LoadInfo is that we can have a useful aRequestingPrincipal even for sandboxed stuff, no? >@@ -595,27 +597,46 @@ static nsresult NewImageChannel(nsIChann >+ requestingPrincipal = do_GetService(NS_NULLPRINCIPAL_CONTRACTID); This needs to be do_CreateInstance. Nothing else is obviously jumping out at me in the imgLoader changes.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #41) > > MOZ_ASSERT(aRequestingPrincipal->GetIsNullPrincipal(), "sandboxed channel needs a nullPrincipal"); > > That looks wrong to me. The whole point of having the sandboxed state in > the LoadInfo is that we can have a useful aRequestingPrincipal even for > sandboxed stuff, no? That is a good point - I will investigate that part again. But if we do not have a principal in nsDocShell or also imgLoader we should rather fall back to the NullPrincipal instead of the SystemPrincipal. Do you agree? > > >@@ -595,27 +597,46 @@ static nsresult NewImageChannel(nsIChann > >+ requestingPrincipal = do_GetService(NS_NULLPRINCIPAL_CONTRACTID); > > This needs to be do_CreateInstance. Obviously you are right - that problem was fixed in the wrong patch, hence my patch queue was messed up before exporting. > Nothing else is obviously jumping out at me in the imgLoader changes. Did you (can you) also browse over the changes where we query the ChannelPrincipal and see if we need to call GetChannel*URI*Principal instead of GetChannel*Result*Principal in one case or the other? I inspected all of the callsites, and it seems right at the moment, but I am not 100% sure.
Assignee | ||
Comment 43•10 years ago
|
||
In the current version Boris manually sets the LoadInfo once the channel is created in nsDocShell::DoURILoad. For what we are trying to accomplish in this bug we moved that logic a little further up so that the loadInfo-information gets passed to NS_NewChannel, which then creates and attaches the loadInfo to the channel. Problem is/was, that there is an if-else, where in the else branch the channel doesn't get created through NS_NewChannel, but rather by calling NS_NewInputStreamChannel [1]. Hence we have to manually set the loadInfo on such channels. That was causing a problem in particular for 'about:srcdoc'. Once we land patches following that initial groundwork here, we can delete all of the code that manually attaches the loadInfo to channels after creation because that information will be passed along to channel constructors. Anyway, this fix should resolve all of our problems at the moment. [1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9900 Also, clearing the needinfo flag for Jonas.
Flags: needinfo?(jonas)
Comment 44•10 years ago
|
||
> Did you (can you) also browse over the changes where we query the ChannelPrincipal
Will try to look tomorrow.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 45•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #44) > > Did you (can you) also browse over the changes where we query the ChannelPrincipal > > Will try to look tomorrow. Thanks Boris. Given the fix from Comment 43 it seems we are getting pretty close: https://tbpl.mozilla.org/?tree=Try&rev=b22afaa4a2dc We still have some assertion failures, will debug those today, but overall looks pretty good.
Assignee | ||
Comment 46•10 years ago
|
||
When debugging test crashtests/914521.html [1], I encountered the following problem: In nsGlobalWindow an assertion is triggered [2] where the uri in question is "moz-nullprincipal:{60b2b627-30d8-425e-b53e-06f8e4c363f1}" instead of "about:blank". Here is the information we used to create that channel: > NS_NewChannel { > uri(host): about:blank > aRequestingPrincipal: moz-nullprincipal:{60b2b627-30d8-425e-b53e-06f8e4c363f1} > aRequestingNode: yes > aInheritPrincipal: yes > aSandboxed: no > aContentPolicyType: 6 (TYPE_DOCUMENT) > } Interesting is, that when creating the channel, that in nsAboutProtocolHandler [3] we set the owner to null: > if (IsSafeForUntrustedContent(aboutMod, uri)) { > (*result)->SetOwner(nullptr); > } Later, when calling GetChannelPrincipal [4] we return the wrong principal because the inheritFlag is set and hence we return the nullPrincipal from the loadinfo instead of falling through to getting the uri (GetChannelURIPrincipal in the new code) of that channel. The reason why that is currently working (in mozilla-central) is because SetupChanelOwner does not set a loadInfo on the channel if the principal in nsDocShell is null. In our code, where we *always* want a principal in the loadInfo on the channel we fall back to the NullPrincipal if the loadingPrincipal is a nullptr. Currenlty that is the problem, the loadPrincipal in nsDocShell is null, we fall back to the nullPrincipal which we then set in the loadInfo. When quering the principal from the channel we get the nullPrincipal back because the owner was set to null in nsAboutProtocolHandler. The problem, in fact, happens a little earlier than where the assertion is triggered. Namely in nsDocument::Reset() [5] where we call GetChannelPrincipal (GetChannelResultPrincipal in the new code). Simply changing GetChannelResultPrincipal to GetChannelURIPrincipal (see attached patch 'f4_getChannelPrincipal.patch') in nsDocument::Reset() is obviously not correct, because we always want the 'ResultPrincipal' except for that special case in 'about:blank' described above. At the moment I am not sure what the best way forward is here, Boris, Jonas, any suggestions? [1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/crashtests/914521.html?force=1 [2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2073 [3] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#143 [4] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#340 [5] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2198
Assignee | ||
Comment 47•10 years ago
|
||
Probably the right thing to do is to fall back to the CodebasePrincipal instead of using the NullPrincipal if no requestingPrincipal is provided in nsDocShell.cpp.
> nsCOMPtr<nsIPrincipal> requestingPrincipal = do_QueryInterface(aOwner);
> if (!requestingPrincipal) {
> - requestingPrincipal = do_CreateInstance(NS_NULLPRINCIPAL_CONTRACTID, &rv);
> - NS_ENSURE_SUCCESS(rv, rv);
> + nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
> + NS_ENSURE_TRUE(secMan, NS_ERROR_NOT_AVAILABLE);
> + rv = secMan->GetCodebasePrincipal(aURI,
> + getter_AddRefs(requestingPrincipal));
> }
What other implications would that have?
Comment 48•10 years ago
|
||
> Boris, Jonas, any suggestions? We shouldn't set the inherit boolean to true if the owner in docshell was null. > Probably the right thing to do is to fall back to the CodebasePrincipal No, that would be pretending like the target of the load is responsible for the load, whereas in reality we just have no idea who's responsible. The _really_ ideal thing, of course, would be to make sure that the owner is never null... so we always know who's responsible for the load. That's a pretty delicate operation with security implications, though, so best done as a separate patch from everything else.
Flags: needinfo?(bzbarsky)
Comment 49•10 years ago
|
||
Comment on attachment 8465029 [details] [diff] [review] f4_getChannelPrincipal.patch >+ nsIPrincipal GetChannelResultPrincipal(in nsIChannel aChannel); getChannelResultPrincipal The rest of this looks good to me.
Assignee | ||
Comment 50•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #48) > > Boris, Jonas, any suggestions? > > We shouldn't set the inherit boolean to true if the owner in docshell was > null. That seems to work just fine. To be consistent I also did this in imgLoader.cpp and also in nsObjectLoadingContent.cpp to be consistent. Basically everywhere we call SetupChannelOwner. > The _really_ ideal thing, of course, would be to make sure that the owner is > never null... so we always know who's responsible for the load. That's a > pretty delicate operation with security implications, though, so best done > as a separate patch from everything else. Yes, I agree, that would be ideal. We have to investigate closely. In general, we have to revisit all the callsites and make sure we hand the right/correct principal to NS_NewChannel.
Comment 51•10 years ago
|
||
> To be consistent I also did this in imgLoader.cpp and also in nsObjectLoadingContent.cpp
I would think the principal is never null in nsObjectLoadingContent.
Assignee | ||
Comment 52•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #51) > > To be consistent I also did this in imgLoader.cpp and also in nsObjectLoadingContent.cpp > > I would think the principal is never null in nsObjectLoadingContent. So far I haven't encountered a nullptr as principal in nsObjectLoadingContent when running mochis. Rather be on the safe side though.
Comment 53•10 years ago
|
||
> So far I haven't encountered a nullptr as principal in nsObjectLoadingContent
Right, because it uses thisContent->NodePrincipal(), which is guaranteed to never ever return null.
Assignee | ||
Comment 54•10 years ago
|
||
Boris, any particular reason you are creating the LoadInfo using 'new' instead of calling 'do_CreateInstance'? It seems that is causing linker problems for compiled code tests. Would you mind if I change to use 'do_CreateInstance' and call a init() right after?
Comment 55•10 years ago
|
||
> any particular reason you are creating the LoadInfo using 'new' instead of calling > 'do_CreateInstance' Yes, because it's way saner and simpler to read and doesn't obfuscate what's going on. It's also much faster. And doesn't require LoadInfo to be an XPCOM object inheriting from nsISupports and all that gunk. > It seems that is causing linker problems for compiled code tests These are test not linked into libxul? Why are they trying to create LoadInfo, exactly? Are they just calling NS_NewChannel, basically? > Would you mind if I change to use 'do_CreateInstance' Yes. If we just need to export a factory function from libxul for use by these tests, we can do that, but that doesn't involve setting this thing up to be an XPCOM object.
Assignee | ||
Comment 56•10 years ago
|
||
Patrick, for this project we are going to change NS_NewChannel so that callsites either have to provide a RequestingNode or a RequestingPrincipal which then gets set in the LoadInfo and stored on every channel. In my opinion it would be the cleanest solution to have two NS_NewChannel functions which internally call a 'common' function so that we do not have to duplicate code for the actual creation of the channel through the ioService. I don't want that common function to be visible/accessible from anything outside nsNetUtil. As discussed with Jonas, we would preferably only have two functions accessible for counsumers to create a channel, because we are worried that consumers accidentally call the wrong function. So here is the actual question: Can we move the various functions to create a channel in nsNetUtil.h (NewChannel, NS_OpenURI, NS_NewStreamLoader) to the *.cpp file? I see that the are all declared inline, so I am assuming that is the reason why they are in the *.h file, right? Is there any other reason or would it be feasible to move them to the *.cpp file?
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 57•10 years ago
|
||
Attachment #8465026 -
Attachment is obsolete: true
Attachment #8465027 -
Attachment is obsolete: true
Attachment #8465028 -
Attachment is obsolete: true
Attachment #8465029 -
Attachment is obsolete: true
Assignee | ||
Comment 58•10 years ago
|
||
Assignee | ||
Comment 59•10 years ago
|
||
Assignee | ||
Comment 60•10 years ago
|
||
Assignee | ||
Comment 61•10 years ago
|
||
Assignee | ||
Comment 62•10 years ago
|
||
Assignee | ||
Comment 63•10 years ago
|
||
Assignee | ||
Comment 64•10 years ago
|
||
Assignee | ||
Comment 65•10 years ago
|
||
Assignee | ||
Comment 66•10 years ago
|
||
Assignee | ||
Comment 67•10 years ago
|
||
Assignee | ||
Comment 68•10 years ago
|
||
Assignee | ||
Comment 69•10 years ago
|
||
Assignee | ||
Comment 70•10 years ago
|
||
Jonas, these patches extend nsNetutil.h to have 3 functions available for creating new channels: a) takes a reqeusting nsINode (where we then query the principal from that node) b) takes a principal (in case no node is available; node ends up being a nullptr in the loadinfo) 3) NS_NewChannelInternal in general should only be called from within a) or b), with the following exceptions: * nsDocShell.cpp * txMozillaStylesheetCompiler.cpp * nsXBLService.cpp * Loader.cpp * nsCrossSiteListenerProxy.cpp * nsXMLHttpRequest.cpp * EventSource.cpp * imgLoader.cpp where the Node's principal and the requesting principal are not identical. This happens for example in an XML-request where the requesting principal would be the systemPrincipal, but the channel shouldn't get the systemPrincipal. See current codebase: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1969 Further, we separated callsite updates in different patches (3a-3i). Please keep in mind that 3 files: * nsObjectLoadingContent.cpp * nsDocShell.cpp * imgLoader.cpp currently use "SetupChannelOwner" which is modified and renamed to "ShouldInheritPrincipal" in patch 4. Finally, we have to split GetChannelPrincipal into GetChannelResultPrincipal and GetChannelURIPrincipal (patch 5). http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#322 Jonas, asking your for feedback on the patches, but don't want to flag all of the patches. Just setting the NeedInfo flag.
Flags: needinfo?(jonas)
Assignee | ||
Comment 71•10 years ago
|
||
Regarding commented compiled code tests: Since we added the following code NS_newChannel in nsNetutil.h: + nsCOMPtr<nsILoadInfo> loadInfo = + new mozilla::LoadInfo(aRequestingPrincipal, + aRequestingNode, + aInheritPrincipal, + aSandboxed, + aContentPolicyType); + channel->SetLoadInfo(loadInfo); we are facing the following linker error for TestCSPParser.cpp (and also for other compiled code tests): > 0:04.06 Executing: /usr/bin/ccache clang++ -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -I/home/ckerschb/moz/mc/build/unix/headers -pthread -pipe -DDEBUG -DTRACING -g -fno-omit-frame-pointer -o TestCSPParser /home/ckerschb/moz/mc-obj-dbg/content/base/test/tmpHl1n0G.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B ../../../build/unix/gold -Wl,-rpath-link,/home/ckerschb/moz/mc-obj-dbg/dist/bin -Wl,-rpath-link,/usr/local/lib -Wl,--whole-archive ../../../dist/lib/libmozglue.a ../../../dist/lib/libmemory.a -Wl,--no-whole-archive -rdynamic ../../../xpcom/glue/libxpcomglue_s.a ../../../memory/mozalloc/libmozalloc.so ../../../nsprpub/lib/ds/libplds4.so ../../../nsprpub/lib/libc/src/libplc4.so ../../../nsprpub/pr/src/libnspr4.so ../../../toolkit/library/libxul.so -ldl > 0:04.06 /home/ckerschb/moz/mc-obj-dbg/content/base/test/tmpHl1n0G.list: > 0:04.06 INPUT("TestCSPParser.o") > 0:04.06 > 0:04.06 ../../../dist/include/nsNetUtil.h:256: error: undefined reference to 'mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)' > 0:04.06 clang: error: linker command failed with exit code 1 (use -v to see invocation) > 0:04.07 make[2]: *** [TestCSPParser] Error 1 > 0:04.07 make[1]: *** [content/base/test/target] Error 2 > 0:04.07 make[1]: *** Waiting for unfinished jobs.... Surprising, because libxul holds a function with exactly that signature: > ckerschb@ckerschb-optiplex:~/moz/mc-obj-dbg/dist/bin$ nm -C libxul.so | grep "mozilla::LoadInfo::LoadInfo(" > 00000000043defd0 t mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int) > 00000000043defd0 t mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int) Probably the symbols are not exported correctly. I tried to statically link the docshell_base by adding it to USE_LIBS in the moz.build, but that's just the tip of the iceberg, which results in many other linker errors: > USE_LIBS += [ >+ 'docshell_base', > 'mozalloc', > 'nspr', > 'xpcomglue_s', > 'xul', > ] Since other compiled code tests are also commented out, and also that comment here: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/moz.build#37 make me believe there is not an easy fix for this problem. Any suggestions are greatly appreciated!
Comment 72•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #56) > So here is the actual question: Can we move the various functions to create > a channel in nsNetUtil.h (NewChannel, NS_OpenURI, NS_NewStreamLoader) to the > *.cpp file? I see that the are all declared inline, so I am assuming that is > the reason why they are in the *.h file, right? Is there any other reason or > would it be feasible to move them to the *.cpp file? I don't know the historical answers to your question. Looking forward, I'm fine with your suggestion - though the linking in there can be a mess at times, not unlike the string classes. I see you've found that with comment 71.
Flags: needinfo?(mcmanus)
Comment on attachment 8470155 [details] [diff] [review] 1_loadinfo_changes.patch Review of attachment 8470155 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/LoadInfo.cpp @@ +70,5 @@ > +LoadInfo::GetLoadingContext(nsISupports** outLoadingContext) > +{ > + nsCOMPtr<nsISupports> context = do_QueryReferent(mLoadingContext); > + if (context) { > + NS_ADDREF(*outLoadingContext = context); This has the problem that you'll do two addrefs and one release whenever this function is called. That's excessive. And it has the problem that outLoadingContext isn't set to null if the QI fails. A better solution is: context.forget(outLoadingContext) Or even better, skip the nsCOMPtr entirely and simply do CallQueryInterface(mLoadingContext, outLoadingContext) Or, why have the QI at all? You can always cast to nsISupports: NS_IF_ADDREF(*outLoadingContext = mLoadingContext); ::: docshell/base/nsILoadInfo.idl @@ +57,5 @@ > * use loadingPrincipal for its principal, even when the data is loaded over > * http:// or another protocol that would normally use a URI-based principal. > * This attribute will never be true when loadingSandboxed is true. > */ > + readonly attribute unsigned long forceInheritPrincipal; Why this change? Having boolean accessor functions that can get at the individual flags still seems like a good thing, even if we make the constructor take a bitmask in the future. Generally speaking, I think the old setup resulted in better code. At some point we should switch the constructor from taking multiple arguments to taking a bitmask, but I don't think this moves us closer to that approach. Even once we take a bitmask it still makes sense to have easy-to-use C++ functions for getting at the individual bits. The only thing that will change if/when we move to a bitmask is the constructor signature, as well as the internal storage. @@ +70,5 @@ > + /** > + * The contentPolicyType of the channel, used for security checks > + * like MCB, and CSP. > + */ > + readonly attribute nsContentPolicyType contentPolicyType; Can you mark this as [infallible] to create a cleaner signature for C++ callers? ::: dom/src/jsurl/nsJSProtocolHandler.cpp @@ +159,5 @@ > + rv = aChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > + NS_ENSURE_SUCCESS(rv, rv); > + if (loadInfo) { > + InheritType inheritPrincipal = nsILoadInfo::eDontInheritPrincipal; > + rv = loadInfo->GetForceInheritPrincipal(&inheritPrincipal); This is a good example of why it's useful to still have a GetForceIneritPrincipal with the old signature.
Comment on attachment 8470159 [details] [diff] [review] 3_c_callsites_in_netwerk.patch Review of attachment 8470159 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/test/TestPageLoad.cpp @@ +304,5 @@ > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); > + RETURN_IF_FAILED(rv, rv, "Couldn't get script security manager!"); > + nsCOMPtr<nsIPrincipal> systemPrincipal; > + rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal)); > + RETURN_IF_FAILED(rv, rv, "Couldn't get system principal!"); Why can't you use nsContentUtils::GetSystemPrincipal here? Doesn't matter much though given that this is just a text file. But there's always a risk that people will copy this pattern into real code. ::: netwerk/test/TestProtocols.cpp @@ +635,5 @@ > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsIPrincipal> systemPrincipal; > + rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal)); > + NS_ENSURE_SUCCESS(rv, rv); Same here. Generally speaking I think we can use nsContentUtils::GetSystemPrincipal pretty much everywhere. ::: netwerk/test/moz.build @@ +41,4 @@ > # TestSocketTransport', > # TestStreamChannel', > # TestStreamPump', > +# TestStreamLoader', Why these changes?
Assignee | ||
Comment 75•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #74) > ::: netwerk/test/TestPageLoad.cpp > @@ +304,5 @@ > > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); > > + RETURN_IF_FAILED(rv, rv, "Couldn't get script security manager!"); > > + nsCOMPtr<nsIPrincipal> systemPrincipal; > > + rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal)); > > + RETURN_IF_FAILED(rv, rv, "Couldn't get system principal!"); > > Why can't you use nsContentUtils::GetSystemPrincipal here? Doesn't matter > much though given that this is just a text file. But there's always a risk > that people will copy this pattern into real code. > ::: netwerk/test/TestProtocols.cpp > @@ +635,5 @@ > > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); > > + NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsIPrincipal> systemPrincipal; > > + rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal)); > > + NS_ENSURE_SUCCESS(rv, rv); > > Same here. Generally speaking I think we can use > nsContentUtils::GetSystemPrincipal pretty much everywhere. Generally, I use nsContentUtils::GetSystemPrincipal everywhere in the code, besides in compiled code tests, because all compiled code tests get the systemPrincipal from the securityManager. Presumably it has to do with linking problems. > ::: netwerk/test/moz.build > @@ +41,4 @@ > > # TestSocketTransport', > > # TestStreamChannel', > > # TestStreamPump', > > +# TestStreamLoader', > > Why these changes? Linker problems; see detailed explanation of the problem here: https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c71
Comment on attachment 8470167 [details] [diff] [review] 4_setupChannelOwner_changes.patch Review of attachment 8470167 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsContentUtils.cpp @@ +6499,5 @@ > } > > // static > bool > +nsContentUtils::ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal, It sounds weird that this function takes a aForceInherit boolean. I would expect a function called ChannelShouldInheritPrincipal to always set forceInherit. Also, Boris definitely needs to review the changes here.
Assignee | ||
Comment 77•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #73) > ::: docshell/base/LoadInfo.cpp > NS_IF_ADDREF(*outLoadingContext = mLoadingContext); Indeed. Initially I tried to make everything work passing an nsINode around instead of using nsISupports. In which case we would have needed to query the referent. Anyway, I updated the part of the code. > @@ +70,5 @@ > > + /** > > + * The contentPolicyType of the channel, used for security checks > > + * like MCB, and CSP. > > + */ > > + readonly attribute nsContentPolicyType contentPolicyType; > > Can you mark this as [infallible] to create a cleaner signature for C++ > callers? Unfortunately not: error: [infallible] only works on builtin types (numbers, booleans, and raw char types), ../../../dist/idl/nsILoadInfo.idl line 55:25 0:07.71 [infallible] readonly attribute nsContentPolicyType contentPolicyType;
Assignee | ||
Comment 78•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #76) > Comment on attachment 8470167 [details] [diff] [review] > 4_setupChannelOwner_changes.patch > > Review of attachment 8470167 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/src/nsContentUtils.cpp > @@ +6499,5 @@ > > } > > > > // static > > bool > > +nsContentUtils::ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal, > > It sounds weird that this function takes a aForceInherit boolean. I would > expect a function called ChannelShouldInheritPrincipal to always set > forceInherit. > > Also, Boris definitely needs to review the changes here. Indeed, Boris needs to review that part - I didn't want to change too much of his code re-used from 'SetupChannelOwner.
Comment 79•10 years ago
|
||
Comment on attachment 8470157 [details] [diff] [review] 3_a_callsites_in_content.patch * content/base/src/ImportManager.cpp Question for reviewer - Are mImportParent->MasterDocument()'s ScriptObjectPrincipal and mImportParent->NodePrincipal the same? If they are, we can call NS_NewChannel with mImportParent as the requestingNode. If they aren't, we need to instead of calling NS_NewChannelInternal and pass both of them. * I think we need to use NS_NewChannelInternal() in nsHTMLMediaElement.cpp and nsHTMLTrackElement.cpp * In nsHTMLDocument.cpp :: Open(), we should use callerDoc and call NS_NewChannel() with a node instead of a principal. * In nsHTMLDocument.cpp :: CreateAndAddWyciwygChannel(), can we also get a callerDoc with nsCOMPtr<nsIDocument> callerDoc = nsContentUtils::GetDocumentFromContext(); Is NodePrincipal() really the loadingPrincipal, or is it just the Principal? * In content/media/MediaResource.cpp, element is the media element itself, not the requestingContext. We need to ask reviewers what the loadInfo should be here. * content/xul/document/src/XULDocument.cpp - the XULDocument has an mChannel. In LoadOverlayInternal we create a new channel. Can we use the loadInfo from mChannel to populate the new channel? Also, in XULDocument::LoadScript(), we call nsScriptLoader::ShouldLoadScript() and pass in "this" for the requestingNode. This calls content policies. Later we call, NS_NewStreamLoader(). We should set the requestingNode to this instead of using system principal. Note that we currently can't do this, because NS_NewStreamLoader() takes a principal but not a node.
Comment 80•10 years ago
|
||
> Can we move the various functions to create > a channel in nsNetUtil.h (NewChannel, NS_OpenURI, NS_NewStreamLoader) to the > *.cpp file? I see that the are all declared inline, so I am assuming that is > the reason why they are in the *.h file, right? Is there any other reason or > would it be feasible to move them to the *.cpp file? Historically everything was in the .h file to avoid external programs (tests, etc) not being able to see the symbols in a .o file (this was back when we enforced IDL-only linkage between modules). We've got (bitrotted) patches for splitting up nsNetUtil.h into .h/.cpp files in bug 905127 but it got stalled on some linker issues (which may be resolved: we need to unbitrot and see). For now I suggest you just dump stuff into the .h file and we'll worry about moving large functions into a .cpp file as part of that bug, not this one (you have plenty to do :)
Assignee | ||
Comment 81•10 years ago
|
||
As requested by smaug for review; combining all the patches into one massiv patch. Once we got smaugs review, we can split them again for the individual reviewers.
Attachment #8470155 -
Attachment is obsolete: true
Attachment #8470156 -
Attachment is obsolete: true
Attachment #8470157 -
Attachment is obsolete: true
Attachment #8470158 -
Attachment is obsolete: true
Attachment #8470159 -
Attachment is obsolete: true
Attachment #8470160 -
Attachment is obsolete: true
Attachment #8470161 -
Attachment is obsolete: true
Attachment #8470162 -
Attachment is obsolete: true
Attachment #8470163 -
Attachment is obsolete: true
Attachment #8470165 -
Attachment is obsolete: true
Attachment #8470166 -
Attachment is obsolete: true
Attachment #8470167 -
Attachment is obsolete: true
Attachment #8470169 -
Attachment is obsolete: true
Attachment #8472737 -
Flags: review?(bugs)
Flags: needinfo?(jonas)
Comment 82•10 years ago
|
||
Comment on attachment 8470157 [details] [diff] [review] 3_a_callsites_in_content.patch Commenting on patch 3_b for callsites in /dom (even though the patch has been consolidated with the other code for now). * dom/base/Navigator.cpp :: MozIsLocallyAvailable() Looks like we have a doc here. Why do we use system principal? (Note that I can't find any callers to this function anyway. It's in an idl, so maybe addons call it.) * dom/plugins/base/nsPluginHost.cpp Looks like we use element in the call to content policies and we use the documents principal. Perhaps we should use NS_NewChannelInternal() with the element and doc->NodePrincipal() instead of NS_NewChannel with a potentially null doc. (Note in overall revamp code, we have the following comment: + doc); // TODO, doc might be null here!!! ) * dom/plugins/base/nsPluginStreamListenerPeer.cpp We use systemPrincipal here, but perhaps we can get the doc instead by doing something like: nsRefPtr<nsPluginInstanceOwner> owner = mPluginInstance->GetOwner(); if (owner) { nsCOMPtr<nsIDocument> doc; nsresult rv = owner->GetDocument(getter_AddRefs(doc)); } * dom/workers/ScriptLoader.cpp Are parentDoc's principal and "principal" the same here? If they are potentially different, we should call NS_NewChannelInternal and pass both. * dom/xbl/nsXBLService.cpp Expand the comment a bit // FetchBindingDocument(). LoadInfo will end up with no principal or node in those cases, so we use systemPrincipal. // This achieves the same result of bypassing security checks, but it gives the wrong information to potential future consumers of loadInfo. * dom/xml/XMLDocument.cpp See this comment: https://mxr.mozilla.org/mozilla-central/source/dom/xml/XMLDocument.cpp#323 principal appears to be the principal of the XMLDocument being loaded. callingDoc is the loadingContext. In the call to content policies, we use callingDoc as the context and principal as the requestingPrincipal. Why? These changes were made by Jonas in bug 490790 so maybe he knows why. This may be a mistake where we send the principal instead of the requestingPrincipal. Moreover, why do we fallback to using this (the currently loading doc) if callingDoc doesn't exist? Needinfo'ing Jonas just for this question. * dom/xslt/base/txURIUtils.cpp Should we use TYPE_XSLT here instead of TYPE_OTHER?
Flags: needinfo?(jonas)
Comment 83•10 years ago
|
||
Comment on attachment 8470159 [details] [diff] [review] 3_c_callsites_in_netwerk.patch Commenting on patch 3_b for callsites in /netwerk In /netwerk/protocol/http/HttpChannelParent.cpp, we have DoAsyncOpen() that takes a bunch of parameters (https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#157). The parameters come from HttpChannelOpenArgs, which are set by the HttpChannelChild::AsyncOpen() (https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1235). Can we use "this" in HttpChannelChild::AsyncOpen to get the loadInfo of the current channel, set that in HttpChannelOpenArgs, and then use it in HttpChannelParent::DoAsyncOpen()'s call to NS_NewChannel instead of passing systemPrincipal? Similarly, for netwerk/protocol/file/FTPChannelParent.cpp
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment 84•10 years ago
|
||
Comment on attachment 8470161 [details] [diff] [review] 3_e_callsites_in_layout.patch diff --git a/layout/mathml/nsMathMLChar.cpp b/layout/mathml/nsMathMLChar.cpp --- a/layout/mathml/nsMathMLChar.cpp +++ b/layout/mathml/nsMathMLChar.cpp @@ -145,18 +146,23 @@ static nsresult LoadProperties(const nsString& aName, nsCOMPtr<nsIPersistentProperties>& aProperties) { nsAutoString uriStr; uriStr.AssignLiteral("resource://gre/res/fonts/mathfont"); uriStr.Append(aName); uriStr.StripWhitespace(); // that may come from aName uriStr.AppendLiteral(".properties"); - return NS_LoadPersistentPropertiesFromURISpec(getter_AddRefs(aProperties), - NS_ConvertUTF16toUTF8(uriStr)); + return NS_LoadPersistentPropertiesFromURISpec(NS_ConvertUTF16toUTF8(uriStr), + nullptr, // aCharset + nullptr, // aBaseURI + nullptr, // aIoService + nsContentUtils::GetSystemPrincipal(), + nsIContentPolicy::TYPE_OTHER, + getter_AddRefs(aProperties)) Would this be considered TYPE_FONT? In layout/style/Loader.cpp, content policies are called with CheckLoadAllowed(), which takes a requestingPrincipal and requestingContext. NS_NewChannel() is called by LoadSheet(). Callers of LoadSheet() (like LoadStyleLink(), LoadChildSheet(), and InternalLoadNonDocumentSheet()) call CheckLoadAllowed() and then call LoadSheet(). When we call NS_NewChannel(), we set mDocument to be the requestingNode. But calls to CheckLoadAllowed() try to set the requestingContext to something else, and then fallback to the document (examples in LoadStyleLink(), LoadChildSheet()). Hence, we should add a third parameter to LoadSheet() that includes the requestingContext, and then use that in the call to NS_Newchannel. Alternatively, we could add the requestingContext to the first parameter of LoadSheet() - SheetLoadData (https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#106).
Comment 85•10 years ago
|
||
Comment on attachment 8470161 [details] [diff] [review] 3_e_callsites_in_layout.patch diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp --- a/layout/style/Loader.cpp +++ b/layout/style/Loader.cpp @@ -1420,19 +1420,28 @@ Loader::LoadSheet(SheetLoadData* aLoadDa mozilla::net::PredictorLearn(aLoadData->mURI, mDocument->GetDocumentURI(), nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE, mDocument); } // Just load it nsCOMPtr<nsIInputStream> stream; nsCOMPtr<nsIChannel> channel; - rv = NS_OpenURI(getter_AddRefs(stream), aLoadData->mURI, nullptr, - nullptr, nullptr, nsIRequest::LOAD_NORMAL, - getter_AddRefs(channel)); + rv = NS_OpenURI(aLoadData->mURI, + nullptr, // aIoService + nullptr, // aLoadGroup + nullptr, // aCallbacks + nsIRequest::LOAD_NORMAL, + nsContentUtils::GetSystemPrincipal(), + nsILoadInfo::eDontInheritPrincipal, + nsILoadInfo::eNotSandboxed, + nsIContentPolicy::TYPE_OTHER, + getter_AddRefs(channel), // outChannel + getter_AddRefs(stream)); + For NS_OpenURI, we should use aLoadData->mLoaderPrincipal instead of SystemPrincipal and we should also pass in the requestingContext from the LoadSheet() parameters (see the previous comment). This means we need an "NS_OpenURIInternal()" that takes both a node and a principal. We couldn't use an "NS_OpenURINode()" because the NodePrincipal and aLoadData->mLoaderPrincipal may be different (ex: one may be the principal of the stylesheet while the other is the principal of the document, but I'm a little fuzzy on how this works).
Comment 86•10 years ago
|
||
Comment on attachment 8470161 [details] [diff] [review] 3_e_callsites_in_layout.patch For layout/style/nsFontFaceLoader.cpp, it looks like the csp for channelPolicy is taken off aProxy->mPrincipal and aFontToLoad->mPrincipal, respectively. Should we pass these in as requestingPrincipals?
Comment 87•10 years ago
|
||
Comment on attachment 8470162 [details] [diff] [review] 3_f_callsites_in_widget.patch widget/windows/WinUtils.cpp :: AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void) We are calling NS_NewChannel() with TYPE_OTHER and systemPrincipal. Assuming this is using a generic favicon and no network request is being made, systemPrincipal is okay, but we should change to TYPE_IMAGE. widget/windows/nsDataObj.cpp Not sure what this code does. The reviewer for the /widget callsites will have to look carefully at this one to check its use of systemPrincipal.
Assignee | ||
Comment 88•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #71) > > 0:04.06 ../../../dist/include/nsNetUtil.h:256: error: undefined reference to 'mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)' > > 0:04.06 clang: error: linker command failed with exit code 1 (use -v to see invocation) > > 0:04.07 make[2]: *** [TestCSPParser] Error 1 > > 0:04.07 make[1]: *** [content/base/test/target] Error 2 > > 0:04.07 make[1]: *** Waiting for unfinished jobs.... Getting help from people in #build, it seems we have three options to solve the linker problem here: a) We convert the 5 failing tests (TestOpen, TestPageLoad, TestProtocols, TestStreamLoader, TestCSPParser) to gtests, which seems like a lot of unecessary work at the moment, see: https://developer.mozilla.org/en-US/docs/GTest b) We export the symbols for LoadInfo, which no one is really in favor of. c) We call do_CreateInstance in NSNetutil.h if MOZILLA_INTERNAL_API is not defined, so that production code creates a LoadInfo calling 'new', and compiled code tests create the Loadinfo through do_createInstance. I incorporated changes for (c), which seems the best option for me at the moment. If we can come up with a better solution, I am happy to incorporate those changes.
b) sounds better than c) to me. But I'm fine with leaving this up to bz.
Comment 90•10 years ago
|
||
Comment on attachment 8472737 [details] [diff] [review] bug_1038756_combined_patch_for_smaug.patch >@@ -194,17 +194,23 @@ interface nsIScriptSecurityManager : nsI > void checkSameOriginURI(in nsIURI aSourceURI, > in nsIURI aTargetURI, > in boolean reportError); > /** > * Get the principal for the given channel. This will typically be the > * channel owner if there is one, and the codebase principal for the. > * channel's URI otherwise. aChannel must not be null. > */ >- nsIPrincipal getChannelPrincipal(in nsIChannel aChannel); >+ nsIPrincipal getChannelResultPrincipal(in nsIChannel aChannel); >+ >+ /** >+ * Get the codebase principal for the channel's URI. >+ * aChannel must not be null. >+ */ >+ nsIPrincipal getChannelURIPrincipal(in nsIChannel aChannel); This naming could be better. Otherwise getChannelURIPrincipal will be used when getChannelResultPrincipal should be. Because of loadcontext usage, it isn't really principal from the URI, but principal from the channel + uri. I think I would keep getChannelPrincipal as such, and then getCodebasePrincipalFromChannelURI() (that is long and terrible enough name for people to avoid it ;) ) Or something like that. I know jonas suggested the naming in the patch, but IMO it is just a bit too error prone. Could be because of my non-native-English. still looking at the rest...
Comment 91•10 years ago
|
||
Or perhaps getChannelResultPrincipal is ok too.
Doesn't "getChannelURIPrincipal" indicate that the principal is the result of both URI and channel? Both parts are right there in the name. The goal is to eventually make getChannelURIPrincipal return a system principal whenever a chrome URI is loaded, or whenever about:config is loaded. So I'd like to avoid getCodebasePrincipalFromChannelURI.
Flags: needinfo?(jonas)
Comment 93•10 years ago
|
||
Comment on attachment 8472737 [details] [diff] [review] bug_1038756_combined_patch_for_smaug.patch >@@ -2005,22 +2005,20 @@ public: > * the principal no matter what, as long as the principal is not null. > * > * If aIsSandboxed is true, then aLoadingPrincipal must not be null. In this > * case, the owner on the channel, if any, will be reset to null and the > * nsILoadInfo will say the channel should be sandboxed. > * > * The return value is whether the channel was told to inherit the principal. > */ >- static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal, >- nsIChannel* aChannel, >- nsIURI* aURI, >- bool aInheritForAboutBlank, >- bool aIsSandboxed, >- bool aForceInherit); >+ static bool ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal, >+ nsIURI* aURI, >+ bool aInheritForAboutBlank, >+ bool aForceInherit); I'm missing something here. The new name talks about Channel, yet there is no Channel around. And the comment for the method still talks about aChannel. >+typedef unsigned long InheritType; >+typedef unsigned long SandboxType; Could you please not do this. The exiting enums are there to force right usage which improves code readability . At least in C++ when LoadInfo is create we should pass enum values. > /** >+ * The loadingContext of that channel, used for security checks >+ * like MCB, and CSP. >+ * Warning: The loadingContext can be null! >+ */ >+ readonly attribute nsISupports loadingContext; This needs better documentation (to prevent the mess we have with nsIContentPolicy) But actually, is this ever something else than a node? If not, why not use nsIDOMNode here? >+++ b/dom/base/Navigator.cpp >@@ -957,18 +957,28 @@ Navigator::MozIsLocallyAvailable(const n > > nsCOMPtr<nsILoadGroup> loadGroup; > nsCOMPtr<nsIDocument> doc = mWindow->GetDoc(); > if (doc) { > loadGroup = doc->GetDocumentLoadGroup(); > } > > nsCOMPtr<nsIChannel> channel; >- rv = NS_NewChannel(getter_AddRefs(channel), uri, >- nullptr, loadGroup, nullptr, loadFlags); >+ rv = NS_NewChannel(uri, >+ nullptr, // aIoService >+ loadGroup, >+ nullptr, // aCallbacks >+ loadFlags, >+ nullptr, // aChannelPolicy >+ nsContentUtils::GetSystemPrincipal(), Why SystemPrincipal? >+++ b/dom/plugins/base/nsPluginStreamListenerPeer.cpp >@@ -637,17 +637,28 @@ nsPluginStreamListenerPeer::RequestRead( > if (numRequests == 0) > return NS_ERROR_FAILURE; > > nsresult rv = NS_OK; > > nsCOMPtr<nsIInterfaceRequestor> callbacks = do_QueryReferent(mWeakPtrChannelCallbacks); > nsCOMPtr<nsILoadGroup> loadGroup = do_QueryReferent(mWeakPtrChannelLoadGroup); > nsCOMPtr<nsIChannel> channel; >- rv = NS_NewChannel(getter_AddRefs(channel), mURL, nullptr, loadGroup, callbacks); >+ rv = NS_NewChannel(mURL, >+ nullptr, // aIoService >+ loadGroup, >+ callbacks, >+ nsIRequest::LOAD_NORMAL, >+ nullptr, // aChannelPolicy >+ nsContentUtils::GetSystemPrincipal(), Really? >+/* >+ * How to create a new Channel using NS_NewChannel: >+ * 1) Please try to call NS_NewChannel providing a requesting *nsINode* >+ * 2) If no requesting nsINode is available, >+ * call NS_NewChannel providing a requesting *nsIPrincipal*. >+ * 3) Call NS_NewChannelInternal *only* if requesting Principal and >+ * the Node's Principal have to be different. >+ * >> Most likely this is not the case! << >+ * Needs special approval! NS_NewChannelInternal should >+ * only be called from within nsNetutil.h. Yet you call NS_NewChannelInternal in many other place. Rather odd.
Attachment #8472737 -
Flags: review?(bugs) → feedback-
Comment 94•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #92) > Doesn't "getChannelURIPrincipal" indicate that the principal is the result > of both URI and channel? Both parts are right there in the name. The main issue is that at least to me getChannelURIPrincipal vs getChannelResultPrincipal doesn't tell clearly enough what the difference is. What is ChannelURI and what is ChannelResult. Or one can read that Channel's URI's Principal vs Channel's Result's Principal etc. So I'm worried that wrong principal will be used accidentally, if the method names aren't clear enough.
I'm definitely open for other suggestions. The two functions that we have are * One function returns the principal that the "server" returning a response has. This is only affected by the URI that we're loading from as well as the "data jar" (i.e. app-id etc). * One function returns the principal that we should use for the resource returned by the channel. So it's affected by things like sandbox flags, "force inherit" flags (set when doing XHR CORS loads) and probably other types of context in the future. I definitely agree that we should have clear names. The current ones were the best I could think of.
Assignee | ||
Comment 96•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #79) > Comment on attachment 8470157 [details] [diff] [review] > * I think we need to use NS_NewChannelInternal() in nsHTMLMediaElement.cpp > and nsHTMLTrackElement.cpp Yes, that makes sense, incorporated that change. > * In nsHTMLDocument.cpp :: Open(), we should use callerDoc and call > NS_NewChannel() with a node instead of a principal. I agree here as well - done. I leave the other questions from Comment 79 for reviewers.
Assignee | ||
Comment 97•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #82) > Commenting on patch 3_b for callsites in /dom (even though the patch has > been consolidated with the other code for now). > > * dom/base/Navigator.cpp :: MozIsLocallyAvailable() > Looks like we have a doc here. Why do we use system principal? (Note that I > can't find any callers to this function anyway. It's in an idl, so maybe > addons call it.) Agreed, should be the doc. > * dom/plugins/base/nsPluginHost.cpp > Looks like we use element in the call to content policies and we use the > documents principal. Perhaps we should use NS_NewChannelInternal() with the > element and doc->NodePrincipal() instead of NS_NewChannel with a potentially > null doc. > (Note in overall revamp code, we have the following comment: > + doc); // TODO, doc might be null here!!! > ) Yes, we can use doc->NodePrincipal, but if there is no doc, we should use a nullPrincipal as the principal, otherwise we hit assertions because every loadInfo needs a principal. I incorporated that change, makes sense to me. > * dom/plugins/base/nsPluginStreamListenerPeer.cpp > We use systemPrincipal here, but perhaps we can get the doc instead by doing > something like: > nsRefPtr<nsPluginInstanceOwner> owner = mPluginInstance->GetOwner(); > if (owner) { > nsCOMPtr<nsIDocument> doc; > nsresult rv = owner->GetDocument(getter_AddRefs(doc)); > } Leave that for reviewers - probably that is a good idea. > * dom/workers/ScriptLoader.cpp > Are parentDoc's principal and "principal" the same here? If they are > potentially different, we should call NS_NewChannelInternal and pass both. I think it makes sense to call NS_NewChannelInternal here. I did that. Leaving the rest of your comments/questions for individual reviewers.
Assignee | ||
Comment 98•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #84) > Comment on attachment 8470161 [details] [diff] [review] > 3_e_callsites_in_layout.patch > NS_LoadPersistentPropertiesFromURISpec(NS_ConvertUTF16toUTF8(uriStr), > + nullptr, // aCharset > + nullptr, // aBaseURI > + nullptr, // aIoService > + > nsContentUtils::GetSystemPrincipal(), > + > nsIContentPolicy::TYPE_OTHER, > > Would this be considered TYPE_FONT? Sounds right to me, incorporated that. > In layout/style/Loader.cpp, content policies are called with > CheckLoadAllowed(), which takes a requestingPrincipal and requestingContext. > NS_NewChannel() is called by LoadSheet(). Callers of LoadSheet() (like > LoadStyleLink(), LoadChildSheet(), and InternalLoadNonDocumentSheet()) call > CheckLoadAllowed() and then call LoadSheet(). > > When we call NS_NewChannel(), we set mDocument to be the requestingNode. > But calls to CheckLoadAllowed() try to set the requestingContext to > something else, and then fallback to the document (examples in > LoadStyleLink(), LoadChildSheet()). Hence, we should add a third parameter > to LoadSheet() that includes the requestingContext, and then use that in the > call to NS_Newchannel. Alternatively, we could add the requestingContext to > the first parameter of LoadSheet() - SheetLoadData > (https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#106). I will leave this one up for reviewers, but when browsing over that code in layout/style/Loader.cpp, I think that the call to NS_OpenURI should use the same principal as NS_newChannel. incorporated that change there.
Assignee | ||
Comment 99•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #85) > Comment on attachment 8470161 [details] [diff] [review] > 3_e_callsites_in_layout.patch > > diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp > --- a/layout/style/Loader.cpp > +++ b/layout/style/Loader.cpp > @@ -1420,19 +1420,28 @@ Loader::LoadSheet(SheetLoadData* aLoadDa > mozilla::net::PredictorLearn(aLoadData->mURI, > mDocument->GetDocumentURI(), > > nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE, > mDocument); > } > > // Just load it > nsCOMPtr<nsIInputStream> stream; > nsCOMPtr<nsIChannel> channel; > - rv = NS_OpenURI(getter_AddRefs(stream), aLoadData->mURI, nullptr, > - nullptr, nullptr, nsIRequest::LOAD_NORMAL, > - getter_AddRefs(channel)); > + rv = NS_OpenURI(aLoadData->mURI, > + nullptr, // aIoService > + nullptr, // aLoadGroup > + nullptr, // aCallbacks > + nsIRequest::LOAD_NORMAL, > + nsContentUtils::GetSystemPrincipal(), > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER, > + getter_AddRefs(channel), // outChannel > + getter_AddRefs(stream)); > + > > For NS_OpenURI, we should use aLoadData->mLoaderPrincipal instead of > SystemPrincipal and we should also pass in the requestingContext from the > LoadSheet() parameters (see the previous comment). This means we need an > "NS_OpenURIInternal()" that takes both a node and a principal. We couldn't > use an "NS_OpenURINode()" because the NodePrincipal and > aLoadData->mLoaderPrincipal may be different (ex: one may be the principal > of the stylesheet while the other is the principal of the document, but I'm > a little fuzzy on how this works). Oh, here we go, haven't read that comment yet. But just incorporated that change. At least we found the same problem with that part of the code.
Assignee | ||
Comment 100•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #86) > Comment on attachment 8470161 [details] [diff] [review] > 3_e_callsites_in_layout.patch > > For layout/style/nsFontFaceLoader.cpp, it looks like the csp for > channelPolicy is taken off aProxy->mPrincipal and aFontToLoad->mPrincipal, > respectively. Should we pass these in as requestingPrincipals? I think we should keep using ps->GetDocument(), which is what NS_CheckContentLoadPolicy uses as the context.
Assignee | ||
Comment 101•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #87) > widget/windows/WinUtils.cpp :: > AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void) > We are calling NS_NewChannel() with TYPE_OTHER and systemPrincipal. > Assuming this is using a generic favicon and no network request is being > made, systemPrincipal is okay, but we should change to TYPE_IMAGE. I leave the decision about the systemPrincipal do the reviewers, but TYPE_IMAGE sounds right to me. > widget/windows/nsDataObj.cpp > Not sure what this code does. The reviewer for the /widget callsites will > have to look carefully at this one to check its use of systemPrincipal. Indeed.
Assignee | ||
Comment 102•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #93) > Comment on attachment 8472737 [details] [diff] [review] > bug_1038756_combined_patch_for_smaug.patch > > >@@ -2005,22 +2005,20 @@ public: > > * the principal no matter what, as long as the principal is not null. > > * > > * If aIsSandboxed is true, then aLoadingPrincipal must not be null. In this > > * case, the owner on the channel, if any, will be reset to null and the > > * nsILoadInfo will say the channel should be sandboxed. > > * > > * The return value is whether the channel was told to inherit the principal. > > */ > >- static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal, > >- nsIChannel* aChannel, > >- nsIURI* aURI, > >- bool aInheritForAboutBlank, > >- bool aIsSandboxed, > >- bool aForceInherit); > >+ static bool ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal, > >+ nsIURI* aURI, > >+ bool aInheritForAboutBlank, > >+ bool aForceInherit); > I'm missing something here. The new name talks about Channel, yet there is > no Channel around. > And the comment for the method still talks about aChannel. We updated that comment to be more explicit about what this function is for. > >+typedef unsigned long InheritType; > >+typedef unsigned long SandboxType; > Could you please not do this. > The exiting enums are there to force right usage which improves > code readability . At least in C++ when LoadInfo is create we should pass > enum values. There is a comment right above which mentions that in bug 1041176 we collapse the flags into one unsigned long and use individual bits (similar to loadFlags). Bug 1041176 will land at the same time. > > > /** > >+ * The loadingContext of that channel, used for security checks > >+ * like MCB, and CSP. > >+ * Warning: The loadingContext can be null! > >+ */ > >+ readonly attribute nsISupports loadingContext; > This needs better documentation (to prevent the mess we have with > nsIContentPolicy) > But actually, is this ever something else than a node? If not, why not use > nsIDOMNode here? Initially I wanted to use nsINode, but that's doesn't work because there is no *.idl for nsINode, hence we fall back to nsISupports which I didn't really like because it's too generic. Using nsIDOMNode is the better solution. I incorporated that change. Exactly what I wanted it to be. We also extended the comment on top. > >+/* > >+ * How to create a new Channel using NS_NewChannel: > >+ * 1) Please try to call NS_NewChannel providing a requesting *nsINode* > >+ * 2) If no requesting nsINode is available, > >+ * call NS_NewChannel providing a requesting *nsIPrincipal*. > >+ * 3) Call NS_NewChannelInternal *only* if requesting Principal and > >+ * the Node's Principal have to be different. > >+ * >> Most likely this is not the case! << > >+ * Needs special approval! NS_NewChannelInternal should > >+ * only be called from within nsNetutil.h. > Yet you call NS_NewChannelInternal in many other place. Rather odd. We expanded that comment, but in general it means: Everyone who is going to create a channel after that initial patch landed shouldn't use NSNewChannelInternal but rather NS_NewChannel providing either a node or a principal.
Comment 103•10 years ago
|
||
> error: [infallible] only works on builtin types (numbers, booleans, and raw char types), ../../../dist/idl/nsILoadInfo.idl line 55:25
Seems like we should consider fixing that.
Or failing that manually adding a %C++ block that does the right thing.
Assignee | ||
Comment 104•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #88) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #71) > > > 0:04.06 ../../../dist/include/nsNetUtil.h:256: error: undefined reference to 'mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)' > > > 0:04.06 clang: error: linker command failed with exit code 1 (use -v to see invocation) > > > 0:04.07 make[2]: *** [TestCSPParser] Error 1 > > > 0:04.07 make[1]: *** [content/base/test/target] Error 2 > > > 0:04.07 make[1]: *** Waiting for unfinished jobs.... > > Getting help from people in #build, it seems we have three options to solve > the linker problem here: > a) We convert the 5 failing tests (TestOpen, TestPageLoad, TestProtocols, > TestStreamLoader, TestCSPParser) to gtests, which seems like a lot of > unecessary work at the moment, see: > https://developer.mozilla.org/en-US/docs/GTest > b) We export the symbols for LoadInfo, which no one is really in favor of. > c) We call do_CreateInstance in NSNetutil.h if MOZILLA_INTERNAL_API is not > defined, so that production code creates a LoadInfo calling 'new', and > compiled code tests create the Loadinfo through do_createInstance. > > I incorporated changes for (c), which seems the best option for me at the > moment. If we can come up with a better solution, I am happy to incorporate > those changes. Apparently Jonas (Comment 89) is more in favor of b). Boris, do you feel the same way? If so, can you provide some guidance how I can export those symbols so that compiled code tests link correctly? (I described the exact problem in detail Comment 71).
Flags: needinfo?(bzbarsky)
Comment 105•10 years ago
|
||
I somewhat prefer (b) as well. You can use MOZ_EXPORT to export things.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 106•10 years ago
|
||
Summary for all reviewers: Channels know very little about who initiated the request for the data they are fetching. But there are use cases where we need some loading information during the lifetime of the channel. Hence, in bug 965413, bz added a LoadInfo object that hangs off channels. This bug is to populate that LoadInfo object at channel creation time (via NS_NewChannel and friends). In this code, we pass a loadingPrincipal and/or a loadingNode to NS_NewChannel and NS_NewChannel then creates the loadInfo object with the passed in values. Please take a careful look at the code that we have flagged you for. In particular, look to see if we have identified the correct loading node and/or loading principal. We are not looking for the node and principal corresponding to the data that the channel is fetching, but the node and principal of the entity that is loading that data. These values are crucial to security checking code that is currently being reimplemented. If we pass the wrong values, we may end up allowing resource loads that should be blocked by the browser. If we use systemPrincipal, that will mean that all loads will be permitted. So please check that the load is coming from the system and that any needed security checks have already been done.
Assignee | ||
Comment 107•10 years ago
|
||
Attachment #8472737 -
Attachment is obsolete: true
Attachment #8478590 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 108•10 years ago
|
||
Since we are going to change the argument list for NS_NewChannel, NS_OpenURI and NS_NewStreamLoader I think it's also good timing to polish the code within those function bodies a little bit.
Attachment #8478591 -
Flags: review?(mcmanus)
Assignee | ||
Comment 109•10 years ago
|
||
In nsHTMLDocument.cpp :: CreateAndAddWyciwygChannel(), can we get a callerDoc with nsCOMPtr<nsIDocument> callerDoc = nsContentUtils::GetDocumentFromContext(); Also, is NodePrincipal() really the loadingPrincipal, or is it just the Principal?
Attachment #8478592 -
Flags: review?(jst)
Assignee | ||
Comment 110•10 years ago
|
||
Are mImportParent->MasterDocument()'s ScriptObjectPrincipal and mImportParent->NodePrincipal the same? If they are, we can call NS_NewChannel with mImportParent as the requestingNode. If they aren't, we need to instead of calling NS_NewChannelInternal and pass both of them.
Attachment #8478594 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 111•10 years ago
|
||
content/xul/document/src/XULDocument.cpp - the XULDocument has an mChannel. In LoadOverlayInternal we create a new channel. Can we use the loadInfo from mChannel to populate the new channel?
Attachment #8478595 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 112•10 years ago
|
||
In content/media/MediaResource.cpp, element is the media element itself, not the requestingContext. What should the loadInfo should be here - do we have the document that initiated the media load and the loading principal?
Attachment #8478596 -
Flags: review?(roc)
Assignee | ||
Comment 113•10 years ago
|
||
dom/plugins/base/nsPluginStreamListenerPeer.cpp - We try and get the loading document here by doign the following. Is that correct? Does this give us the loading doc? nsRefPtr<nsPluginInstanceOwner> owner = mPluginInstance->GetOwner(); if (owner) { nsCOMPtr<nsIDocument> doc; nsresult rv = owner->GetDocument(getter_AddRefs(doc)); } ** dom/xslt/base/txURIUtils.cpp Should we use TYPE_XSLT here instead of TYPE_OTHER?
Attachment #8478597 -
Flags: review?(jst)
Assignee | ||
Comment 114•10 years ago
|
||
Attachment #8478599 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 115•10 years ago
|
||
Wait to request review on this one until we have the parent/child httpchannel args code done. We use #undef METER because otherwise we get compile errors in nsIncrementalDownload, having an 'already defined error' of METER, because it's defined in nsBufferedStreams.cpp as well as HashTable.h, where Hashtable undefs it, at the end, see: http://mxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#1681 We should also undef it in nsBufferedStreams.cpp.
Attachment #8478600 -
Flags: review?(mcmanus)
Assignee | ||
Comment 116•10 years ago
|
||
Attachment #8478601 -
Flags: review?(mcmanus)
Assignee | ||
Comment 117•10 years ago
|
||
Attachment #8478602 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 118•10 years ago
|
||
Attachment #8478603 -
Flags: review?(roc)
Assignee | ||
Comment 119•10 years ago
|
||
**In layout/style/Loader.cpp, content policies are called with CheckLoadAllowed(), which takes a requestingPrincipal and requestingContext. NS_NewChannel() is called by LoadSheet(). Callers of LoadSheet() (like LoadStyleLink(), LoadChildSheet(), and InternalLoadNonDocumentSheet()) call CheckLoadAllowed() and then call LoadSheet(). When we call NS_NewChannel(), we set mDocument to be the requestingNode. But calls to CheckLoadAllowed() try to set the requestingContext to something else, and then fallback to the document (examples in LoadStyleLink(), LoadChildSheet()). Hence, can we add a third parameter to LoadSheet() that includes the requestingContext, and then use that in the call to NS_NewChannel? Alternatively, could we add the requestingContext to the first parameter of LoadSheet() - SheetLoadData (https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#106) ? **For layout/style/nsFontFaceLoader.cpp, it looks like the csp for channelPolicy is taken off aProxy->mPrincipal and aFontToLoad->mPrincipal, respectively. Should we pass these in as requestingPrincipals? Or should we use ps->GetDocument(), which is passed into content policies, but in a completely different part of the code. Not sure how we go from checking the policies in CheckLoadAllowed() to StartLoad() where NS_NewChannel() is called. Is ps->GetDocument()'s principal the same as mPrincipal?
Attachment #8478605 -
Flags: review?(dbaron)
Assignee | ||
Comment 120•10 years ago
|
||
Attachment #8478607 -
Flags: review?(smichaud)
Assignee | ||
Comment 121•10 years ago
|
||
** widget/windows/WinUtils.cpp :: AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void) - We are calling NS_NewChannel() with systemPrincipal. Assuming this is using a generic favicon and no network request is being made, systemPrincipal is okay. Can you confirm this? If a network request is being made, we need to find the real loading principal. ** widget/windows/nsDataObj.cpp Not sure what this code does. Please look carefully at this one to check its use of systemPrincipal. Can we identify the loader principal, or is this loaded by system?
Attachment #8478608 -
Flags: review?(jmathies)
Assignee | ||
Comment 122•10 years ago
|
||
Attachment #8478613 -
Flags: review?(roc)
Assignee | ||
Comment 123•10 years ago
|
||
Attachment #8478615 -
Flags: review?(gpascutto)
Assignee | ||
Comment 124•10 years ago
|
||
Attachment #8478617 -
Flags: review?(mak77)
Assignee | ||
Comment 125•10 years ago
|
||
Attachment #8478620 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 126•10 years ago
|
||
* Does passing systemPrincipal as the loadingPrincipal make sense for these files? Are the channels created by system operations? For intl/strres/nsStringBundleTextOverride.cpp and intl/hyphenation/hnjstdio.cpp its probably okay. But for /intl/strres/nsStringBundle.cpp it's less clear.
Attachment #8478621 -
Flags: review?(jshin1987)
Assignee | ||
Comment 127•10 years ago
|
||
Attachment #8478622 -
Flags: review?(timeless)
Assignee | ||
Comment 128•10 years ago
|
||
Attachment #8478623 -
Flags: review?(benjamin)
Assignee | ||
Comment 129•10 years ago
|
||
Attachment #8478624 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 130•10 years ago
|
||
Attachment #8478625 -
Flags: review?(l10n)
Assignee | ||
Comment 131•10 years ago
|
||
Attachment #8478626 -
Flags: review?(mrbkap)
Attachment #8478605 -
Flags: review?(dbaron) → review?(bzbarsky)
Assignee | ||
Comment 132•10 years ago
|
||
Attachment #8478627 -
Flags: review?(mwu)
Assignee | ||
Comment 133•10 years ago
|
||
Assignee | ||
Comment 134•10 years ago
|
||
Assignee | ||
Comment 135•10 years ago
|
||
Attachment #8478633 -
Flags: review?(roc)
Assignee | ||
Comment 136•10 years ago
|
||
Attachment #8478634 -
Flags: review?(ehsan)
Assignee | ||
Comment 137•10 years ago
|
||
* What does nsWebBrowserPersist::SaveURIInternal() do? Is there a way to get the loading Document here so we don't use systemPrincipal? Or is this really a system operation?
Attachment #8478635 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 138•10 years ago
|
||
Attachment #8478636 -
Flags: review?(ehsan)
Assignee | ||
Comment 139•10 years ago
|
||
Attachment #8478637 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 140•10 years ago
|
||
Attachment #8478638 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8478628 -
Flags: review?(mrbkap)
Assignee | ||
Updated•10 years ago
|
Attachment #8478632 -
Flags: review?(seth)
Updated•10 years ago
|
Attachment #8478634 -
Flags: review?(ehsan) → review+
Comment 141•10 years ago
|
||
Comment on attachment 8478636 [details] [diff] [review] 3_30_callsites_in_editor.patch Review of attachment 8478636 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsHTMLDataTransfer.cpp @@ +1091,5 @@ > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER, > + nullptr, // outChannel > + getter_AddRefs(imageStream)); Nit: please remove the trailing whitespace.
Attachment #8478636 -
Flags: review?(ehsan) → review+
Comment 142•10 years ago
|
||
Reposting this for reviewers who may not have seen it - (In reply to Christoph Kerschbaumer [:ckerschb] from comment #106) > Summary for all reviewers: > Channels know very little about who initiated the request for the data they > are fetching. But there are use cases where we need some loading > information during the lifetime of the channel. Hence, in bug 965413, bz > added a LoadInfo object that hangs off channels. This bug is to populate > that LoadInfo object at channel creation time (via NS_NewChannel and > friends). In this code, we pass a loadingPrincipal and/or a loadingNode to > NS_NewChannel and NS_NewChannel then creates the loadInfo object with the > passed in values. > > Please take a careful look at the code that we have flagged you for. In > particular, look to see if we have identified the correct loading node > and/or loading principal. We are not looking for the node and principal > corresponding to the data that the channel is fetching, but the node and > principal of the entity that is loading that data. These values are crucial > to security checking code that is currently being reimplemented. If we pass > the wrong values, we may end up allowing resource loads that should be > blocked by the browser. If we use systemPrincipal, that will mean that all > loads will be permitted. So please check that the load is coming from the > system and that any needed security checks have already been done.
Comment 143•10 years ago
|
||
Comment on attachment 8478607 [details] [diff] [review] 3_12_callsites_in_widget_cocoa.patch I've read comment #106, and I *think* I understand it. Which leads me to ask the following question: nsISound.play(in nsIURL aURL) is scriptable, so it can be called from user-level code, on an arbitrary URL. In that case, is it correct to always pass the system principal to NS_NewStreamLoader(), as this patch does?
Comment on attachment 8478596 [details] [diff] [review] 3_04_callsites_in_content_media.patch Review of attachment 8478596 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaResource.cpp @@ +939,5 @@ > + nullptr, // aChannelPolicy > + element, > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER, Shouldn't this be TYPE_MEDIA? @@ +1448,5 @@ > + nullptr, // aChannelPolicy > + element, > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER, TYPE_MEDIA
Attachment #8478596 -
Flags: review?(roc) → review-
Comment on attachment 8478603 [details] [diff] [review] 3_10_callsites_in_layout_mathml.patch Review of attachment 8478603 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLChar.cpp @@ +155,5 @@ > + nullptr, // aCharset > + nullptr, // aBaseURI > + nullptr, // aIoService > + nsContentUtils::GetSystemPrincipal(), > + nsIContentPolicy::TYPE_FONT, This isn't an actual font. I think it should be TYPE_OTHER.
Attachment #8478603 -
Flags: review?(roc) → review-
Attachment #8478613 -
Flags: review?(roc) → review+
Comment on attachment 8478633 [details] [diff] [review] 3_27_callsites_in_gfx.patch Review of attachment 8478633 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxUserFontSet.cpp @@ +1088,5 @@ > + nullptr, // aChannelPolicy > + aPrincipal, > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER, This is a font, but we're not really loading it here, so I guess this is OK.
Attachment #8478633 -
Flags: review?(roc) → review+
(In reply to Steven Michaud from comment #143) > nsISound.play(in nsIURL aURL) is scriptable, so it can be called from > user-level code, on an arbitrary URL. In that case, is it correct to always > pass the system principal to NS_NewStreamLoader(), as this patch does? What does "user-level code" mean? Is that as opposed to kernel code, i.e. that JS code that's part of the Firefox UI, a.k.a. chrome JS? Or does that mean that a webpage can cause that to be called using a webpage provided URI? Chrome JS, addons, and C++ code is generally considered to be part "the system", and so passing a system principal is generally ok. However if webpage code is the one that triggers a request with a webpage controller URI, then passing a system principal is not ok. So if the code path is webpage --calls--> someDOMfunction(uri) --calls--> someChromeFunction(uri) --calls--> nsISound.play(uri) then passing a system principal is not ok. There the webpage principal needs to be used. However if the webpage does something, for example close a tab, which triggers some chrome UI, and the chrome UI decides that as part of a transition or whatever we want to play a "woooosh" sound effect, then passing a system principal is ok. I.e. if a network load happens on the request of a webpage, including using a webpage provided URI, then the principal needs to be the principal of the webpage. If a network load happens on request of chrome code, then the principal should be the system principal. Another way to think about it is that loading some URIs can transfer the user's money from the user's bank account. If you want a security check to happen before the network load starts as to ensure that such a URI isn't being loaded, then don't pass a system principal. If you are sure that the URI is fine, then go ahead and use a system principal. A good example is, the favicon displayed in each tab. That should not be loaded using the system principal. However the lock icon displayed in the URL bar on https websites should be loaded using the system principal. I hope that clears it up.
Comment 148•10 years ago
|
||
Comment on attachment 8478607 [details] [diff] [review] 3_12_callsites_in_widget_cocoa.patch (In reply to comment #147) Actually you can't create an nsISound object in non-privileged (non-Chrome) JavaScript code. I just realized this when I tried it and couldn't get it to work. So there shouldn't be any problem passing the system principal to NS_NewStreamLoader() in this patch.
Attachment #8478607 -
Flags: review?(smichaud) → review+
Comment 149•10 years ago
|
||
> Actually you can't create an nsISound object in non-privileged (non-Chrome) JavaScript code.
Or presumably any XPCOM object.
Comment 150•10 years ago
|
||
> * What does nsWebBrowserPersist::SaveURIInternal() do? Implement things like "save link as" and the saving of subresources for "save document as". > Or is this really a system operation? It sort of kind of is, maybe. At least for the "save link as" case. For the case when we're saving a document and all it subresources, we should probably use the document's principal here.
Comment 151•10 years ago
|
||
(Following up comment #148) Actually, rereading comment #147, things aren't quite so simple as I say in comment #148. But there's no way to tell from widget code what ultimately triggered a call to nsISound.play() -- a webpage, or just Chrome code. And widget code knows nothing about web pages. So we're (more or less) forced to default to passing the system principal to NS_NewStreamLoader().
Comment 152•10 years ago
|
||
Comment on attachment 8478625 [details] [diff] [review] 3_22_callsites_in_rdf.patch Redirecting to RDF-pike instead of L10N-pike
Attachment #8478625 -
Flags: review?(l10n) → review?(axel)
Comment 153•10 years ago
|
||
Comment on attachment 8478625 [details] [diff] [review] 3_22_callsites_in_rdf.patch Review of attachment 8478625 [details] [diff] [review]: ----------------------------------------------------------------- Actually, Benjamin is a better reviewer for this.
Attachment #8478625 -
Flags: review?(axel) → review?(benjamin)
Comment 154•10 years ago
|
||
Comment on attachment 8478615 [details] [diff] [review] 3_15_callsites_in_toolkit_components_url-classifier.patch Review of attachment 8478615 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp @@ +103,5 @@ > + nsContentUtils::GetSystemPrincipal(), > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER, > + getter_AddRefs(mChannel)); We'll end up modifying the LoadContext on this channel, as per https://hg.mozilla.org/mozilla-central/rev/61817a92297c but I guess that's irrelevant to this patch?
Attachment #8478615 -
Flags: review?(gpascutto) → review+
Updated•10 years ago
|
Attachment #8478623 -
Flags: review?(benjamin) → review+
Comment 155•10 years ago
|
||
Comment on attachment 8478625 [details] [diff] [review] 3_22_callsites_in_rdf.patch We should just remove the RDF code entirely, but while it's here I'm pretty certain that this is the correct setup.
Attachment #8478625 -
Flags: review?(benjamin) → review+
Comment 156•10 years ago
|
||
(In reply to Steven Michaud from comment #151) > (Following up comment #148) > > Actually, rereading comment #147, things aren't quite so simple as I say in > comment #148. > > But there's no way to tell from widget code what ultimately triggered a call > to nsISound.play() -- a webpage, or just Chrome code. And widget code knows > nothing about web pages. So we're (more or less) forced to default to > passing the system principal to NS_NewStreamLoader(). Can we add a parameter to play() telling us whether chrome code called it? In those cases, we can use systemPrincipal, and if its a webpage we can use nullprincipal?
Comment 157•10 years ago
|
||
> Can we add a parameter to play() telling us whether chrome code called it?
I frankly don't know if this is reasonable or feasible. What do you think, Boris?
Flags: needinfo?(bzbarsky)
(In reply to Tanvi Vyas [:tanvi] from comment #156) > Can we add a parameter to play() telling us whether chrome code called it? > In those cases, we can use systemPrincipal, and if its a webpage we can use > nullprincipal? Don't use the nullprincipal for webpage use (docshell is an exception). The right solution would be to add a node/principal argument to play(). But it would be good if someone that knows how/when we use nsISound could actually verify if any of this is needed.
Assignee | ||
Comment 159•10 years ago
|
||
Comment on attachment 8478592 [details] [diff] [review] 3_01_callsites_in_content_base-html.patch Review of attachment 8478592 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsXMLHttpRequest.cpp @@ +1757,5 @@ > + nullptr, // aIoService > + loadGroup, > + nullptr, // aCallbacks > + nsIRequest::LOAD_BACKGROUND, > + nullptr, // aChannelPolicy Note to myself: Obviously this should not a nullptr, but rather the channelPolicy from above.
Comment 160•10 years ago
|
||
Comment on attachment 8478608 [details] [diff] [review] 3_13_callsites_in_widget_windows.patch (In reply to Christoph Kerschbaumer [:ckerschb] from comment #121) > Created attachment 8478608 [details] [diff] [review] > 3_13_callsites_in_widget_windows.patch > > ** widget/windows/WinUtils.cpp :: > AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void) - We are calling > NS_NewChannel() with systemPrincipal. Assuming this is using a generic > favicon and no network request is being made, systemPrincipal is okay. Can > you confirm this? If a network request is being made, we need to find the > real loading principal. This is loading a moz-icon which I'm pretty sure is restricted to local access. They aren't designed to go out onto the network since they load icons out of local files for proper desktop integration in areas like helper application settings. > ** widget/windows/nsDataObj.cpp > Not sure what this code does. Please look carefully at this one to check > its use of systemPrincipal. Can we identify the loader principal, or is > this loaded by system? This is used in drag and drop on Windows. nsDataObj is a COM object that windows speaks two when the user is dragging things like images out of content. Since nsIRequest::LOAD_FROM_CACHE doesn't guarantee cache only hits, it sounds like the right principal needs to be set. The object gets created when the drag session is invoked in nsDragService here - http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDragService.cpp#174 http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsClipboard.cpp#115 You can pass whatever information that's needed/available when the object is created.
Attachment #8478608 -
Flags: review?(jmathies)
Assignee | ||
Comment 161•10 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #144) > 3_04_callsites_in_content_media.patch > ::: content/media/MediaResource.cpp > > + nsILoadInfo::eNotSandboxed, > > + nsIContentPolicy::TYPE_OTHER, > > Shouldn't this be TYPE_MEDIA? Indeed - good catch!
Attachment #8478596 -
Attachment is obsolete: true
Attachment #8479349 -
Flags: review?(roc)
Assignee | ||
Comment 162•10 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #145) > 3_10_callsites_in_layout_mathml.patch > ::: layout/mathml/nsMathMLChar.cpp > > + nsContentUtils::GetSystemPrincipal(), > > + nsIContentPolicy::TYPE_FONT, > > This isn't an actual font. I think it should be TYPE_OTHER. Updated!
Attachment #8478603 -
Attachment is obsolete: true
Attachment #8479350 -
Flags: review?(roc)
Attachment #8479349 -
Flags: review?(roc) → review+
Attachment #8479350 -
Flags: review?(roc) → review+
Updated•10 years ago
|
Attachment #8478628 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8478626 -
Flags: review?(mrbkap) → review+
Comment 163•10 years ago
|
||
Comment on attachment 8478594 [details] [diff] [review] 3_02_callsites_in_content_base_src_ImportManager.patch Review of attachment 8478594 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Christoph Kerschbaumer [:ckerschb] from comment #110) > Are mImportParent->MasterDocument()'s ScriptObjectPrincipal and > mImportParent->NodePrincipal the same? Yes they are.
Attachment #8478594 -
Flags: review?(gkrizsanits) → review+
Comment 164•10 years ago
|
||
Comment on attachment 8478620 [details] [diff] [review] 3_17_callsites_in_toolkit_components_downloads.patch Note that nsDownloadManager.cpp is only used on Firefox for Android now. I believe it is still covered by automated tests on that platform, though I'm not sure whether resuming is one of them. This change however looks straightforward, and is likely to work if it builds.
Attachment #8478620 -
Flags: review?(paolo.mozmail) → review+
Updated•10 years ago
|
Attachment #8478599 -
Flags: review?(bent.mozilla) → review+
Comment 165•10 years ago
|
||
Comment on attachment 8478622 [details] [diff] [review] 3_19_callsites_in_xpfe.patch TYPE_OTHER should probably
Attachment #8478622 -
Flags: review?(timeless) → review?(neil)
Comment 166•10 years ago
|
||
Comment on attachment 8478622 [details] [diff] [review] 3_19_callsites_in_xpfe.patch TYPE_OTHER should probably be TYPE_DOCUMENT
Updated•10 years ago
|
Attachment #8478622 -
Flags: review?(neil) → review+
Comment 167•10 years ago
|
||
Comment on attachment 8478590 [details] [diff] [review] 1_loadinfo_changes.patch >+++ b/docshell/base/LoadInfo.cpp >+ nsIDOMNode* aLoadingContext, Why nsIDOMNode*? Why not just nsINode*? Seems like the main consumer has an nsINode* anyway... The IDL-visible thing would obviously keep being nsIDOMNode. >+++ b/docshell/base/LoadInfo.h >+#include "nsIDOMNode.h" This should probably be a forward-declare of nsINode, with the actual #include in the .cpp. >+++ b/docshell/base/nsILoadInfo.idl >+/* In bug 1041176, the flags will be collapsed into on s/on/an/, but if this is just getting removed maybe not worth bothering. >+ const unsigned long eInheritPrincipal = 0; >+ const unsigned long eDontInheritPrincipal = 1; I think these should probably be the other way around, since it's the "inherit the principal" case that should have the extra flag. I guess bug 1041176 fixes this up too? >+ * like MCB, and CSP. The loadingContext of a channel is the It took me a bit to figure out what "MCB" is. Since that's not a standard abbreviation, worth spelling it out. >+ * if http://a.com >+ * includes an image from http://b.com, the loadingContext >+ * attached to http://b.com's channel is the node for http://a.com Should probably be: if a document loaded from http://a.com includes an image loaded from http://b.com, the loadingContext attached to the image load's channel is the <img> node in the http://a.com document. or so. >+ readonly attribute nsIDOMNode loadingContext; It would be nice to have a C++-friendly getter here too. Something [notxpcom,nostdcall,noscript] returning an already_AddRefed<nsINode>, say. Some of the other patches in this bug could benefit from such a thing. You can see nsIFrameLoader.idl's GetFrameLoader for an example of how to declare such a thing. >+ readonly attribute nsContentPolicyType contentPolicyType; [infallible], right? r=me with those nits fixed.
Attachment #8478590 -
Flags: review?(bzbarsky) → review+
Comment 168•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #167) > Comment on attachment 8478590 [details] [diff] [review] > 1_loadinfo_changes.patch > >+++ b/docshell/base/LoadInfo.cpp > >+ * if http://a.com > >+ * includes an image from http://b.com, the loadingContext > >+ * attached to http://b.com's channel is the node for http://a.com > > Should probably be: > > if a document loaded from http://a.com includes an image loaded from > http://b.com, the loadingContext attached to the image load's channel is > the > <img> node in the http://a.com document. > > or so. > That's not quite right. The loadingContext attached to the image load's channels is the node for the http://a.com document. It's not the <img> node itself.
Comment 169•10 years ago
|
||
> It's not the <img> node itself.
Ah, I see. In that case <img> may not be the best example to use here.
Comment 170•10 years ago
|
||
Comment on attachment 8478590 [details] [diff] [review] 1_loadinfo_changes.patch >diff --git a/docshell/base/nsILoadInfo.idl b/docshell/base/nsILoadInfo.idl > > /** >+ * The loadingContext of the channel, used for security checks Replace with: The loadingContext of the channel is used for security checks
Comment 171•10 years ago
|
||
> Can we add a parameter to play() telling us whether chrome code called it?
As far as I can tell from MXR, we never use nsISound::Play in Firefox code.
We can add a parameter, but we obviously can't enforce addons using it so far, right? So I think it's fine to treat nsISound::Play as a chrome load for now and if we think that addons can screw things up by passing untrusted URIs to it we should file a followup bug to see what we can do about that. Most simply, by checking whether addons really use it and if not removing the API.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 172•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #167) > >+ const unsigned long eInheritPrincipal = 0; > >+ const unsigned long eDontInheritPrincipal = 1; > > I think these should probably be the other way around, since it's the > "inherit the principal" case that should have the extra flag. I guess bug > 1041176 fixes this up too? Yes, bug 1041176 cleans that up too. > >+ readonly attribute nsContentPolicyType contentPolicyType; > > [infallible], right? error: [infallible] only works on builtin types (numbers, booleans, and raw char types), ../../../dist/idl/nsILoadInfo.idl line 55:25 0:07.71 [infallible] readonly attribute nsContentPolicyType contentPolicyType; We could leave as is and follow up on it once we allow other types than builtin types to be infallible, or we could make it an unsigned long and not use type nsContentPolicyType. I would rather follow up on it later. But in general, i totally agree, it should be infallible.
Comment 173•10 years ago
|
||
Comment on attachment 8478595 [details] [diff] [review] 3_03_callsites_in_content_xul.patch I think this patch shows that the new NS_NewChannel signature needs work. Specifically, it has the following _required_ arguments, for any sane Gecko caller: URI, loadgroup, principal, two loadinfo bits, content policy type, outparam. It also has the following arguments that are fundamentally optional, in order from most likely to be used to least likely to be used: channelPolicy, callbacks, ioservice. Those three arguments should probably stay optional. The callbacks and ioservice should _definitely_ stay optional. Yes, this means having the outparam come before them; that's why it used to be in the first spot. Putting the loadgroup arg before the ioservice arg will incidentally fix a problem the current API has, as long as we're changing this stuff... Similar for NS_NewStreamLoader. r=me with that fixed so people don't have to pass quite as many bajillions of arguments.
Attachment #8478595 -
Flags: review?(bzbarsky) → review+
Comment 174•10 years ago
|
||
> error: [infallible] only works on builtin types
Oh, right, that was discussed earlier and I forgot about it. :( Sorry.
I think the simplest expedient thing to do here is to manually do the work that [infallible] does for you. That is, throw in a %C++ block that has:
inline nsContentPolicyType GetContentPolicyType()
{
nsContentPolicyType result;
mozilla::DebugOnly<nsresult> rv = GetContentPolicyType(&result);
MOZ_ASSERT(NS_SUCCEEDED(rv));
return result;
}
Assignee | ||
Comment 175•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #173) > Comment on attachment 8478595 [details] [diff] [review] > 3_03_callsites_in_content_xul.patch > > I think this patch shows that the new NS_NewChannel signature needs work. > > Specifically, it has the following _required_ arguments, for any sane Gecko > caller: URI, loadgroup, principal, two loadinfo bits, content policy type, > outparam. > > It also has the following arguments that are fundamentally optional, in > order from most likely to be used to least likely to be used: channelPolicy, > callbacks, ioservice. Bug 1041180 removes the channelpolicy completely, since we now store the requestingPrincipal and the contentPolicyType in the loadgroup. I already have a patch for that and verified that that is working as expected. Will put it on bugzilla soon. So that argument goes away. > Those three arguments should probably stay optional. The callbacks and > ioservice should _definitely_ stay optional. Yes, this means having the > outparam come before them; that's why it used to be in the first spot. Any chance I could convince you that we remain with the current API? Otherwise I will go ahead and make them optinal again.
Comment 176•10 years ago
|
||
Comment on attachment 8478602 [details] [diff] [review] 3_09_callsites_in_docshell.patch >+ nsCOMPtr<nsINode> requestingNode = nullptr; Can this just be nsINode*? You definitely don't need the "= nullptr" bit; you're assigning the variable on the very next line. I think using GetFrameElementInternal in all cases here is wrong. That will be the <xul:browser> for a toplevel tab, no? Do we not have tests for that? The right thing to do is to use GetFrameElementInternal() only when aContentPolicyType is subdocument, I would think. >+ nsCOMPtr<nsPIDOMWindow> win = do_QueryObject(mScriptGlobal); This is completely pointless, no? mScriptGlobal is already a subclass of nsPIDOMWindow. >+ nsPIDOMWindow* innerWin = mScriptGlobal->GetCurrentInnerWindow(); >+ if (innerWin) { >+ requestingNode = innerWin->GetExtantDoc(); >+ } This is equivalent to: requestingNode = mScriptGlobal->GetExtantDoc(); But why are we using the currently-loaded document as the "source node" for the load anyway? This is a toplevel load, so null might seem like a saner value here? >+ // Please note that in comparsion to NS_CheckContentLoadPolicy we do not >+ // use the referrer to create a principal which would be wrong, because >+ // "about:blank" would have the principal "chrome://browser/content/browser.xul" So wait. How is that going to work once you push the NS_CheckContentLoadPolicy check down into the channel implementation like you plan to? We should get this sorted out. Also, the comment is talking about some special cases of about:blank, not about:blank in general, even though it sure tries to sound like it's the latter.... >+ // we have to manually set the loadInfo here, since the channel within >+ // that else branch is not created through NS_NewChannel and hence You mean "the channel we just created"? I'm not sure what "else branch" this is referring to, but this is clearly talking about the channel that was just created. I don't quite understand this patch. It does all this stuff, but then we still end up calling SetupChannelOwner later, right? Which is good, because the loadinfo it creates in the srcdoc case is totally bogus (claims to be not sandboxed when it might need to be, claims to not inherit the principal when it should). I'm going to assume that this is fixed up in the SetupChannelOwner patch in the queue... r=me with the above fixed and the interaction with referrer sorted out. It does make sense to me to use a nullprincipal here, but then we need to make sure it's not used for any security checks on the load itself that we really care about. :( Which cases have a null owner here anyway?
Attachment #8478602 -
Flags: review?(bzbarsky) → review+
Comment 177•10 years ago
|
||
Comment on attachment 8478635 [details] [diff] [review] 3_29_callsites_in_embedding.patch r=me
Attachment #8478635 -
Flags: review?(bzbarsky) → review+
Comment 178•10 years ago
|
||
Comment on attachment 8478638 [details] [diff] [review] 5_GetChannelPrincipal_changes.patch Fwiw, I think the diff would be smaller and the blame better preserved if the .cpp file had nsScriptSecurityManager::GetChannelURIPrincipal after GetChannelResultPrincipal. Please make that change. Please document in nsCrossSiteListenerProxy.cpp why it's not using GetChannelResultPrincipal. r=me with that
Attachment #8478638 -
Flags: review?(bzbarsky) → review+
Comment 179•10 years ago
|
||
Comment on attachment 8478605 [details] [diff] [review] 3_11_callsites_in_layout_style.patch >+++ b/layout/style/Loader.cpp >+ nsCOMPtr<nsIPrincipal> requestingPrincipal = aLoadData->mLoaderPrincipal; Seems like this could just be an nsIPrincipal*, right? >+ requestingPrincipal = do_CreateInstance("@mozilla.org/nullprincipal;1", &rv); This doesn't look right to me. At least if we plan to use this principal for security checks. The only time aLoadData->mLoaderPrincipal is null here is if InternalLoadNonDocumentSheet got called with a null principal. That can happen via: LoadSheetSync LoadSheet with a null aOriginPrincipal All the LoadSheetSync callsites are system. The only caller of LoadSheet that passes a null aOriginPrincipal is nsHTMLEditor::ReplaceStyleSheet, which is system code. So !requestingPrincipal should be treated as the system-principal case. You can also tell this because in this case we currently skip sheet loading security checks. >+++ b/layout/style/nsFontFaceLoader.cpp >@@ -338,24 +338,27 @@ nsUserFontSet::StartLoad(gfxMixedFontFam Hmm. So this is getting the CSP from aProxy->mPrincipal. But it's using ps->GetDocument() as the node and not passing in a principal, right? That looks wrong. The reason there's a separate principal on aProxy is so that user stylesheets applied to the document can load fonts that the document itself can't load. See nsUserFontSet::CheckFontLoad. So this should be passing in aProxy->mPrincipal for the principal, in addition to passing a node. In general, it's worth making sure that the principals you pass in for all these patches match the principals security checks are actually done on. Similar in SyncLoadFontData but with aFontToLoad. r=me with the above fixed.
Attachment #8478605 -
Flags: review?(bzbarsky) → review+
Comment 180•10 years ago
|
||
Comment on attachment 8478637 [details] [diff] [review] 4_setupChannelOwner_changes.patch r=me, but the nullprincipal bits in the image loader patch context look wrong to me for the same reasons that they look wrong to me in the docshell code... no principal provided is more likely to be a system load than a "don't do this load" load, and using a nullprincipal for a load almost certainly means the load will be blocked once we push the security checks down, no?
Attachment #8478637 -
Flags: review?(bzbarsky) → review+
Comment 181•10 years ago
|
||
In general, I think any place where we cons up a NullPrincipal and then use it in loadinfo in these patches is almost certainly a bug... If we have to land some of them for now, we should have clear bugs filed on getting them fixed before we push security checks down into the channel, so we don't have to go reaudit all this code again.
Assignee | ||
Comment 182•10 years ago
|
||
Comment on attachment 8478591 [details] [diff] [review] 2_nsNetutil_changes.patch Skipping the review for now - since Boris requested some changes in the API. No need to review it before the change.
Attachment #8478591 -
Flags: review?(mcmanus)
Comment 183•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #179) > Comment on attachment 8478605 [details] [diff] [review] > 3_11_callsites_in_layout_style.patch > > >+++ b/layout/style/Loader.cpp > >+ nsCOMPtr<nsIPrincipal> requestingPrincipal = aLoadData->mLoaderPrincipal; > > Seems like this could just be an nsIPrincipal*, right? > > >+ requestingPrincipal = do_CreateInstance("@mozilla.org/nullprincipal;1", &rv); > > This doesn't look right to me. At least if we plan to use this principal > for security checks. The only time aLoadData->mLoaderPrincipal is null > here is if InternalLoadNonDocumentSheet got called with a null principal. > That can happen via: > > LoadSheetSync > LoadSheet with a null aOriginPrincipal > > All the LoadSheetSync callsites are system. The only caller of LoadSheet > that passes a null aOriginPrincipal is nsHTMLEditor::ReplaceStyleSheet, > which is system code. > > So !requestingPrincipal should be treated as the system-principal case. You > can also tell this because in this case we currently skip sheet loading > security checks. > > >+++ b/layout/style/nsFontFaceLoader.cpp > >@@ -338,24 +338,27 @@ nsUserFontSet::StartLoad(gfxMixedFontFam > > Hmm. So this is getting the CSP from aProxy->mPrincipal. But it's using > ps->GetDocument() as the node and not passing in a principal, right? > > That looks wrong. The reason there's a separate principal on aProxy is so > that user stylesheets applied to the document can load fonts that the > document itself can't load. See nsUserFontSet::CheckFontLoad. > > So this should be passing in aProxy->mPrincipal for the principal, in > addition to passing a node. > > In general, it's worth making sure that the principals you pass in for all > these patches match the principals security checks are actually done on. > > Similar in SyncLoadFontData but with aFontToLoad. > > r=me with the above fixed. Hi Boris, Did you happen to see comment 119? It discusses similar points and asks some questions. I went through almost all the callsites and tried to manually inspect that we were passing the same values to NS_NewChannel as we pass to content policies. Where there were discrepancies, we asked the reviewers. Since dbaron passed this review to you, you probably didn't see the comments included when we flagged him for review. For the layout/style/Loader.cpp, I propose passing in the requestingContext instead of using mDocument (mDocument doesn't look right to me for the reasons mentioned in the comment). However, this doesn't address the case where we have a null requestingPrincipal. We can either fall back to systemPrincipal or fall back to nullPrincipal for now and create another bug to remove nullPrincipal before we move security checks to asyncOpen. For nsFontFaceLoader, it sounds like we can pass in aProxy->mPrincipal to the first call to NS_NewChannelInternal and aFontToLoad->mPrincipal to the second call to NS_NewChannelInternal.
Comment 184•10 years ago
|
||
> Did you happen to see comment 119? No, I hadn't, sorry. This bug is too long. :( > I propose passing in the requestingContext instead of using mDocument That seems fine, yes. > We can either fall back to systemPrincipal or fall back to nullPrincipal You want the system principal. > we can pass in aProxy->mPrincipal to the first call to NS_NewChannelInternal and > aFontToLoad->mPrincipal to the second call Yes, exactly.
Comment 185•10 years ago
|
||
Comment on attachment 8478624 [details] [diff] [review] 3_21_callsites_in_uriloader.patch Review of attachment 8478624 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +382,5 @@ > > + rv = NS_NewChannel(mURI, > + nullptr, // aIoService > + nullptr, // loadGroup > + this, nit, maybe leave the comment that these are the callbacks
Attachment #8478624 -
Flags: review?(honzab.moz) → review+
Comment 186•10 years ago
|
||
Comment on attachment 8478627 [details] [diff] [review] 3_24_callsites_in_modules.patch Review of attachment 8478627 [details] [diff] [review]: ----------------------------------------------------------------- This code is in a channel, so could we reuse some of the load info that's already set on the channel?
Assignee | ||
Comment 187•10 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #186) > Comment on attachment 8478627 [details] [diff] [review] > 3_24_callsites_in_modules.patch > > Review of attachment 8478627 [details] [diff] [review]: > ----------------------------------------------------------------- > > This code is in a channel, so could we reuse some of the load info that's > already set on the channel? Unfortunately all the information we are about to set in the loadInfo for this patch is currently not available on a channel. Please see: https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c106
Flags: needinfo?(mwu)
Comment on attachment 8478627 [details] [diff] [review] 3_24_callsites_in_modules.patch Review of attachment 8478627 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libjar/nsJARChannel.cpp @@ +862,5 @@ > + mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS), > + nsContentUtils::GetSystemPrincipal(), > + nsILoadInfo::eDontInheritPrincipal, > + nsILoadInfo::eNotSandboxed, > + nsIContentPolicy::TYPE_OTHER); This does not seem right. Why not use the loadinfo that is on the jar-channel? nsJarChannel *is* a channel so you should have all this information available from whoever created the nsJarChannel. (I think that's what mwu was saying too)
Assignee | ||
Comment 189•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #188) > Comment on attachment 8478627 [details] [diff] [review] > 3_24_callsites_in_modules.patch > > Review of attachment 8478627 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: modules/libjar/nsJARChannel.cpp > @@ +862,5 @@ > > + mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS), > > + nsContentUtils::GetSystemPrincipal(), > > + nsILoadInfo::eDontInheritPrincipal, > > + nsILoadInfo::eNotSandboxed, > > + nsIContentPolicy::TYPE_OTHER); > > This does not seem right. Why not use the loadinfo that is on the > jar-channel? nsJarChannel *is* a channel so you should have all this > information available from whoever created the nsJarChannel. (I think that's > what mwu was saying too) Ah, that makes sense now as I re-read Michael's comment. Clearing needinfo-flag and will incorporate that change.
Flags: needinfo?(mwu)
Comment 190•10 years ago
|
||
Comment on attachment 8478600 [details] [diff] [review] 3_07_callsites_in_netwerk.patch Review of attachment 8478600 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsBufferedStreams.cpp @@ +814,5 @@ > NS_IF_ADDREF(*aStream); > return NS_OK; > } > > +#undef METER is this a ridealong?
Attachment #8478600 -
Flags: review?(mcmanus) → review+
Updated•10 years ago
|
Attachment #8478601 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 191•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #173) > Comment on attachment 8478595 [details] [diff] [review] > 3_03_callsites_in_content_xul.patch > > I think this patch shows that the new NS_NewChannel signature needs work. > > Specifically, it has the following _required_ arguments, for any sane Gecko > caller: URI, loadgroup, principal, two loadinfo bits, content policy type, > outparam. > > It also has the following arguments that are fundamentally optional, in > order from most likely to be used to least likely to be used: channelPolicy, > callbacks, ioservice. Changed the API, so the signature looks like this: > inline nsresult /* NS_NewChannelNode */ > NS_NewChannel(nsIChannel** outChannel, > nsIURI* aUri, > nsINode* aRequestingNode, > nsSecurityFlags aSecurityFlags, > nsContentPolicyType aContentPolicyType, > nsIChannelPolicy* aChannelPolicy = nullptr, > nsILoadGroup* aLoadGroup = nullptr, > nsIInterfaceRequestor* aCallbacks = nullptr, > nsLoadFlags aLoadFlags = nsIRequest::LOAD_NORMAL, > nsIIOService* aIoService = nullptr) Agreed, that change cleans up callsites; I incorporated similar changes for NS_OpenURI, and NS_NewStreamLoader.
Updated•10 years ago
|
Attachment #8478627 -
Flags: review?(mwu)
Assignee | ||
Comment 192•10 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #190) > Comment on attachment 8478600 [details] [diff] [review] > 3_07_callsites_in_netwerk.patch > > Review of attachment 8478600 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/src/nsBufferedStreams.cpp > @@ +814,5 @@ > > NS_IF_ADDREF(*aStream); > > return NS_OK; > > } > > > > +#undef METER > > is this a ridealong? This bug is getting way to long :-(. I wrote a quick note when I flagged you for review: Comment 115
Assignee | ||
Comment 193•10 years ago
|
||
Comment on attachment 8478592 [details] [diff] [review] 3_01_callsites_in_content_base-html.patch As discussed in person, I will put up a new patch since this one got bitroted by changes requested by other reviewers.
Attachment #8478592 -
Flags: review?(jst)
Assignee | ||
Comment 194•10 years ago
|
||
Comment on attachment 8478597 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch As discussed in person, I will put up a new patch since this one got bitroted by changes requested by other reviewers.
Attachment #8478597 -
Flags: review?(jst)
Comment 195•10 years ago
|
||
Comment on attachment 8478632 [details] [diff] [review] 3_26_callsites_in_image.patch Review of attachment 8478632 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! You need to update the commit message to say "r=seth" instead of "r=joe".
Attachment #8478632 -
Flags: review?(seth) → review+
Comment on attachment 8478632 [details] [diff] [review] 3_26_callsites_in_image.patch Review of attachment 8478632 [details] [diff] [review]: ----------------------------------------------------------------- Sorry if I'm misunderstanding the code, but this doesn't look right to me. ::: image/src/imgLoader.cpp @@ +1402,5 @@ > mAcceptHeader, > aLoadFlags, > aPolicy, > + aLoadingPrincipal, > + aCX); Isn't aCX a JS-context? Those are not nodes so the QI in NewImageChannel will fail and return null. @@ +1902,5 @@ > mAcceptHeader, > requestFlags, > aPolicy, > + aLoadingPrincipal, > + aCX); Same here.
Attachment #8478632 -
Flags: feedback-
Comment 197•10 years ago
|
||
> Isn't aCX a JS-context?
No. It's the random nsISupports argument to imgILoader.loadImage that's supposed to represent some sort of "load context" for the image. In practice, it's a document, and we should stop the silly pretense that imagelib is somehow standalone and change the signature to be nsIDocument and rename the argument.
Trying to QI an actual JSContext* wouldn't compile, since it's not nsISupports. ;)
Comment 198•10 years ago
|
||
(In reply to timeless from comment #166) > Comment on attachment 8478622 [details] [diff] [review] > 3_19_callsites_in_xpfe.patch > > TYPE_OTHER should probably be TYPE_DOCUMENT Looked at this a little more carefully. * https://mxr.mozilla.org/mozilla-central/source/xpfe/components/directory/nsDirectoryViewer.cpp#1257 The NS_NewChannel call in nsDirectoryViewerFactory::CreateInstance() only gets called if the user sets an about:config pref. Moreover, that functionality is broken (setting the pref and opening an ftp page causes tabs to continuosly open). We can use TYPE_OTHER and systemPrincipal for this. And we should probably file a bug to to remove this code. * https://mxr.mozilla.org/mozilla-central/source/xpfe/components/directory/nsDirectoryViewer.cpp#908 The NS_NewChannel call in nsHTTPIndex::FireTimer is a bit more tricky. I believe the channel is used to get data from the ftp server about the directories and files they serve. The data is read in as text/html and formatted by the browser in a xul page. Who requests this content? Is it always a top level load - if so, using systemPrincipal would be fine. Also, what do we call this ftp data? It's not quite the traditional TYPE_DOCUMENT, but it is at the top-level (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl). This feels more like TYPE_OTHER.
Updated•10 years ago
|
Attachment #8478617 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 199•10 years ago
|
||
Comment on attachment 8479350 [details] [diff] [review] 3_10_callsites_in_layout_mathml_v2.patch Since this patch is highly independent from the rest of the code updated in this bug, we land this part ahead of time in Bug 1062529. Marking it as obsolete becasue of that change.
Attachment #8479350 -
Attachment is obsolete: true
Assignee | ||
Comment 200•10 years ago
|
||
Comment on attachment 8479350 [details] [diff] [review] 3_10_callsites_in_layout_mathml_v2.patch Oh oh, that was the wrong one.
Attachment #8479350 -
Attachment is obsolete: false
Assignee | ||
Comment 201•10 years ago
|
||
Comment on attachment 8478638 [details] [diff] [review] 5_GetChannelPrincipal_changes.patch This is the right one to mark obsolete.
Attachment #8478638 -
Attachment is obsolete: true
Assignee | ||
Comment 202•10 years ago
|
||
Comment on attachment 8478621 [details] [diff] [review] 3_18_callsites_in_intl.patch Jungshik is not available to do the review for that patch, but he recommended smontagu. Any chance you can take a look at this? To get an understanding what we are trying to accomplish, please also see Comment 106.
Attachment #8478621 -
Flags: review?(jshin1987) → review?(smontagu)
Comment 203•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #126) > Created attachment 8478621 [details] [diff] [review] > 3_18_callsites_in_intl.patch > > * Does passing systemPrincipal as the loadingPrincipal make sense for these > files? Are the channels created by system operations? For > intl/strres/nsStringBundleTextOverride.cpp and intl/hyphenation/hnjstdio.cpp > its probably okay. But for /intl/strres/nsStringBundle.cpp it's less clear. I'm not sure I understand the question "Are the channels created by system operations", can you clarify that for me? nsStringBundle::LoadProperties could be called by an extension doing formatStringFromName, if that's relevant.
Addons count as "system" code still. Webpages are however not system. See comment 147 for a more detailed explanation. I'm fairly sure that all stringbudle loading counts as being triggered by the system.
Comment 205•10 years ago
|
||
Comment on attachment 8478621 [details] [diff] [review] 3_18_callsites_in_intl.patch Review of attachment 8478621 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Jonas, and apologies for not having read the whole bug report ;-)
Attachment #8478621 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 206•10 years ago
|
||
Attachment #8478590 -
Attachment is obsolete: true
Attachment #8478591 -
Attachment is obsolete: true
Attachment #8478592 -
Attachment is obsolete: true
Attachment #8478594 -
Attachment is obsolete: true
Attachment #8478595 -
Attachment is obsolete: true
Attachment #8478597 -
Attachment is obsolete: true
Attachment #8478599 -
Attachment is obsolete: true
Attachment #8478600 -
Attachment is obsolete: true
Attachment #8478601 -
Attachment is obsolete: true
Attachment #8478602 -
Attachment is obsolete: true
Attachment #8478605 -
Attachment is obsolete: true
Attachment #8478607 -
Attachment is obsolete: true
Attachment #8478608 -
Attachment is obsolete: true
Attachment #8478613 -
Attachment is obsolete: true
Attachment #8478615 -
Attachment is obsolete: true
Attachment #8478617 -
Attachment is obsolete: true
Attachment #8478620 -
Attachment is obsolete: true
Attachment #8478621 -
Attachment is obsolete: true
Attachment #8478622 -
Attachment is obsolete: true
Attachment #8478623 -
Attachment is obsolete: true
Attachment #8478624 -
Attachment is obsolete: true
Attachment #8478625 -
Attachment is obsolete: true
Attachment #8478626 -
Attachment is obsolete: true
Attachment #8478627 -
Attachment is obsolete: true
Attachment #8478628 -
Attachment is obsolete: true
Attachment #8478632 -
Attachment is obsolete: true
Attachment #8478633 -
Attachment is obsolete: true
Attachment #8478634 -
Attachment is obsolete: true
Attachment #8478635 -
Attachment is obsolete: true
Attachment #8478636 -
Attachment is obsolete: true
Attachment #8478637 -
Attachment is obsolete: true
Attachment #8479349 -
Attachment is obsolete: true
Attachment #8479350 -
Attachment is obsolete: true
Attachment #8486684 -
Flags: review+
Assignee | ||
Comment 207•10 years ago
|
||
Attachment #8486685 -
Flags: review?(mcmanus)
Assignee | ||
Comment 208•10 years ago
|
||
Attachment #8486686 -
Flags: review?(jst)
Assignee | ||
Comment 209•10 years ago
|
||
Attachment #8486687 -
Flags: review+
Assignee | ||
Comment 210•10 years ago
|
||
Attachment #8486688 -
Flags: review+
Assignee | ||
Comment 211•10 years ago
|
||
Attachment #8486689 -
Flags: review+
Assignee | ||
Comment 212•10 years ago
|
||
Attachment #8486690 -
Flags: review?(jst)
Assignee | ||
Comment 213•10 years ago
|
||
Attachment #8486691 -
Flags: review+
Assignee | ||
Comment 214•10 years ago
|
||
Attachment #8486693 -
Flags: review+
Assignee | ||
Comment 215•10 years ago
|
||
Attachment #8486694 -
Flags: review+
Assignee | ||
Comment 216•10 years ago
|
||
Attachment #8486695 -
Flags: review+
Assignee | ||
Comment 217•10 years ago
|
||
Attachment #8486696 -
Flags: review+
Assignee | ||
Comment 218•10 years ago
|
||
Attachment #8486697 -
Flags: review+
Assignee | ||
Comment 219•10 years ago
|
||
Attachment #8486698 -
Flags: review+
Assignee | ||
Comment 220•10 years ago
|
||
Attachment #8486699 -
Flags: review?(jmathies)
Assignee | ||
Comment 221•10 years ago
|
||
Attachment #8486700 -
Flags: review+
Assignee | ||
Comment 222•10 years ago
|
||
Attachment #8486701 -
Flags: review+
Assignee | ||
Comment 223•10 years ago
|
||
Attachment #8486703 -
Flags: review+
Assignee | ||
Comment 224•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8486704 -
Flags: review+
Assignee | ||
Comment 225•10 years ago
|
||
Attachment #8486706 -
Flags: review+
Assignee | ||
Comment 226•10 years ago
|
||
Attachment #8486707 -
Flags: review+
Assignee | ||
Comment 227•10 years ago
|
||
Attachment #8486708 -
Flags: review+
Assignee | ||
Comment 228•10 years ago
|
||
Attachment #8486709 -
Flags: review+
Assignee | ||
Comment 229•10 years ago
|
||
Attachment #8486710 -
Flags: review+
Assignee | ||
Comment 230•10 years ago
|
||
Attachment #8486711 -
Flags: review+
Assignee | ||
Comment 231•10 years ago
|
||
Attachment #8486712 -
Flags: review?(mwu)
Assignee | ||
Comment 232•10 years ago
|
||
Attachment #8486713 -
Flags: review+
Assignee | ||
Comment 233•10 years ago
|
||
Attachment #8486714 -
Flags: review+
Assignee | ||
Comment 234•10 years ago
|
||
Attachment #8486715 -
Flags: review+
Assignee | ||
Comment 235•10 years ago
|
||
Attachment #8486716 -
Flags: review+
Assignee | ||
Comment 236•10 years ago
|
||
Attachment #8486717 -
Flags: review+
Assignee | ||
Comment 237•10 years ago
|
||
Attachment #8486718 -
Flags: review+
Assignee | ||
Comment 238•10 years ago
|
||
Attachment #8486719 -
Flags: review+
Comment 239•10 years ago
|
||
Comment on attachment 8486712 [details] [diff] [review] 3_24_callsites_in_modules.patch Review of attachment 8486712 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libjar/nsJARChannel.cpp @@ +853,5 @@ > // kick off an async download of the base URI... > rv = NS_NewDownloader(getter_AddRefs(mDownloader), this); > + if (NS_SUCCEEDED(rv)) { > + // Since we might not have a loadinfo on all channels yet > + // we have to provide default arguments in case mLoadInfo is null; I thought this bug ensures loadinfo is set on all channels.. is there going to be a followup to ensure loadinfo is always set? @@ +863,5 @@ > + mLoadGroup, > + mCallbacks, > + mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS)); > + } > + else { Put else on the same line as }.
Attachment #8486712 -
Flags: review?(mwu) → review+
Comment 240•10 years ago
|
||
> I thought this bug ensures loadinfo is set on all channels..
... that Gecko creates.
Extensions can nsIIOService.newChannel and then not set it (and likely do just that).
Comment 241•10 years ago
|
||
s/extensions/extensions or other JS code/
Comment 242•10 years ago
|
||
Comment on attachment 8486699 [details] [diff] [review] 3_13_callsites_in_widget_windows.patch Review of attachment 8486699 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsITransferable.idl @@ +196,5 @@ > [noscript] attribute boolean isPrivateData; > > + /** > + * Only used on windows to transfer the requestingNode to the > + * loadInfo of the new channel. You added support for saving this node, its just that some platforms don't have a use for it. Lets describe what this does, with a note about its only use being on windows. Something like - The source dom node this transferable was created from. Note, currently only in use on Windows for network principal information in drag operations. ::: widget/windows/WinUtils.cpp @@ +61,5 @@ > > namespace mozilla { > namespace widget { > + > +#define ENTRY(_msg) { #_msg, _msg } nit - some sort of white space touchup here? Please check the patch over for unnecessary changes. ::: widget/windows/nsDataObj.cpp @@ +66,5 @@ > { > nsresult rv; > + rv = NS_NewChannel(getter_AddRefs(mChannel), > + pSourceURI, > + aRequestingNode, Does NS_NewChannel handle null aRequestingNode nodes gracefully? Looks like that could happen here occasionally.
Attachment #8486699 -
Flags: review?(jmathies) → review+
Comment 243•10 years ago
|
||
Comment on attachment 8486686 [details] [diff] [review] 3_01_callsites_in_content_base-html.patch - In nsXMLHttpRequest::Open(): + if (IsSystemXHR()) { + // Don't give this document the system principal. We need to keep track of + // mPrincipal being system because we use it for various security checks + // that should be passing, but the document data shouldn't get a system + // principal. + nsresult rv; rv is already declared outside of this scope, so no need to re-declare it here. r=jst
Attachment #8486686 -
Flags: review?(jst) → review+
Comment 244•10 years ago
|
||
Comment on attachment 8486690 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch r=jst, but I'd love to get bz to look over specifically the plugin and xbl bits here.
Attachment #8486690 -
Flags: review?(jst)
Attachment #8486690 -
Flags: review?(bzbarsky)
Attachment #8486690 -
Flags: review+
Comment on attachment 8486690 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch Review of attachment 8486690 [details] [diff] [review]: ----------------------------------------------------------------- I'd also like to have peterv look at the XSLT bits given that they are fairly complex.
Attachment #8486690 -
Flags: review?(peterv)
Updated•10 years ago
|
Attachment #8486695 -
Flags: review+ → review?(bzbarsky)
Updated•10 years ago
|
Attachment #8486685 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 246•10 years ago
|
||
Comment on attachment 8486695 [details] [diff] [review] 3_09_callsites_in_docshell.patch Review of attachment 8486695 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +9978,5 @@ > nsCOMPtr<nsIChannel> channel; > > bool isSrcdoc = !aSrcdoc.IsVoid(); > + > + nsCOMPtr<nsINode> requestingNode = mScriptGlobal->GetExtantDoc(); note to myself: mScriptGlobal might be null here.
Comment 247•10 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #242) > Comment on attachment 8486699 [details] [diff] [review] > 3_13_callsites_in_widget_windows.patch > ::: widget/windows/nsDataObj.cpp > @@ +66,5 @@ > > { > > nsresult rv; > > + rv = NS_NewChannel(getter_AddRefs(mChannel), > > + pSourceURI, > > + aRequestingNode, > > Does NS_NewChannel handle null aRequestingNode nodes gracefully? Looks like > that could happen here occasionally. In what situations would we get a null aRequestingNode?
Flags: needinfo?(jmathies)
Comment 248•10 years ago
|
||
Comment on attachment 8486695 [details] [diff] [review] 3_09_callsites_in_docshell.patch >+nsDocShell::createPrincipalFromReferrer(nsIURI* aReferrer, "CreatePrincipalFromReferrer". >+ nsCOMPtr<nsINode> requestingNode = mScriptGlobal->GetExtantDoc(); Er... why? Shouldn't this be the frameelement in some cases? r=me with those fixed.
Attachment #8486695 -
Flags: review?(bzbarsky) → review+
Comment 249•10 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #247) > (In reply to Jim Mathies [:jimm] from comment #242) > > Comment on attachment 8486699 [details] [diff] [review] > > 3_13_callsites_in_widget_windows.patch > > ::: widget/windows/nsDataObj.cpp > > @@ +66,5 @@ > > > { > > > nsresult rv; > > > + rv = NS_NewChannel(getter_AddRefs(mChannel), > > > + pSourceURI, > > > + aRequestingNode, > > > > Does NS_NewChannel handle null aRequestingNode nodes gracefully? Looks like > > that could happen here occasionally. > > In what situations would we get a null aRequestingNode? Someone fails to set the requesting node? Maybe the safe thing to do here is return failure if aRequestingNode is null on entry to nsDataObj::CStream::Init(). We handle that gracefully.
Flags: needinfo?(jmathies)
Comment 250•10 years ago
|
||
Comment on attachment 8486690 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch In Navigator::MozIsLocallyAvailable, what should happen when doc is null? Right now we'll crash with this patch as-is, right? That seems dubious. I'm not sure in which situation we would not have a "doc" in the pluginhost code. Using a nullprincipal there is probably fine, given that. But you're not passing in the nullprincipal you create; you're passing in "doc", no? And if that's null, we'll crash. Same for nsPluginStreamListenerPeer. Why is nsPluginStreamListenerPeer getting SEC_INHERIT_PRINCIPAL? That doesn't match the old behavior, afaict. >+ // This is because the principal and node could be different. (BZ, can you confirm?) I believe so, yes. The origin principal will be that of the stylesheet that had the -moz-binding declaration, while the node is the bound document. That said, I'm not sure about passing in the bound document at all here: we don't want to consider it in security checks, because we want to allow chrome to bind stuff in content documents, right? Please ask Jonas or Peter to look at the xslt changes; I didn't really look at those.
Attachment #8486690 -
Flags: review?(bzbarsky) → review-
Updated•10 years ago
|
Comment on attachment 8486684 [details] [diff] [review] 1_loadinfo_changes.patch Review of attachment 8486684 [details] [diff] [review]: ----------------------------------------------------------------- Looks good otherwise. ::: docshell/base/nsILoadInfo.idl @@ +25,5 @@ > + const unsigned long SEC_NORMAL = 0; > + > + /** > + * Inherit the Principal. If this flag is set, then > + * SEC_SANDBOXED can *not* be set. We should say what setting this flag actually does. I would say something like: Force inheriting of the Principal. The resulting resource will use the principal of the document which is doing the load. Setting this flag will cause GetChannelResultPrincipal to return the same principal as the loading principal that's passed in when creating the channel. This will happen independently of the scheme of the URI that the channel is loading. So if the loading document comes from "http://a.com/", and the channel is loading the URI "http://b.com/whatever", GetChannelResultPrincipal will return a principal from "http://a.com/". This flag can not be used together with SEC_SANDBOXED. @@ +27,5 @@ > + /** > + * Inherit the Principal. If this flag is set, then > + * SEC_SANDBOXED can *not* be set. > + */ > + const unsigned long SEC_INHERIT_PRINCIPAL = 0x01; I'd like to name this SEC_FORCE_INHERIT_PRINCIPAL. To set it apart from "normal" inheritance, like how data: can inherit the principal of the loader. @@ +31,5 @@ > + const unsigned long SEC_INHERIT_PRINCIPAL = 0x01; > + > + /** > + * Sandbox the load. If this flag is set, then > + * SEC_INHERIT_PRINCIPAL can *not* be set. Maybe something like: Sandbox the load. The resulting resource will use a freshly created null principal. So GetChannelResultPrincipal will always return a null principal whenever this flag is set. This will happen independently of the scheme of the URI that the channel is loading. This flag can not be used together with SEC_INHERIT_PRINCIPAL. @@ +57,5 @@ > > + > + /** > + * The loadingDocument of the channel, used for security checks > + * like Mixed Content Blocker, and CSP. The loadingDocument of a channel is I'd leave out the "used for security checks like MCB and CSP" part. Lots of properties on nsILoadInfo will be used for security checks. @@ +59,5 @@ > + /** > + * The loadingDocument of the channel, used for security checks > + * like Mixed Content Blocker, and CSP. The loadingDocument of a channel is > + * the document that requested the load of the resource that is tied > + * to this channel. It is *NOT* the resource itself, it's the Leave out "that is tied to this channel". I think that part is clear but is making the sentence hard to read. @@ +65,5 @@ > + * Script Example: Assume a document who's origin is http://a.com > + * embeds a script from http://b.com. The loadingDocument for the channel > + * associated with the http://b.com script load is the document with origin > + * http://a.com > + * Frame Example: Assume a document with origin http://a.com embeds a frame Say "iframe" rather than "frame". The word "frame" can mean very many different things. You might even want to say "<script> example" and "<iframe> example" in the titles above (though no need to say it in the text). @@ +66,5 @@ > + * embeds a script from http://b.com. The loadingDocument for the channel > + * associated with the http://b.com script load is the document with origin > + * http://a.com > + * Frame Example: Assume a document with origin http://a.com embeds a frame > + * from http://b.com. The loadingDocument for the channel associated with Rather than saying "a frame from http://b.com", say "a <iframe src="http://b.com>" @@ +67,5 @@ > + * associated with the http://b.com script load is the document with origin > + * http://a.com > + * Frame Example: Assume a document with origin http://a.com embeds a frame > + * from http://b.com. The loadingDocument for the channel associated with > + * the http://b.com frame load is the document who's origin is http://a.com. Rather than "frame load" say "network request". @@ +72,5 @@ > + * Now assume the frame to http://b.com then further embeds a script from > + * http://c.com. The loadingDocument for the channel associated with the > + * http://c.com script load is the frame with origin http://b.com. > + * > + * Warning: The loadingDocument can be null! Are we enforcing that loadingDocument is only null when loadingPrincipal is the system principal? If so we should say that here. If not we should try to enforce that in later patches in followup bugs.
Attachment #8486684 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8486690 -
Flags: review?(jonas)
Assignee | ||
Comment 252•10 years ago
|
||
Comment on attachment 8486690 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch As discussed on IRC, Peter will have a look tomorrow morning. No need for Jonas to review it today then. Cancelling review request.
Attachment #8486690 -
Flags: review?(jonas)
Peterv: The goal is that for loading stylesheets, the document that we pass in should be the source document that's being transformed. The principal that we pass in should be the principal of whoever linked to the stylesheet. So either the principal of the stylesheet that contains the <xsl:import>/<xsl:include>, or the principal of the source document for <?xml-stylesheet href=...?>. For document() loads, I guess we should pass in the source document as the document, and the principal of the stylesheet that contained the xpath expression as the principal.
Comment on attachment 8486690 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch Review of attachment 8486690 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xml/nsXMLPrettyPrinter.cpp @@ +100,5 @@ > NS_LITERAL_CSTRING("chrome://global/content/xml/XMLPrettyPrint.xsl")); > NS_ENSURE_SUCCESS(rv, rv); > > nsCOMPtr<nsIDOMDocument> xslDocument; > + rv = nsSyncLoadService::LoadDocument(xslUri, nsContentUtils::GetSystemPrincipal(), nullptr, true, This line is too long. Limit to 80 columns. ::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp @@ -369,3 @@ > public: > txCompileObserver(txMozillaXSLTProcessor* aProcessor, > nsILoadGroup* aLoadGroup); I suspect that we need to change the flow here. We should probably change nsIDocumentTransformer.init to take an nsIDocument rather than an nsIPrincipal. Then change txMozillaXSLTProcessor::LoadStyleSheet, TX_LoadSheet and txCompileObserver::txCompileObserver to not take a loadgroup. Then change txCompileObserver::startLoad to get the document from mProcessor and get both principal and loadgroup from that document. Then pass principal, loadgroup and document to NS_NewChannel I haven't looked at all the other code paths yet.
Comment 255•10 years ago
|
||
Comment on attachment 8486690 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch Review of attachment 8486690 [details] [diff] [review]: ----------------------------------------------------------------- -'ing because the change in TX_LoadSheet doesn't actually do anything and it seems you expect it to. ::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp @@ +433,5 @@ > if (NS_CP_REJECTED(shouldLoad)) { > return NS_ERROR_DOM_BAD_URI; > } > > + // ckerschb: we shouldn't use nullptr for the node, can we get it somehow? There often is no node, because we don't create nodes for the stylesheet. I don't know what these nodes are used for, but if we really need one and we don't actually care that it's not the stylesheet-loading element, then we could use the source document. That would mean storing it in the txCompileObserver, passed in from TX_LoadSheet through the txCompileObserver constructor. @@ +534,5 @@ > nsRefPtr<txStylesheetCompiler> compiler = > new txStylesheetCompiler(NS_ConvertUTF8toUTF16(spec), observer); > NS_ENSURE_TRUE(compiler, NS_ERROR_OUT_OF_MEMORY); > > + nsCOMPtr<nsINode> requestingNode(do_QueryInterface(aProcessor->GetSourceContentModel())); AFAICT this will be null, since we call LoadStyleSheet (and TX_LoadSheet) before we call SetSourceContentModel. You'll probably have to pass mDocument from nsXMLContentSink::LoadXSLStyleSheet to LoadStyleSheet and on to TX_LoadSheet.
Attachment #8486690 -
Flags: review?(peterv) → review-
Comment 256•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #253) > So either the principal of the stylesheet that contains the > <xsl:import>/<xsl:include>, or the principal of the source document for > <?xml-stylesheet href=...?>. We use the source document's principal for <?xml-stylesheet href=...?> and GetSimpleCodebasePrincipal on the referrer uri to calculate the principal for <xsl:import>/<xsl:include>, so that part is ok already? > For document() loads, I guess we should pass in the source document as the > document, and the principal of the stylesheet that contained the xpath > expression as the principal. Passing in the document would mean changing nsSyncLoadService::LoadDocument so that we can pass it in. And we'd need a way to communicate the principal to txParseDocumentFromURI (storing it in the DocumentFunctionCall?).
Assignee | ||
Comment 257•10 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #255) > 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch > > Review of attachment 8486690 [details] [diff] [review]: > ----------------------------------------------------------------- > > -'ing because the change in TX_LoadSheet doesn't actually do anything and it > seems you expect it to. The primary goal for all the patches is to basically duplicate current logic and put the same information (node,principal) that we use to call contenPolicies into the LoadInfo. > > + // ckerschb: we shouldn't use nullptr for the node, can we get it somehow? > There often is no node, because we don't create nodes for the stylesheet. I > don't know what these nodes are used for, but if we really need one and we > don't actually care that it's not the stylesheet-loading element, then we > could use the source document. That would mean storing it in the > txCompileObserver, passed in from TX_LoadSheet through the txCompileObserver > constructor. Right, there is no node in that case. http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#428 I guess my comment was misleading. For now I suggest we use 'nullptr' and file a followup to actually should pass the source-document around and also use it for contentPolicies. In general, your suggestion seems right to me, so let's incorporate that change in the follow up. > @@ +534,5 @@ > > nsRefPtr<txStylesheetCompiler> compiler = > > new txStylesheetCompiler(NS_ConvertUTF8toUTF16(spec), observer); > > NS_ENSURE_TRUE(compiler, NS_ERROR_OUT_OF_MEMORY); > > > > + nsCOMPtr<nsINode> requestingNode(do_QueryInterface(aProcessor->GetSourceContentModel())); > > AFAICT this will be null, since we call LoadStyleSheet (and TX_LoadSheet) > before we call SetSourceContentModel. You'll probably have to pass mDocument > from nsXMLContentSink::LoadXSLStyleSheet to LoadStyleSheet and on to > TX_LoadSheet. If that is indeed null, than it's also null in the contentPolicy check right above: http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#514 which means we are still duplicating current logic, which seems fine at the moment, but of course we will follow up on it. Again, your suggestion seems right to me in that case as well. To sum it up, we will file a follow up where in the end we will use the same document/node that we use for contentPolicies in our loadInfo.
Comment 258•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #257) > If that is indeed null, than it's also null in the contentPolicy check right > above: > http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/ > txMozillaStylesheetCompiler.cpp#514 > which means we are still duplicating current logic, which seems fine at the > moment, but of course we will follow up on it. It's possible that the existing logic is buggy, my suggestion is to actually fix it. But if you don't want to do that then you should explicitly pass in null instead of pretending that we're passing in something by calling GetSourceContentModel.
Assignee | ||
Comment 259•10 years ago
|
||
As discussed on IRC and over email, let's just call NS_NewChannel[Principal]() for now in xslt/txMozillaStylesheetCompiler.cpp leaving the nullptr Node for a future patch. I filed Bug 1067517 where we can follow up on the problem.
Attachment #8486690 -
Attachment is obsolete: true
Attachment #8489542 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8489542 -
Flags: review?(bzbarsky)
Comment 260•10 years ago
|
||
The following followup bugs were filed: * https://bugzilla.mozilla.org/show_bug.cgi?id=1066833 - followup to determine if TYPE_IMAGE, TYPE_STYLESHEET, and TYPE_SCRIPT callsites should pass the loading element instead or in additon to the loading document (or at least to try and figure out what they should take) * https://bugzilla.mozilla.org/show_bug.cgi?id=1066843 - nsISound::Play shouldn't pass systemprincipal * https://bugzilla.mozilla.org/show_bug.cgi?id=1066868 - fix the way we get principal/context in nsDocShell file followup bug for docshell * https://bugzilla.mozilla.org/show_bug.cgi?id=1066826 - Remove mozislocallyavailable api from navigator.cpp * https://bugzilla.mozilla.org/show_bug.cgi?id=1067517 - followup from comment 254-259 to fix code in txMozillaStylesheetCompiler.cpp
Comment 261•10 years ago
|
||
Comment on attachment 8489542 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch Why do we need the copy/paste bits in the plugin code? Why not just use NS_NewChannelInternal and pass both doc and principal? Or does that expect a non-null doc?
Assignee | ||
Comment 262•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #261) > Why do we need the copy/paste bits in the plugin code? Why not just use > NS_NewChannelInternal and pass both doc and principal? Or does that expect > a non-null doc? The doc is allowed to be null if we call NS_NewChannelInternal(). The ultimate goal here is to call NS_NewChannel[Node] wherever we can and completely eliminate NS_NewChannelInternal to be called from anything outside of nsNetutil.h at some point. If you want we can call NS_NewChannlInternal() getting the principal of the doc, if there is one, and otherwise use the nullPrincipal. To answer your question, we agreed that we should try to avoid calling NS_NewChannelInternal() wherever we can. That's the only reason.
Comment 263•10 years ago
|
||
> If you want we can call NS_NewChannlInternal() getting the principal of the doc, if there > is one, and otherwise use the nullPrincipal.
I'd prefer that to having two separate NS_NewChannel calls, I think.
And please file a followup to simplify this if/when we can prove that the doc is in fact never null here.
Assignee | ||
Comment 264•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #263) > I'd prefer that to having two separate NS_NewChannel calls, I think. Sure thing - here it is. > And please file a followup to simplify this if/when we can prove that the > doc is in fact never null here. Sure will do.
Attachment #8489542 -
Attachment is obsolete: true
Attachment #8489542 -
Flags: review?(peterv)
Attachment #8489542 -
Flags: review?(bzbarsky)
Attachment #8489810 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8489810 -
Flags: review?(peterv)
Comment 265•10 years ago
|
||
Comment on attachment 8489810 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch r+ on the XSLT/pretty printer bits.
Attachment #8489810 -
Flags: review?(peterv) → review+
Comment 266•10 years ago
|
||
Comment on attachment 8489810 [details] [diff] [review] 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch r=me on the plugin/dom/xbl bits.
Attachment #8489810 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 267•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=4120e1cbb5b4
Target Milestone: --- → mozilla35
Comment 268•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=4120e1cbb5b4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 269•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #197) > > Isn't aCX a JS-context? > > No. It's the random nsISupports argument to imgILoader.loadImage that's > supposed to represent some sort of "load context" for the image. In > practice, it's a document, and we should stop the silly pretense that > imagelib is somehow standalone and change the signature to be nsIDocument > and rename the argument. > > Trying to QI an actual JSContext* wouldn't compile, since it's not > nsISupports. ;) Filed followup bug https://bugzilla.mozilla.org/show_bug.cgi?id=1073179 for imgLoader to switch from nsISupports to nsIDocument
Comment 270•10 years ago
|
||
Can someone here look at Bug 1073838 - TB build bustage (comm-central): NS_NewStreamLoader and NS_NewChannel and see what changes are necessary there to compile TB successfully? TIA
Comment 271•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d0f7f15e15fb https://hg.mozilla.org/mozilla-central/rev/d20696b5c309 https://hg.mozilla.org/mozilla-central/rev/666d31a3753a https://hg.mozilla.org/mozilla-central/rev/2e52e8387850 https://hg.mozilla.org/mozilla-central/rev/8511e9341536 https://hg.mozilla.org/mozilla-central/rev/8175d676cc6f https://hg.mozilla.org/mozilla-central/rev/a12b926cff35 https://hg.mozilla.org/mozilla-central/rev/b968440b6204 https://hg.mozilla.org/mozilla-central/rev/5609cb03aacc https://hg.mozilla.org/mozilla-central/rev/cc656384d43e https://hg.mozilla.org/mozilla-central/rev/0dabb0a747fc https://hg.mozilla.org/mozilla-central/rev/2d6e88e962a7 https://hg.mozilla.org/mozilla-central/rev/7fc6512d34b1 https://hg.mozilla.org/mozilla-central/rev/de85d97eac27 https://hg.mozilla.org/mozilla-central/rev/55806e3e450f https://hg.mozilla.org/mozilla-central/rev/20bb35fc802a https://hg.mozilla.org/mozilla-central/rev/4afe0fb6dd86 https://hg.mozilla.org/mozilla-central/rev/1a1712c6b142 https://hg.mozilla.org/mozilla-central/rev/2382ff174f22 https://hg.mozilla.org/mozilla-central/rev/218ea09a7286 https://hg.mozilla.org/mozilla-central/rev/3910656e0a2c https://hg.mozilla.org/mozilla-central/rev/da34da9570d2 https://hg.mozilla.org/mozilla-central/rev/5ea42524c416 https://hg.mozilla.org/mozilla-central/rev/ecc3d0491803 https://hg.mozilla.org/mozilla-central/rev/d4ced32fc77b https://hg.mozilla.org/mozilla-central/rev/02bc405eeb7a https://hg.mozilla.org/mozilla-central/rev/49775ca3647f https://hg.mozilla.org/mozilla-central/rev/5b066ef38ccf https://hg.mozilla.org/mozilla-central/rev/d6bd6192791b https://hg.mozilla.org/mozilla-central/rev/8dbb44fb03c1 https://hg.mozilla.org/mozilla-central/rev/6812665c79b3 https://hg.mozilla.org/mozilla-central/rev/9ddccdec7224 https://hg.mozilla.org/mozilla-central/rev/68e95822939b https://hg.mozilla.org/mozilla-central/rev/c09922195848 https://hg.mozilla.org/mozilla-central/rev/4120e1cbb5b4
Updated•9 years ago
|
Blocks: CVE-2015-0809
Comment 272•9 years ago
|
||
Not sure if it's worth a new bug, but there's a typo in one of the patches here: https://hg.mozilla.org/mozilla-central/rev/666d31a3753a#l17.40 This check duplicates the one right above it and was probably supposed to be removed along with the SetLoadInfo call: 17.52 - rv = channel->SetLoadInfo(loadInfo); 17.53 if (rv.Failed()) { 17.54 return nullptr; 17.55 }
Assignee | ||
Comment 273•9 years ago
|
||
(In reply to Nickolay_Ponomarev from comment #272) > Not sure if it's worth a new bug, but there's a typo in one of the patches > here: https://hg.mozilla.org/mozilla-central/rev/666d31a3753a#l17.40 > > This check duplicates the one right above it and was probably supposed to be > removed along with the SetLoadInfo call: > 17.52 - rv = channel->SetLoadInfo(loadInfo); > 17.53 if (rv.Failed()) { > 17.54 return nullptr; > 17.55 } Yes, that is a typo and yes, I don't think we need to file a new bug for the removal of those 3 lines. Thanks for pointing it out.
Attachment #8601219 -
Flags: review?(jonas)
Attachment #8601219 -
Flags: review?(jonas) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•