Closed Bug 1199295 Opened 4 years ago Closed 4 years ago

Investigate loadingPrincipal for loadInfo in uriloader/prefetch

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(7 files, 10 obsolete files)

11.32 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
24.23 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
1.43 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
3.50 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
1.33 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.12 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
50.02 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
AS smaug points out [1] we should consider passing a different loadingPrincipal in uriloader/prefetch. Maybe we can find a better principal than the systemPrincipal for those cases [2]. As of know it's totally fine to use the systemPrincipal, because these callsites didn't perform any security checks anyway, but maybe they should. Long story short, using systemPrincipal does not make things worse, but maybe we could improve security here.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1192948#c5
[2] http://hg.mozilla.org/mozilla-central/diff/d4ced32fc77b/uriloader/prefetch/nsOfflineCacheUpdate.cpp
Blocks: 1182535
Can someone explain what uriloader/prefetch does? I.e. what feature of Firefox is it used for?
(In reply to Jonas Sicking (:sicking) from comment #1)
> Can someone explain what uriloader/prefetch does? I.e. what feature of
> Firefox is it used for?

Perhaps Honza or smaug can answer this.
Flags: needinfo?(honzab.moz)
uriloader/prefetch does two things:
- offline application cache update processing (nsOfflineCacheUpdate and Service), something we still maintain
- prefetch service (used from nsContentSink, and derivatives) for preloading (precaching) hrefs and other stuff (how exactly this works and whether it's actually used I really don't know and I'm no expert to that exact code)
Flags: needinfo?(honzab.moz)
For both of those we need to get a correct principal as well as correct security flags and nsContentPolicyType.

So I think we need to require all callers to pass that information in. Fortunately it should also mean that we can remove security checks at those callsites.
(In reply to Jonas Sicking (:sicking) from comment #4)
> For both of those we need to get a correct principal as well as correct
> security flags and nsContentPolicyType.
> 
> So I think we need to require all callers to pass that information in.
> Fortunately it should also mean that we can remove security checks at those
> callsites.

Alrighty, let's have a look what we can do here.
Assignee: nobody → mozilla
Jonas, here is a first attempt of replacing mDocumentURI with mLoadingPrincipal and hence passing the loadingPrincipal all the way down to uriloader/prefetch where we actually create the channel.

What do you think about that approach? Before doing any more work here I would like you to confirm this is an approach we are considering and if we also need to pass down other information, like e.g. a contentType.
Attachment #8658352 - Flags: feedback?(jonas)
Comment on attachment 8658352 [details] [diff] [review]
bug_1199295_uriloader_prefetch.patch

Review of attachment 8658352 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/offline/nsDOMOfflineResourceList.cpp
@@ +454,5 @@
>  {
>    nsresult rv = Init();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // i think we can remove that check

why?

::: uriloader/prefetch/nsIOfflineCacheUpdate.idl
@@ +194,5 @@
>     */
>    readonly attribute uint64_t byteProgress;
>  };
>  
> +[scriptable, uuid(a297a334-bcae-4779-a564-555593edc96b)]

note that I'm modifying this code as well right now in bug 1165256

@@ +271,5 @@
>       *        The pref branch to use to check the
>       *        offline-apps.allow_by_default pref.  If not specified,
>       *        the pref service will be used.
>       */
> +    // maybe remove?

why should these be removed?
Blocks: 1165256
Jonas, what is the time frame for this bug?  I would like to base a patch for one of the NSec bugs on patches proposed here.  Also to avoid conflicts and save some work (bug 1165256 may use rewrite anyway after this lands).  

There is no critical need tho, I already have a patch doing what I want for that bug.
Flags: needinfo?(jonas)
Comment on attachment 8658352 [details] [diff] [review]
bug_1199295_uriloader_prefetch.patch

Review of attachment 8658352 [details] [diff] [review]:
-----------------------------------------------------------------

My understanding is that the spec requires the manifest to be same-origin with the document that links to the manifest. Is that correct?

::: dom/base/nsContentSink.cpp
@@ +1120,1 @@
>      rv = mDocument->NodePrincipal()->CheckMayLoad(manifestURI, true, false);

If this is just there to ensure that we enforce that the manifest is same-origin with the document that links to it, then yeah, I think we can remove this if you make the other changes below.

@@ +1123,5 @@
>      }
>      else {
>        // Only continue if the document has permission to use offline APIs or
>        // when preferences indicate to permit it automatically.
> +      // not sure if we can remove that check

I don't think we can remove these. It looks to me like this is enforcing things other than origin policies.

::: dom/base/nsGlobalWindow.cpp
@@ +4424,5 @@
>  
>      nsCOMPtr<nsIURI> manifestURI;
>      nsContentUtils::GetOfflineAppManifest(mDoc, getter_AddRefs(manifestURI));
>  
> +    // uri is different from mDoc->NodePrincipal()

Different how? And when?

::: uriloader/prefetch/nsIOfflineCacheUpdate.idl
@@ +271,5 @@
>       *        The pref branch to use to check the
>       *        offline-apps.allow_by_default pref.  If not specified,
>       *        the pref service will be used.
>       */
> +    // maybe remove?

I don't think these can be removed. These enforce different policies than network policies.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +181,1 @@
>                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

This should use SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED I think, no?

@@ +181,2 @@
>                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
>                         nsIContentPolicy::TYPE_OTHER,

Technically we should have a separate type for appcache manifests I think, but I suspect that is not worth adding since we are deprecating this code anyway.

@@ +374,1 @@
>                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

Is this for loading the resources pointed to by the manifest? If so using SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is probably correct.
Attachment #8658352 - Flags: feedback?(jonas) → feedback+
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #8)
> Jonas, what is the time frame for this bug?  I would like to base a patch
> for one of the NSec bugs on patches proposed here.  Also to avoid conflicts
> and save some work (bug 1165256 may use rewrite anyway after this lands).  
> 
> There is no critical need tho, I already have a patch doing what I want for
> that bug.

I'll let you and Christoph decide this since the two of you are the ones writing the code. I'll try to stay on top of feedback requests, though these changes are complex enough that someone else should probably review.
Flags: needinfo?(jonas)
Christoph, are you OK with making this bug block a priority bug for which deadline is Oct 02?
Flags: needinfo?(mozilla)
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #11)
> Christoph, are you OK with making this bug block a priority bug for which
> deadline is Oct 02?

I am fine with moving this bug to the front of my priority queue. Let's hope that we get review requests in quite quickly.

Jonas, from your feedback I suspect you are fine with just passing the principal all the way do the point where we create the new channel, but just to make sure, nothing else besides the principal, right?
Flags: needinfo?(mozilla) → needinfo?(jonas)
We need to have access to all the normal information. So if we at the callsite know what we are loading, and thus can hardcode things like security flags and contentpolicytype, then simply passing down the principal sounds fine.

But in comment 3 Honza mentioned that this code is also used for prefetching resources that we detect during HTML parsing (at least that was my understanding of comment 3), then I would expect that we need to use different policies for different types of resources that are being prefetched.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #13)
> We need to have access to all the normal information. So if we at the
> callsite know what we are loading, and thus can hardcode things like
> security flags and contentpolicytype, then simply passing down the principal
> sounds fine.
> 
> But in comment 3 Honza mentioned that this code is also used for prefetching
> resources that we detect during HTML parsing (at least that was my
> understanding of comment 3), then I would expect that we need to use
> different policies for different types of resources that are being
> prefetched.

If that is the case, then it would make sense to pass an 'nsILoadinfo*' as an argument rather than an nsIPrincipal. Honza, what do you think?
Flags: needinfo?(honzab.moz)
I'm OK with both.  What is the time frame for a first patch I could start building patch for my bug 1165256 on?
Flags: needinfo?(honzab.moz)
It's hard for me to tell if a LoadInfo is the right thing to use for the prefetch code. Where is that code?

The current patch looks fine, no need to use a LoadInfo there, right?
(In reply to Jonas Sicking (:sicking) from comment #16)
> It's hard for me to tell if a LoadInfo is the right thing to use for the
> prefetch code. Where is that code?

I honestly don't think that any of this code is used for prefetching resources by the HTML parser (if at all we interpret comment 3 correctly and that's what Honza is thinking). Wouldn't any of my changes cause compile errors if that is the case?

> The current patch looks fine, no need to use a LoadInfo there, right?

So far no need for passing a LoadInfo, I agree. I think we are fine just passing down the principal. Who would be a good reviewer for this patch?
I agree with the feedback comments from Honza (Comment 7) and Jonas (Comment 9) and we can't remove those security checks from the *.idl and also not from the code, because we don't end up creating a new channel in all of those cases.

I think what we should do is only pass the loadingPrincipal instead of the uri all the way down. So we end up having the correct loadingPrincipal on the channel and leave everything else untouched.
Attachment #8658352 - Attachment is obsolete: true
Note first comment in comment 9. I think one of the checks can be removed.
According a reviewer - definitely the offline cache changes should go through me.  The other code depends.  Since I really don't know how it works, probably someone else should do that.  No idea who, tho.
Status: NEW → ASSIGNED
Alright, lets give this a first spin, just replacing the uri with the principal.
Attachment #8660179 - Attachment is obsolete: true
Flags: needinfo?(honzab.moz)
(In reply to Jonas Sicking (:sicking) from comment #9)
> My understanding is that the spec requires the manifest to be same-origin
> with the document that links to the manifest. Is that correct?
> 
> ::: dom/base/nsContentSink.cpp
> @@ +1120,1 @@
> >      rv = mDocument->NodePrincipal()->CheckMayLoad(manifestURI, true, false);
> 
> If this is just there to ensure that we enforce that the manifest is
> same-origin with the document that links to it, then yeah, I think we can
> remove this if you make the other changes below.

I think that is the case, but smaug probably knows better if it's safe to remove CheckMayLoad there.
Attachment #8663236 - Flags: review?(bugs)
Comment on attachment 8663236 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_dom_changes.patch

So doesn't this change the behavior, because we move the sameOrigin check to happen later.
If the manifest isn't same origin, we don't call SelectDocAppCacheNoManifest anymore. I wouldn't change the behavior of this fragile, deprecated code.
Or explain why this patch wouldn't change the behavior.

In other words, I'm not familiar enough with this code to be ready to say that such behavioral changes are ok.

The security check was added in https://bugzilla.mozilla.org/show_bug.cgi?id=402272 in GetSecurityManager()->CheckSameOriginURI form.

And if the check failed, calling SelectDocAppCacheNoManifest was added in
https://bugzilla.mozilla.org/show_bug.cgi?id=460353
Based on the assignee of the latter bug, mayhemer clearly might have some opinion on that.
Attachment #8663236 - Flags: review?(bugs) → review-
Is there any harm to leave that mDocument->NodePrincipal()->CheckMayLoad there?
(In reply to Olli Pettay [:smaug] from comment #23)
> If the manifest isn't same origin, we don't call SelectDocAppCacheNoManifest
> anymore. I wouldn't change the behavior of this fragile, deprecated code.

I don't think there is any harm in leaving the additional same origin check there. The only downside is that SOP is (potentially) enforced twice, once on the callsite [mDocument->NodePrincipal()->CheckMayLoad] and once within AsyncOpen2().

As you mentioned, it's fragile code, and also deprecated code. I would rather not update those fragile bits and leave the [mDocument->NodePrincipal()->CheckMayLoad] instead of debugging all the cases where we potentially do not need the checkMayLoad case on the callsite. Let's just wait till it's completely deprecated - can we all live with that?
Keeping the CheckMayLoad does mean that we end up with different fallback behavior for different form of network policy violations. For example it means that having the original URL be cross-origin is treated differently from for example:

* A redirect to the same cross-origin URL.
* A URL which violates CSP.
* A URL which violates some security checks done by an addon installed by the user.

So generally speaking it's better to let the channel do all error checking and then call fallback handling if the channel indicates an error.

That said, I agree that since this code is about to be deprecated, I think trying to fix it up to handle the above scenarios perfectly is no worth it. So just keeping the CheckMayLoad call seems fine with me.
(In reply to Jonas Sicking (:sicking) from comment #26)
> That said, I agree that since this code is about to be deprecated, I think
> trying to fix it up to handle the above scenarios perfectly is no worth it.
> So just keeping the CheckMayLoad call seems fine with me.

I totally agree with that in general we should perform security checks only in one place, but since this code is about to be deprecated I am all in favor of not going trhough the trouble of fixing up that fragile piece of code. Flagging smaug again for review with the security check kept in place.
Attachment #8663236 - Attachment is obsolete: true
Attachment #8663776 - Flags: review?(bugs)
Comment on attachment 8663776 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_dom_changes.patch


>-      updateService->ScheduleOnDocumentStop(manifestURI, mDocumentURI, domdoc);
>+      updateService->ScheduleOnDocumentStop(manifestURI, mDocument->NodePrincipal(), domdoc);
So we do use document uri somewhere under ScheduleOnDocumentStop, so better to verify this handling is right.
Note, iframe which loads data: has its parent's principal, but data: as documentURI, so using parent's principal's uri
as the document uri for the iframe would be rather big change, and I can't see how that could be right.
Attachment #8663776 - Flags: review?(bugs)
Thanks smaug for the feedback and I think you are right. In that case we just extend the argument list to pass the loadingPrincipal all the way down to the point where we create the channel.
Attachment #8663776 - Attachment is obsolete: true
Attachment #8664035 - Flags: review?(bugs)
Honza, we are going to keep this real simple and just pass the principal all the way down instead of any other modifications.
Attachment #8663235 - Attachment is obsolete: true
Comment on attachment 8664035 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_dom_changes.patch

>@@ -4432,17 +4432,17 @@ nsGlobalWindow::GetApplicationCache(Erro
>     if (aError.Failed()) {
>       return nullptr;
>     }
> 
>     nsCOMPtr<nsIURI> manifestURI;
>     nsContentUtils::GetOfflineAppManifest(mDoc, getter_AddRefs(manifestURI));
> 
>     nsRefPtr<nsDOMOfflineResourceList> applicationCache =
>-      new nsDOMOfflineResourceList(manifestURI, uri, this);
>+      new nsDOMOfflineResourceList(manifestURI, uri, mDoc->NodePrincipal(), this);
Technically nothing guarantees mDoc isn't null here. So, could you add a 
if (!mDoc) {
  aError.Throw(NS_ERROR_FAILURE);
  return nullptr;
}
somewhere here. Or perhaps just add || !mDoc to
if (!webNav) {


>     virtual POfflineCacheUpdateChild* AllocPOfflineCacheUpdateChild(
>             const URIParams& manifestURI,
>             const URIParams& documentURI,
>+            const PrincipalInfo& loadingPrincipalInfo,
>             const bool& stickDocument,
>             const TabId& aTabId) override;
Mumbling about this code not using Mozilla coding style. I wouldn't might if you used aFoo for the new param, given the already
inconsistency
Attachment #8664035 - Flags: review?(bugs) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Created attachment 8664037 [details] [diff] [review]
> bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch
> 
> Honza, we are going to keep this real simple and just pass the principal all
> the way down instead of any other modifications.

Fantastic.  I can work with that really well, since it's easy to get OriginAttributes from nsIPrincipal.  Thanks!

I'll take a look at the patch soon.
Flags: needinfo?(honzab.moz)
Attachment #8664037 - Flags: review?(honzab.moz)
Comment on attachment 8664037 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch

Review of attachment 8664037 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/prefetch/OfflineCacheUpdateChild.cpp
@@ +440,5 @@
>      // Need to addref ourself here, because the IPC stack doesn't hold
>      // a reference to us. Will be released in RecvFinish() that identifies 
>      // the work has been done.
>      ContentChild::GetSingleton()->SendPOfflineCacheUpdateConstructor(
> +        this, manifestURI, documentURI, loadingPrincipalInfo,

the appropriate IPDL change should be in this patch, not in the dom parts.  but that is more about how badly the code is organized.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1249,5 @@
>          do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      mDocumentURI = aDocumentURI;
> +    mLoadingPrincipal = aLoadingPrincipal;

redundant?
Attachment #8664037 - Flags: review?(honzab.moz) → review+
Carrying over r+ from Honza.

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #33)
> the appropriate IPDL change should be in this patch, not in the dom parts. 
> but that is more about how badly the code is organized.

Initially I split the patches based on directories. Since the *.ipdl is within dom/ that made sense, but I agree with you, hence I moved the *.ipdl into that patch.
 
> ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
> @@ +1249,5 @@
> > +    mLoadingPrincipal = aLoadingPrincipal;
> 
> redundant?

Indeed, redundant, because we call initInternal right above. I think that initialization of mDocumentURI confused me. Anyway, removed the additional initialization of mLoadingPrincipal.
Attachment #8664037 - Attachment is obsolete: true
Attachment #8664617 - Flags: review+
Carrying over r+ from smaug.

(In reply to Olli Pettay [:smaug] from comment #31)
> Technically nothing guarantees mDoc isn't null here. So, could you add a 

Thanks for catching that. I added the proposed null-check.

> Mumbling about this code not using Mozilla coding style. I wouldn't might if
> you used aFoo for the new param, given the already inconsistency

Yeah, I don't like that either, so I changed all the code I added to use the right codingStyle including the leading 'a' as a prefix for all the incoming arguments.
Attachment #8664035 - Attachment is obsolete: true
Attachment #8664620 - Flags: review+
Honza, when running dom/apps/tests/test_app_update.html I realized we also have to propagate the loadingPrincipal for InitPartial(). Once r+ed, I will qfold that patch into the other one you already reviewed.
Flags: needinfo?(honzab.moz)
Jonas, do you feel comfortable reviewing those callsites? Probably 'dom/apps/Webapps.jsm' is the most tricky one, but we are already using the same technique of creating the principal within that file which gives me more confidence that this is the way to do it.
Attachment #8665782 - Flags: review?(jonas)
Honza, there are also quite so many netwerk tests, I suppose using the systemPrincipal is fine.
Comment on attachment 8665782 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_callsites_dom_b2g_browser_mobile.patch

Review of attachment 8665782 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think we need the owners of the individual pieces of code here to review this code.
Attachment #8665782 - Flags: review?(jonas)
Comment on attachment 8666136 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_callsites_browser.patch

Review of attachment 8666136 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8666136 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8666135 - Flags: review?(fabrice) → review+
Attachment #8666138 - Flags: review?(fabrice) → review+
Attachment #8666134 - Flags: review?(jonas) → review+
Comment on attachment 8665781 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_uriloader_changes_2.patch

Thanks.
Flags: needinfo?(honzab.moz)
Attachment #8665781 - Flags: review+
Honza, unfortunately bugzilla does not allow one to use more than one needinfo flag within a bug, hence you missed reviewing  bug_1199295_loadingprincipal_prefetch_test_updates.patch (see comment 39). Can you please have a look and then we have that landed in no time :-)
Flags: needinfo?(honzab.moz)
Comment on attachment 8665783 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_test_updates.patch

Review of attachment 8665783 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/ajax/offline/test_updateCheck.html
@@ +34,4 @@
>  function manifestCached()
>  {
>    // Run first check for an update
> +  updateService.checkForUpdate(manifestURI, systemPrincipal, 0, false, {

you may need to use the window's principal here.  we do check the HTTP cache and this way you may tell the cache/appcache use a different jar (means that something may not be found when it should be).  this will work now, but may break later.

anyway, appcache is deprecated, so if this works, leave it...
Attachment #8665783 - Flags: review+
Flags: needinfo?(honzab.moz)
Just qfolded the two patches. Carrying over r+ from mayhemer.
Attachment #8664617 - Attachment is obsolete: true
Attachment #8665781 - Attachment is obsolete: true
Attachment #8669007 - Flags: review+
You need to log in before you can comment on or make changes to this bug.