1237868 - nsHostObjectProtocolHandler shouldn't be URI_IS_LOCAL_RESOURCE if it includes remote streams

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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

Comment 2

[Daniel Veditz [:dveditz]]

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).

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 4

[Tanvi Vyas[:tanvi]]

(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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 6

[Tanvi Vyas[:tanvi]]

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.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Sure, putting it in the backlog for now.

Comment 8

[Ethan Tseng [:ethan]]

Henry will help to investigate this bug.

Comment 9

[Daniel Veditz [:dveditz]]

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

Comment 10

[Henry Chang [:hchang]]

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 :(

Comment 11

[Henry Chang [:hchang]]

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

Comment 12

[Henry Chang [:hchang]]

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

Comment 13

[Henry Chang [:hchang]]

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

Comment 14

[Tanvi Vyas[:tanvi]]

Redirecting needinfo to Kate to take a look at this.

Comment 15

[Kate McKinley [:kmckinley, :Kate]]

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.

Comment 16

[Henry Chang [:hchang]]

(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

Comment 17

[Andrea Marchesini [:baku]]

> 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.

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

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.

Comment 19

[Kate McKinley [:kmckinley, :Kate]]

Attachment: remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries

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

Comment 20

[Daniel Veditz [:dveditz]]

(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 21

[Daniel Veditz [:dveditz]]

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().

Comment 22

[Daniel Veditz [:dveditz]]

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 23

[Andrea Marchesini [:baku]]

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().

Comment 24

[Daniel Veditz [:dveditz]]

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

Comment 25

[Kate McKinley [:kmckinley, :Kate]]

Attachment: remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries

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 26

[Daniel Veditz [:dveditz]]

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 27

[Andrea Marchesini [:baku]]

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 '* *'.

Comment 28

[Kate McKinley [:kmckinley, :Kate]]

Attachment: remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries

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 29

[Andrea Marchesini [:baku]]

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

Comment 30

[Kate McKinley [:kmckinley, :Kate]]

Attachment: remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ef6e6757d4

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56ef6e6757d4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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 34

[Kate McKinley [:kmckinley, :Kate]]

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 35

[Gerry Chang [:gchang]]

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+.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f68178e63a35

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Marcia Knous [:marcia]]

(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 39

[Ryan VanderMeulen [:RyanVM]]

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

See comment 34.

Comment 40

[Julien Cristau [:jcristau]]

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

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

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 42

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/41138235d4ea