nsHostObjectProtocolHandler flags includes URI_IS_LOCAL_RESOURCE. This made sense when it landed as a local File system, and technically makes sense when it was turned into blob: (although the data stored locally probably had a remote origin, and we don't know if it was transferred securely from the scheme), but now it's used for media streams and rtsp which are _not_ local resources. I'm sure this is turning into wrong security decisions.

Jonas: you originally added this code, is it still an appropriate flag for what it's being used for?

The various specializations override GetScheme, I guess they could just appropriately override GetProtocolFlags. Maybe make the base GetProtocolFlags abstract so they _have_ to (in case we add new ones in the future).

What effect does setting URI_IS_LOCAL_RESOURCE have these days? I.e. what code looks at that flag and changes behavior based on its contents?

(In reply to Jonas Sicking (:sicking) from comment #3) > What effect does setting URI_IS_LOCAL_RESOURCE have these days? I.e. what > code looks at that flag and changes behavior based on its contents? Two examples are: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#504 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#1132 The later we are trying to update in https://bugzilla.mozilla.org/show_bug.cgi?id=1217766, which is what triggered the filing of this bug.

I think we need more than examples, we need a comprehensive search of at least the code in the tree.

Chris, I'm moving this to DOM:Security. Please let me know if you don't think that makes sense. We need to find someone to take a closer look at this bug and it's implications.

Sure, putting it in the backlog for now.

Henry will help to investigate this bug.

Henry: what is the status here? This is listed as "domsecurity-active" but there hasn't been much activity.

The rule which sets a bug to "domsecurity-active" is not clear to me. (maybe ASSIGNED?) To be honest I am just randomly assigned and had no idea after having a quick look at this :(

That being said, I've got some spare cycles lately so it's may be a good time to re-catch this bug.

Let's assume we are going for comment 2: Because of Bug 1279493, nsMediaSourceProtocolHandler and nsMediaStreamProtocolHandler are gone. nsBlobProtocolHandler are handling media source and media stream instead. Originally, the customized scheme "mediasource" [1] and "mediastream" [2] were used to create their specific handlers. However, the special schemes and their handlers have been removed by Bug 1279493. Blob/MediaSource/DOMMediaStream are all handled by nsBlobProtocolHandler. (The object type is used (rather than a special scheme) to inter what to do with AddDataEntry [3] by C++ template.) As a result, we are not able to return different protocol flags in current implementation. [1] https://hg.mozilla.org/mozilla-central/rev/d6e4999d9dd2#l6.66 [2] https://hg.mozilla.org/mozilla-central/rev/d6e4999d9dd2#l6.101 [3] https://hg.mozilla.org/mozilla-central/rev/d6e4999d9dd2#l1.197

According to comment 12, we no longer have handlers for remote streams. MediaSource/MediaStream/Blob are all handled by the same handlers. The previous approach is to create a special scheme (medaisoruce/mediastream) so we can create different protocol handler for them respectively. In other words, we can no nothing in nsHostObjectProtocolHandler::GetProtocolFlags. However, we can tell if a URI is a blob/mediastream/mediasource by looking up gDataTable, which will be used to associate URI and its type (either blob/mediastream/mediasource) [1][2]. Tanvi, since you marked this bug blocking Bug 1328695, do you have any idea what we can do for now? I wonder if [3] can help since it seems the only place where we can tell the object types. [1] https://dxr.mozilla.org/mozilla-central/rev/a2741dd43eeae54f4dd7423bd832a761481c56ce/dom/file/nsHostObjectProtocolHandler.cpp#489 [2] https://dxr.mozilla.org/mozilla-central/rev/a2741dd43eeae54f4dd7423bd832a761481c56ce/dom/file/nsHostObjectProtocolHandler.cpp#106 [3] https://dxr.mozilla.org/mozilla-central/rev/a2741dd43eeae54f4dd7423bd832a761481c56ce/dom/file/nsHostObjectProtocolHandler.cpp#754

Redirecting needinfo to Kate to take a look at this.

I looked at all the sites where URI_IS_LOCAL_RESOURCE is set or used. We currently set the flag for the following protocols: blob moz-fonttable rtsp chrome moz-icon data file moz-extension resource page-icon moz-anno moz-page-thumb android whether to allow a load to proceed nsCSPService nsMixedContentBlocker nsFrameMessageManager nsDataDocumentContentPolicy nsChromeRegistryChrome whether to update the security ui nsSecureBrowserUIImpl whether an image redirect is insecure imgLoader imgRequest whether to prefetch DNS nsContentSink nsHTMLDNSPrefetch The simplest thing might be to just force nsMediaSourceProtocolHandler to implement it's own GetProtocolFlags. Support for rtsp was removed in bug 1295885, so it's probable we could just remove nsMediaSourceProtocolHandler altogether. Remaining is the question of whether data or blob should be tainted if coming from remote or non-secure origins, and the flag set if and only if they are actually local or secure. There may be some overlap with the data URI work here. It is also not clear if the fonts in moz-fonttable could be originally remote, or if favicons should be considered a local resources.

(In reply to Kate McKinley [:kmckinley] from comment #15) > The simplest thing might be to just force nsMediaSourceProtocolHandler to > implement it's own GetProtocolFlags. Support for rtsp was removed in bug > 1295885, so it's probable we could just remove nsMediaSourceProtocolHandler > altogether. Actually nsMediaSourceProtocolHandler is no longer used at all. nsBlobProtocolHandler will be the only one protocol handler for blob/mediasource/mediastream. That's the work done in Bug 1279493. (I am very sure for this because I remove its definition and gecko still can be built.) However, the removal of nsMediaSourceProtocolHandler doesn't means media source will not have a protocol handler. As I investigated in comment 13, when a protocol handler is required for media stream to create an URI, the BlobProtocolHandler will do this job. [1] ni'ing :baku to make sure if I am understanding correctly. Hi :baku, would you mind having a look that if my comment 12 and comment 13 and this comment explains your changes in Bug 1279493 correctly? Thanks :) [1] https://dxr.mozilla.org/mozilla-central/rev/a2741dd43eeae54f4dd7423bd832a761481c56ce/dom/file/nsHostObjectProtocolHandler.cpp#769

> Hi :baku, would you mind having a look that if my comment 12 and comment 13 > and > this comment explains your changes in Bug 1279493 correctly? Thanks :) Yes, you are right. What I suggest is: 1. Let's make nsHostObjectProtocolHandler to implement nsIProtocolHandlerWithDynamicFlags. This interface is used to have dynamic flags when those flags depend on the URLs and not just on the scheme. 2. in nsHostObjectProtocolHandler::GetFlagsForURI() we use IsMediaStreamURI()/IsMediaSourceURI()/IsBlobURI() to pass URI_IS_LOCAL_RESOURCE. 3. nsHostObjectProtocolHandler::GetProtocolFlags() should not pass URI_IS_LOCAL_RESOURCE. dveditz, what do you think about this? Btw, I filed bug 1330928 for removing nsMediaSourceProtocolHandler. Thanks for catching this.

Looks like this breaks a few tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7007a1cedce50cb97abd3df4fa3b65163e62605 For sure it breaks the /secure-contexts/basic-popup-and-iframe-tests.https.html web-platform test. TEST-UNEXPECTED-TIMEOUT | /secure-contexts/basic-popup-and-iframe-tests.https.html | Test Window.isSecureContext in an iframe loading a blob: URI - Test timed out TEST-UNEXPECTED-TIMEOUT | /secure-contexts/basic-popup-and-iframe-tests.https.html | expected OK The plain mochitest js/xpconnect/tests/mochitest/test_sandbox_fetch.html is suspect since it is failing on all platforms. It has a bug, 1243782. Will resolve these two and push again.

Should font table entries be local? My gut feeling is yes since they are table entries, not the fonts themselves. In addition to implementing nsIProtocolHandlerWithDynamicFlags, it looks like it is necessary to implement GetProtocolFlags in nsBlobProtocolHandler, or the web platform tests fail here. Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12d685812288db8bdbe7899605b18d810e520cb2 Prior try run (without checking URI scheme) is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82c5360e3700d6d45d3e3c62c2adb4bee54e1a0d&selectedJob=85420394

(In reply to Andrea Marchesini [:baku] from comment #17) > 2. in nsHostObjectProtocolHandler::GetFlagsForURI() we use > IsMediaStreamURI()/IsMediaSourceURI()/IsBlobURI() to pass > URI_IS_LOCAL_RESOURCE. > dveditz, what do you think about this? Do you mean to pass URI_IS_LOCAL_RESOURCE if IsBlobURI() and do NOT pass it if it's one of the media types? If so that sounds good. If you mean to call all three types "local" can you explain why?

Comment on attachment 8849775 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Review of attachment 8849775 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.cpp @@ +753,5 @@ > +NS_IMETHODIMP > +nsHostObjectProtocolHandler::GetFlagsForURI(nsIURI *aURI, uint32_t *aResult) { > + Unused << nsHostObjectProtocolHandler::GetProtocolFlags(aResult); > + bool isBlob = false; > + nsresult rv = aURI->SchemeIs(BLOBURI_SCHEME, &isBlob); Why are you checking the scheme instead of calling IsBlobURI() as suggested in comment 17? The media stream and source types will also have a blob: scheme, but will return false for IsBlobURI().

I may be misunderstanding how media stream and source fit in, but I think my question above is an r- if I'm understanding it correctly.

Comment on attachment 8849775 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Review of attachment 8849775 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.cpp @@ +745,5 @@ > NS_IMETHODIMP > nsHostObjectProtocolHandler::GetProtocolFlags(uint32_t *result) > { > *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_SUBSUMERS | > + URI_NON_PERSISTABLE; I agree with Daniel. This patch could be simpler if we just check the result of IsBlobURI(). Plus, do we want to return URI_IS_LOCAL_RESOURCE also for memory blobs? We could maybe retrieve the BlobImpl out of the URI, and check if IsFile().

memory blobs are kind of equivalent to data: urls, which are also URI_IS_LOCAL_RESOURCE so that should be fine.

I had two versions of this patch, and this is the version I meant to post. This version only adds an override for GetProtocolFlags for nsBloblProtocolHandler, and uses IsBlobURI() for nsHostObjectProtocolHandler. try is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ec19efba4e54116d0ea044a82f10e7112450a4f

Comment on attachment 8852163 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Review of attachment 8852163 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.cpp @@ +758,5 @@ > + bool isBlob = false; > + nsresult rv = aURI->SchemeIs(BLOBURI_SCHEME, &isBlob); > + if (IsFontTableURI(aURI) || > + (NS_SUCCEEDED(rv) && isBlob)) { > + */ We don't want to check in commented-out code, do we? If it's important then it needs some comments explaining what it's doing here. @@ +760,5 @@ > + if (IsFontTableURI(aURI) || > + (NS_SUCCEEDED(rv) && isBlob)) { > + */ > + if (IsFontTableURI(aURI) || IsBlobURI(aURI)) { > + *aResult |= URI_IS_LOCAL_RESOURCE; This part looks right.

Comment on attachment 8852163 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Review of attachment 8852163 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.cpp @@ +751,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +nsHostObjectProtocolHandler::GetFlagsForURI(nsIURI *aURI, uint32_t *aResult) { { in a new line @@ +758,5 @@ > + bool isBlob = false; > + nsresult rv = aURI->SchemeIs(BLOBURI_SCHEME, &isBlob); > + if (IsFontTableURI(aURI) || > + (NS_SUCCEEDED(rv) && isBlob)) { > + */ +1. Remove this comment. ::: dom/file/nsHostObjectProtocolHandler.h @@ +41,5 @@ > // nsIProtocolHandler methods, except for GetScheme which is only defined > // in subclasses. > NS_IMETHOD GetDefaultPort(int32_t *aDefaultPort) override; > NS_IMETHOD GetProtocolFlags(uint32_t *aProtocolFlags) override; > + NS_IMETHOD GetFlagsForURI(nsIURI *aURI, uint32_t *aResult) override; this is not part of nsIProtocolHandler. Can you make a separate comment block like this: // nsIProtocolHandlerWithDynamicFlags methods NS_IMETHOD GetFlagsForURI(nsIURI *aURI, uint32_t *aResult) override; @@ +103,5 @@ > class nsFontTableProtocolHandler : public nsHostObjectProtocolHandler > { > public: > + NS_IMETHOD GetScheme(nsACString &result) override; > + NS_IMETHOD NewURI(const nsACString & aSpec, const char * aOriginCharset, nsIURI *aBaseURI, nsIURI * *_retval) override; 80 chars max. indent please. Can you please also move '*' close to 'aOriginCharset'. and remove the space between '* *'.

Small changes: 1) cleanup from reviews 2) add test to check that blobs and fonttable entries are local, media streams are not 3) added GetProtocolFlags to nsFontTableProtocolHandler so they properly report local

Comment on attachment 8852661 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Review of attachment 8852661 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.h @@ +106,5 @@ > { > public: > + NS_IMETHOD GetProtocolFlags(uint32_t *aProtocolFlags) override; > + NS_IMETHOD GetScheme(nsACString &result) override; > + NS_IMETHOD NewURI(const nsACString & aSpec, no space after & ::: dom/file/tests/test_bloburi.js @@ +24,5 @@ > + for (let i = 0; i < uris.length; i++) { > + let uri = ios.newURI(uris[i].uri); > + let handler = ios.getProtocolHandler(uri.scheme).QueryInterface(Ci.nsIProtocolHandler); > + let flags = handler.protocolFlags; > + extra spaces

Carrying over r+ (only change is commit message & rebase).

Not sure this is worth a backport to Beta53 at this point since we go to RC next week, but maybe at least an Aurora approval request for now?

Comment on attachment 8853576 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Approval Request Comment [Feature/Bug causing the regression]: Media streams were merged into nsHostObjectProtocolHandler [User impact if declined]: Some rstp media streams may be inadvertently treated as local resources. [Is this code covered by automated tests?]: test included, functionality already covered by multiple automated tests, mochitest and web platform. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: N/A [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: No [Why is the change risky/not risky?]: Blobs and font table entries are still treated as local resources. This change prevents media streams (rstp) from being treated as local resources. [String changes made/needed]: None

Comment on attachment 8853576 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries Fix a security issue and include test. Aurora54+.

Let's keep this on the radar for possible ESR 52.2 inclusion next cycle too.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #37) > Let's keep this on the radar for possible ESR 52.2 inclusion next cycle too. Tracking for all branches based on Comment 37.

Comment on attachment 8853576 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries See comment 34.

Christoph, do you think this is bad enough to warrant taking the patch in esr52?

That's debatable: The bug was filed over a year ago and we just recently fixed it :-( On the other hand the patch is mostly testing code, the actual fix is only a few lines of code. Given that, I would rather lean towards taking a fix for a sec-moderate bug into consideration for ESR52.

Comment on attachment 8853576 [details] [diff] [review] remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries 54 has this fix, usually we try to keep mainline and corresponding ESR version in sync w.r.t security fixes. Let's uplift this to ESR52