776797 - Lock down POfflineCacheUpdate

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Right now we trust the data in the request, but we should only allow updating resources owned by the current origin loaded in the PBrowser.

Comment 1

[Honza Bambas (:mayhemer)]

Can you please be more specific?

Comment 2

[Andrew Overholt [:overholt]]

I forget why we're blocked on input here.  Can anyone enlighten me?

Comment 3

[Dietrich Ayala (:dietrich)]

Needs jduell's analysis to determine if there's work here or not.

Comment 4

[Lucas Adamski [:ladamski]]

Ping?

Comment 5

[Jason Duell]

From talking to Honza, this appears to be just a matter of checking here

  http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#1046

that both the aManifestURI, aDocumentURI are from the origin the app is from (using the app info we can get from the TabParent itself) and that the app has permission to use the offline cache.

Comment 6

[Honza Bambas (:mayhemer)]

Just to confirm and repeat:

- check the documentURI and the manifestURI are the same origin
- check that for domain of the manifestURI we allow offline caching (nsOfflineCacheUpdateService::OfflineAppAllowedForURI(aManifestURI))

This will not break anything, since manifest URI must always be the same origin as the document URI we originally do the permission check for in nsDocShell::LoadURI (or one of its internal methods).

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 8

[Jason Duell]

Comment on attachment 665099 [details] [diff] [review]
v1

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

::: uriloader/prefetch/OfflineCacheUpdateChild.cpp
@@ +54,5 @@
>  // OfflineCacheUpdateChild::nsISupports
>  //-----------------------------------------------------------------------------
>  
>  NS_INTERFACE_MAP_BEGIN(OfflineCacheUpdateChild)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)

I assume this is an unrelated fix?  Why do you need an entry for nsISupports?  (nsHttpChannel, for instance, doesn't have it in its map)

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #8)
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> 
> I assume this is an unrelated fix?  Why do you need an entry for
> nsISupports?  (nsHttpChannel, for instance, doesn't have it in its map)

Thanks for r.  This is needed, otherwise QI asserts here [1] when invoked from some IPC code.  I think there is always a need for nsISupports.

According nsHttpChannel: it inherits it from HttpBaseChannel ;)

[1] http://hg.mozilla.org/mozilla-central/annotate/aacf4867f830/xpcom/glue/nsISupportsImpl.h#l690

Comment 10

[Andrew Overholt [:overholt]]

This should probably never have had the feature keyword.  Sorry for the noise.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 665099 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/42200a47baea

(https://tbpl.mozilla.org/?tree=Try&rev=e98c3a832362)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/42200a47baea