nsHostObjectProtocolHandler shouldn't be URI_IS_LOCAL_RESOURCE if it includes remote streams

RESOLVED FIXED in Firefox -esr52

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
2 years ago
24 days ago

People

(Reporter: dveditz, Assigned: kmckinley)

Tracking

(Blocks: 1 bug, {sec-moderate})

unspecified
mozilla55
sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox52 wontfix, firefox-esr5254+ fixed, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main54-][adv-esr52.2-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Jonas: you originally added this code, is it still an appropriate flag for what it's being used for?
Flags: needinfo?(jonas)
(Reporter)

Comment 2

2 years ago
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).
(Reporter)

Updated

2 years ago
Keywords: sec-moderate
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?
Flags: needinfo?(jonas)
(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.
Component: DOM → DOM: Security
Sure, putting it in the backlog for now.
Whiteboard: [domsecurity-backlog]
Henry will help to investigate this bug.
Assignee: nobody → hchang
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
(Reporter)

Comment 9

11 months ago
Henry: what is the status here? This is listed as "domsecurity-active" but there hasn't been much activity.
Flags: needinfo?(hchang)
Priority: -- → P2

Comment 10

11 months ago
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 :(
Flags: needinfo?(hchang)

Comment 11

11 months ago
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

11 months ago
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
Blocks: 1328695

Comment 13

11 months ago
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
Flags: needinfo?(tanvi)
Redirecting needinfo to Kate to take a look at this.
Flags: needinfo?(tanvi) → needinfo?(kmckinley)
(Assignee)

Comment 15

10 months ago
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.
Flags: needinfo?(kmckinley)

Comment 16

10 months ago
(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
Flags: needinfo?(amarchesini)

Updated

10 months ago
Depends on: 1330928
> 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.
Flags: needinfo?(amarchesini) → needinfo?(dveditz)
(Assignee)

Updated

9 months ago
Assignee: hchang → kmckinley
(Assignee)

Comment 18

9 months ago
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.
(Assignee)

Comment 19

8 months ago
Created attachment 8849775 [details] [diff] [review]
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
Attachment #8849775 - Flags: review?(dveditz)
Attachment #8849775 - Flags: review?(amarchesini)
(Reporter)

Comment 20

8 months ago
(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?
Flags: needinfo?(dveditz)
(Reporter)

Comment 21

8 months ago
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().
(Reporter)

Comment 22

8 months ago
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.
Flags: needinfo?(kmckinley)
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().
Attachment #8849775 - Flags: review?(amarchesini) → review-
(Reporter)

Comment 24

8 months ago
memory blobs are kind of equivalent to data: urls, which are also URI_IS_LOCAL_RESOURCE so that should be fine.
(Reporter)

Updated

8 months ago
Attachment #8849775 - Flags: review?(dveditz) → review-
(Assignee)

Comment 25

8 months ago
Created attachment 8852163 [details] [diff] [review]
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
Attachment #8849775 - Attachment is obsolete: true
Flags: needinfo?(kmckinley)
Attachment #8852163 - Flags: review?(dveditz)
Attachment #8852163 - Flags: review?(amarchesini)
(Reporter)

Comment 26

8 months ago
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.
Attachment #8852163 - Flags: review?(dveditz) → feedback+
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 '* *'.
Attachment #8852163 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 28

8 months ago
Created attachment 8852661 [details] [diff] [review]
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
Attachment #8852163 - Attachment is obsolete: true
Attachment #8852661 - Flags: review?(amarchesini)
Attachment #8852661 - Flags: feedback?(dveditz)
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
Attachment #8852661 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 30

8 months ago
Created attachment 8853576 [details] [diff] [review]
remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries

Carrying over r+ (only change is commit message & rebase).
Attachment #8852661 - Attachment is obsolete: true
Attachment #8852661 - Flags: feedback?(dveditz)
Attachment #8853576 - Flags: review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/56ef6e6757d4
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → wontfix
status-firefox-esr52: --- → affected
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56ef6e6757d4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
Flags: needinfo?(kmckinley)
(Assignee)

Comment 34

8 months ago
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
Flags: needinfo?(kmckinley)
Attachment #8853576 - Flags: approval-mozilla-aurora?
status-firefox53: affected → wontfix
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+.
Attachment #8853576 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 36

8 months ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f68178e63a35
status-firefox54: affected → fixed
(Reporter)

Updated

8 months ago
Group: dom-core-security → core-security-release
Let's keep this on the radar for possible ESR 52.2 inclusion next cycle too.
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox-esr52: --- → ?
(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.
tracking-firefox54: ? → +
tracking-firefox55: ? → +
tracking-firefox-esr52: ? → 53+
tracking-firefox-esr52: 53+ → 54+
Comment on attachment 8853576 [details] [diff] [review]
remove URI_IS_LOCAL_RESOURCE except for blobs and font table entries

See comment 34.
Attachment #8853576 - Flags: approval-mozilla-esr52?
Christoph, do you think this is bad enough to warrant taking the patch in esr52?
Flags: needinfo?(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.
Flags: needinfo?(ckerschb)

Comment 42

6 months ago
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
Attachment #8853576 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
https://hg.mozilla.org/releases/mozilla-esr52/rev/41138235d4ea
status-firefox-esr52: affected → fixed
Flags: in-testsuite+
Flags: qe-verify-
Whiteboard: [domsecurity-active] → [domsecurity-active][post-critsmash-triage]
Whiteboard: [domsecurity-active][post-critsmash-triage] → [domsecurity-active][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7770
Alias: CVE-2017-7770
Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main54+][adv-esr52.2+] → [domsecurity-active][post-critsmash-triage][adv-main54-][adv-esr52.2-]
(Reporter)

Updated

24 days ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.