resource:// URIs leak information (Tor 8725)

ASSIGNED
Assigned to

Status

()

Core
Security
P1
normal
ASSIGNED
4 years ago
5 days ago

People

(Reporter: Tom Adams, Assigned: cfu)

Tracking

(Blocks: 3 bugs)

20 Branch
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor][fingerprinting][fp:m2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 7 obsolete attachments)

3.38 KB, text/html
Details
59 bytes, text/x-review-board-request
billm
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
billm
: review+
Details | Review
(Reporter)

Description

4 years ago
Created attachment 739040 [details]
A copy of the page I linked to, in case of link rot

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130329030352

Steps to reproduce:

Visit this URI: http://marcorondini.eu/research/resource_uri/


Actual results:

Information from resource:// is leaked, including the fact that I'm using Firefox, the platform, the browser's language (not just the content of the Accept-Language header), and whether or not I was using Tor Browser Bundle.


Expected results:

No information in resource:// should be leaked.
(Reporter)

Updated

4 years ago
Component: Untriaged → Security

Comment 1

4 years ago
I verified this under Windows using the Tor Browser Bundle (reports version Firefox ESR 17.0.4, not sure if this is accurate or not).

However the exploit failed in Aurora (Firefox 22.0a2 2013-4-12) where the javascript was unable to access resource URIs (although I could from the address bar).

This leads me to believe that at least in the 22 branch the security issue has been plugged.

Updated

a year ago
Attachment #739040 - Attachment mime type: text/plain → text/html

Comment 2

a year ago
I saw there is an add-on against that:
https://addons.mozilla.org/fr/firefox/addon/no-resource-uri-leak/

Is it still a real issue in FF46?
Component: Security → Networking
Product: Firefox → Core
rehosted attachment at
https://www.ducksong.com/misc/b2.html
I really have no idea if this is a problem to be fixed, and if so its unlikely the networking layer would do the enforcement (we need to be able to access resource uris in some contexts for sure).. christoph maybe?
Component: Networking → Security
Flags: needinfo?(ckerschb)
Wow, this POC has been around for quite some time. Anyway, I suppose this is something worth fixing. Dave is our TOR specialist, what do you think? Check out the link in Comment 3.
Flags: needinfo?(ckerschb) → needinfo?(huseby)
I suppose Pat is right and this is a dup of Bug 903959.
This problem is also shown on https://www.browserleaks.com/firefox
This bugs was mentioned on http://www.ghacks.net/2016/06/12/firefox-resource-leak/ and someone created an add-on to disable these loads until we fix it: https://addons.mozilla.org/fr/firefox/addon/no-resource-uri-leak/
This is very interesting.  I brought it up with Arthur Edelstein and we're investigating.  I'm taking this bug to investigate and fix if needed.
Assignee: nobody → huseby
Flags: needinfo?(huseby)
For reference, here is the Tor Browser ticket: https://trac.torproject.org/projects/tor/ticket/8725

Updated

11 months ago
Whiteboard: [tor]

Comment 11

11 months ago
the related tor browser ticket: https://trac.torproject.org/projects/tor/ticket/8725
Created attachment 8776322 [details] [diff] [review]
Proof of concept patch

This patch explicitly disallows all resource hosts defined by Firefox itself. We should definitely add telemetry to see how often resouce URIs are actually loaded in content.

Instead of blacklisting certain resource hosts we obviously need something different. Maybe a new flag for chrome manifests where most substitutions are defined and adding a flag to SetSubstitution if it is save to load?
It might be better to do this in nsScriptSecurityManager::CheckLoadURIFlags instead, which already has similar code for chrome://.

Comment 14

11 months ago
Please look at this:
Isolated content environment proposal
https://discourse.mozilla-community.org/t/proposal-isolated-content-environment/9866?u=desktopd

Any ideas?

Comment 15

11 months ago
I want this problem fixed so FF gives less opportunities for fingerprinting but keep in mind that you could harm Webmail encryption via mailvelope if you do. https://github.com/mailvelope/mailvelope/issues/406

Updated

11 months ago
Priority: -- → P3
Comment on attachment 8776322 [details] [diff] [review]
Proof of concept patch

Hey Dave. This is not a real patch yet. I am not sure yet how to decide which hosts to block. We probably need some kind of flag in the ComponentsRegistry. I also don't quite understand how the empty host protocols work, is it just everything in the Firefox directory?

We might actually decide to just block everything that is defined by Firefox and have a new version of SetSubstitution that allows to specify content accessibility. (So that add-ons using the old version of SetSubstitution would still be exposed, but they can opt-into not exposing)
Attachment #8776322 - Flags: feedback?(huseby)

Comment 17

11 months ago
Be carefull with the audio&videoplayer: TorBrowser caused https://trac.torproject.org/projects/tor/ticket/19837 when trying to fix it.
Good catch, seems like we really need to implement AllowContentToAccess, I am going to look into extending the code in nsScriptSecurityManager::CheckLoadURIFlags to work with resource URIs.
Created attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

These flags allows us to whitelist a specific host substitution in the manifest. It is also used to keep old "SetSubstitution" function working, so that for example extensions that use it to provide web-accessible resources don't break.
Attachment #8783214 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8776322 - Attachment is obsolete: true
Attachment #8776322 - Flags: feedback?(huseby)
Created attachment 8783218 [details] [diff] [review]
Alias gre-res to /gre/res and make it contentaccessible

It seems like mostly everything that should be web-accessible is already in /gre/res/, but I have no idea if that is actually true. So we alias resource://gre-res to resource://gre/res and make it contentaccessible.
Attachment #8783218 - Flags: feedback?(mh+mozilla)
Comment on attachment 8783218 [details] [diff] [review]
Alias gre-res to /gre/res and make it contentaccessible

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

I'm not sure why you're seeking my feedback here. If it's on the build system side, then it looks fine. If it's for part creating resource://gre-res and making it contentaccessible, I'm not the right person to ask whether it's the right solution to this bug.
Attachment #8783218 - Flags: feedback?(mh+mozilla)
Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

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

I'm not peer of this code.
Attachment #8783214 - Flags: review?(mh+mozilla)
Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

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

Mhm I must have misread some of the blame. Maybe bz can take a look, seems like you reviewed some of the AllowContentToAccess stuff we have for chrome://.
Attachment #8783214 - Flags: feedback?(bzbarsky)
Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

The caps bits look fine.  The necko bits, I have no idea how that stuff works nowadays...
Attachment #8783214 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

Not that easy to find out who can review this code. I hope you can have a look :)
Attachment #8783214 - Flags: review?(wmccloskey)
Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

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

I'm going to r- this for now because of the add-on issues we talked about. It looks good aside from that though, although I haven't looked to thoroughly.

::: caps/nsScriptSecurityManager.cpp
@@ +948,5 @@
> +
> +              bool accessAllowed = false;
> +              rph->AllowContentToAccess(aTargetBaseURI, &accessAllowed);
> +              if (accessAllowed) {
> +                  return NS_OK;

Indentation is off here.

@@ +958,5 @@
> +              if (reg) {
> +                  bool accessAllowed = false;
> +                  reg->AllowContentToAccess(aTargetBaseURI, &accessAllowed);
> +                  if (accessAllowed) {
> +                      return NS_OK;

Here too.

::: chrome/nsChromeRegistryChrome.cpp
@@ +980,5 @@
>      return;
>    }
>  
> +  uint32_t substitutionFlags = 0;
> +  if (flags & nsChromeRegistry::CONTENT_ACCESSIBLE)

Should have braces here.

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ +338,5 @@
>  
> +  if (SubstitutionEntry* entry = mSubstitutions.Get(root)) {
> +    // How does this work?
> +    *result = entry->baseURI.get();
> +    NS_ADDREF(*result);

It's cleaner to assign to a local nsCOMPtr and then .forget that into result.
Attachment #8783214 - Flags: review?(wmccloskey) → review-
Summary: resource:// URIs leak information → resource:// URIs leak information (Tor 8725)

Updated

10 months ago

Updated

10 months ago
Assignee: huseby → evilpies

Updated

10 months ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Created attachment 8786306 [details] [diff] [review]
v2 - Introduce a contentaccessible flag for resource substitutions

This patch makes extension resource contentaccessible by default, but they can explicitly opt-out with contentaccessible=no.

I still haven't gotten a green try run, seems like there are still more resource to track down and whitelist.
Attachment #8786306 - Flags: review?(wmccloskey)

Updated

10 months ago
Attachment #8783214 - Attachment is obsolete: true

Updated

10 months ago
Blocks: 903959

Updated

10 months ago
Blocks: 1260929
Comment on attachment 8786306 [details] [diff] [review]
v2 - Introduce a contentaccessible flag for resource substitutions

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

This patch really needs some tests! It looks pretty good though. However, I want to look at it again before r+ing.

I didn't look at the caps part. You'll need review from bz or bholley on that.

::: chrome/nsChromeRegistryChrome.cpp
@@ +982,5 @@
>  
> +  uint32_t substitutionFlags = 0;
> +  if (flags & nsChromeRegistry::CONTENT_ACCESSIBLE) {
> +    substitutionFlags |= nsIResProtocolHandler::ALLOW_CONTENT_ACCESS;
> +  } else {

This might be cleaner as:

if (flags & CONTENT_ACCESSIBLE) {
  |= ALLOW_...;
} else if (flags & CONTENT_INACCESSIBLE) {
  // Don't set any flags.
} else {
  // Default for extensions is to allow content access. Default for browser code is not to allow it.
  if (type == ...) {
    |= ALLOW...;
  }
}

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ +149,4 @@
>      aBaseURI->GetSpec(mapping.resolvedURI.spec);
>      aBaseURI->GetOriginCharset(mapping.resolvedURI.charset);
>    }
> +  mapping.flags = flags;

I think you need code to use the extra flags here:
http://searchfox.org/mozilla-central/rev/c31ad35f39c6187b2e121aa6d3a39b7f67397010/chrome/nsChromeRegistryContent.cpp#121

Maybe other places too.

@@ +330,4 @@
>    return NS_OK;
>  }
>  
> +

Extra newline.

@@ +336,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(result);
>  
> +  if (SubstitutionEntry* entry = mSubstitutions.Get(root)) {
> +    // How does this work?

What is this comment for?

::: netwerk/protocol/res/SubstitutingProtocolHandler.h
@@ +76,4 @@
>    nsIIOService* IOService() { return mIOService; }
>  
>  private:
> +  struct SubstitutionEntry : public PLDHashEntryHdr

I don't think you need the inheritance here.

@@ +89,3 @@
>    nsCString mScheme;
>    Maybe<uint32_t> mFlags;
> +  nsClassHashtable<nsCStringHashKey, SubstitutionEntry> mSubstitutions;

You might be able to use nsDataHashtable here to avoid allocating SubstitutionEntrys in the heap.

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl
@@ +12,4 @@
>  [scriptable, uuid(154c64fd-a69e-4105-89f8-bd7dfe621372)]
>  interface nsISubstitutingProtocolHandler : nsIProtocolHandler
>  {
> +  const short ALLOW_CONTENT_ACCESS = 1;

Please add a comment describing what this means. Also, can we put this flag on nsIResProtocolHandler.idl? It doesn't apply to ExtensionProtocolHandler (and is confusing there).

@@ +25,4 @@
>     */
>    void setSubstitution(in ACString root, in nsIURI baseURI);
>  
> +  void setSubstitutionWithFlags(in ACString root, in nsIURI baseURI, in uint32_t flags);

Need a comment here too.

::: xpcom/components/ManifestParser.cpp
@@ +122,4 @@
>      nullptr, &nsChromeRegistry::ManifestOverride, nullptr
>    },
>    {
> +    "resource",         2, false, true, true, true, true,

Please fix the comment on the contentflags field of ManifestDirective.
Attachment #8786306 - Flags: review?(wmccloskey)
Sorry for holding this up, I will try to get back to this soon, probably this week.

Updated

5 months ago
See Also: → bug 779197
Whiteboard: [tor] → [tor][fingerprinting]
Blocks: 1329996
Priority: P3 → P2
If anyone wants to pick this up feel free, I am currently working on some other things.
Sorry, I won't be working on this in the near future.
Assignee: evilpies → nobody
Status: ASSIGNED → NEW

Updated

2 months ago
Priority: P2 → P1
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][fp:m1]

Updated

2 months ago
Assignee: nobody → cfu
(Assignee)

Updated

2 months ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8786306 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8783218 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8866700 - Flags: review?(wmccloskey)
(Assignee)

Updated

a month ago
Attachment #8868827 - Flags: review?(bobbyholley)
Attachment #8866701 - Flags: review?(wmccloskey)
(Assignee)

Comment 42

a month ago
(In reply to Bill McCloskey (:billm) from comment #28)
Hi Bill,

I took this bug and made a new patch, please take a look again.
This patch is based on the one Tom introduced and followed your instruction to modify, except

> ::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl
> @@ +12,4 @@
> >  [scriptable, uuid(154c64fd-a69e-4105-89f8-bd7dfe621372)]
> >  interface nsISubstitutingProtocolHandler : nsIProtocolHandler
> >  {
> > +  const short ALLOW_CONTENT_ACCESS = 1;
> 
> Please add a comment describing what this means. Also, can we put this flag
> on nsIResProtocolHandler.idl? It doesn't apply to ExtensionProtocolHandler
> (and is confusing there).

I think you are right that it should be more reasonable to put the flag ALLOW_CONTENT_ACCESS on nsIResProtocolHandler.
However, I found that the flag is also used by SubstitutingProtocolHandler.
If the flag is put on nsIResProtocolHandler, the dependency will get complex (SubstitutingProtocolHandler will depend on nsIResProtocolHandler, just like parent depends on child).
Therefore, I decided to keep it in nsISubstitutingProtocolHandler and need your comment.

Besides, this patch fails some tests because they try to load resource:// URIs in web pages, and I am not sure if this patch effects other functionalities.
So I use a preference "security.block_resource_uri" to determine whether resource:// URIs should be blocked instead of blocking them by default.

I also added some test cases that resource:// URIs are only blocked with the preference enabled.

For the caps part, I asked bholley to review it.

Thanks
Comment on attachment 8868827 [details]
Bug 863246 - Deny resource:// access to web content

Bouncing to Boris. Sorry for doing so Boris, I'm just incredibly swamped.
Attachment #8868827 - Flags: review?(bobbyholley) → review?(bzbarsky)
Comment on attachment 8868827 [details]
Bug 863246 - Deny resource:// access to web content

https://reviewboard.mozilla.org/r/140412/#review145704

::: commit-message-043ad:1
(Diff revision 1)
> +Bug 863246 - Deny resource:// access to web content

This commit message is wrong.  That's not what the patches are doing.  Please write a reasonable commit message.

::: caps/nsScriptSecurityManager.cpp:80
(Diff revision 1)
>  nsIStringBundle *nsScriptSecurityManager::sStrBundle = nullptr;
>  JSContext       *nsScriptSecurityManager::sContext   = nullptr;
>  bool nsScriptSecurityManager::sStrictFileOriginPolicy = true;
>  
> +// Determines whether resource:// URIs should be blocked
> +static const char * const kPrefBlockResourceUri = "security.block_resource_uri";

Does anything actually set this pref?  If not, why not?

That is, why are we adding a pref here, not changing the default behavior?

::: caps/nsScriptSecurityManager.cpp:883
(Diff revision 1)
>          if (aFlags & nsIScriptSecurityManager::ALLOW_CHROME) {
>  
> -            // For now, don't change behavior for resource:// or moz-icon:// and
> -            // just allow them.
> -            if (!targetScheme.EqualsLiteral("chrome")) {
> +            // For now, don't change behavior for moz-icon:// and just allow it.
> +            if (!targetScheme.EqualsLiteral("chrome") &&
> +                    // resource:// is simply allowed if the preference kPrefBlockResourceUri is false
> +                    (!Preferences::GetBool(kPrefBlockResourceUri) || !targetScheme.EqualsLiteral("resource"))) {

Why is there an uncached pref lookup on a hot path here?

Please use a bool var cache in the usual way, assuming we need the pref at all.  Is the pref just to make this easy to back out?

::: caps/nsScriptSecurityManager.cpp:908
(Diff revision 1)
>  
> +            if (targetScheme.EqualsLiteral("resource")) {
> +                nsCOMPtr<nsIProtocolHandler> ph;
> +                rv = sIOService->GetProtocolHandler("resource", getter_AddRefs(ph));
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);

What guarantees that this does not return null?  We should probably block the load if null is returned.
Attachment #8868827 - Flags: review?(bzbarsky) → review-
(In reply to Chung-Sheng Fu [:cfu] from comment #42)

Hi Chung-Sheng -- thank you for working on this ticket. Disabling resource:// (and chrome://) URIs completely causes breakage of a number of browser features, including video controls, image display, text-box resizing, and directory listing.

So in Tor Browser, my colleague wrote some stopgap JavaScript extension code that blocks resource://, chrome:// and about: URIs from being loaded by content, except that some specific resource: and chrome: URIs are whitelisted, so that they can be loaded and no browser features are broken.

You can see the whitelisted addresses here:
https://gitweb.torproject.org/torbutton.git/tree/src/components/content-policy.js#n33

Could this whitelist (and any additional URIs need to past unit tests) be included in your C++ patch? I think that would eliminate the need for the "security.block_resource_uri" preference altogether.
(Assignee)

Comment 46

a month ago
Hi Arthur,

Thank you very much for your comment.

Hi Boris,

Does the whitelist approach in Tor Browser seem reasonable to you?
If we are going to implement the whitelist, should the accessibility flag of substitution mapping be kept?
Perhaps it is better to keep the flag and related functions because maybe it can be used to temporarily allow URIs in unit tests in order not to fail the tests without adding them to the whitelist.
Besides, should chrome:// URIs be block as well like Tor Browser does?

Thanks!
Flags: needinfo?(bzbarsky)

Comment 47

a month ago
(In reply to Chung-Sheng Fu [:cfu] from comment #46)
> Does the whitelist approach in Tor Browser seem reasonable to you?

I don't think the whitelist approach should get uplifted. It is a nasty workaround which only works for us because we don't need to take care of extensions getting installed (apart from the ones we ship). In fact, later on we had to add a way to disable that whitelist approach for users with additional extensions installed (https://trac.torproject.org/projects/tor/ticket/21396). I have not checked whether resource:// URIs are affected by that but chances are high that they are. But even if not, then there is the issue of keeping the whitelist up-to-date.

Besides that, yes, we'd need a fix for chrome:// URIs as well but, again, without whitelisting particular chrome URLs.
> Does the whitelist approach in Tor Browser seem reasonable to you?

Um... I assumed that it was already implemented in the patches, via the contentaccessible thing.

I would much prefer we handle this via chrome registration instead of a hardcoded whitelist, obviously.

If we have a problem wherein things that should and shouldn't be exposed are in the same package, the right solution, as for chrome://, is to split them up into separate packages.

> Besides that, yes, we'd need a fix for chrome:// URIs as well

What's the problem with chrome:// URIs?  They are already not exposed unless you explicitly opt in to exposing them in your manifest.
Flags: needinfo?(bzbarsky)

Comment 49

a month ago
(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #48)
> > Besides that, yes, we'd need a fix for chrome:// URIs as well
> 
> What's the problem with chrome:// URIs?  They are already not exposed unless
> you explicitly opt in to exposing them in your manifest.

It seemed to me there is a bit uncertainty involved in how to get the patch for this bug done. I just wanted to highlight that there is no real difference between resource:// and chrome:// URIs regarding fingerprinting issues. Thus, however this bug gets fixed chrome:// URIs should be kept in mind, so that chrome:// URIs have/get a similar treatment.
Right, I think we should fix this the same way as chrome:// URIs: don't expose by default, allow opt-in to exposure via the chrome manifest.

Whether that's viable for backporting is a separate issue.  For chrome:// URIs when we first introduced the contentaccessible flag we defaulted to "yes" for a while until we flipped the default, to give extensions time to update....
I think Boris and Georg are right. A `contentaccessible` flag approach is probably much better than the whitelist approach I was suggesting in comment 45.
(Assignee)

Comment 52

a month ago
(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #50)
Hi Arthur and Georg,

Thank you very much for your comments.

Hi Boris,

> Right, I think we should fix this the same way as chrome:// URIs: don't
> expose by default, allow opt-in to exposure via the chrome manifest.

Yes, this patch exactly does what you mean, and I totally agree with you that the limitation of loading resource:// URIs should be default behavior.
However, this patch fails some unit tests and I am not sure if it also breaks any living functionalities.
That's why I added a preference to determine whether the check of the contentaccessible flag should be performed when content is trying to load resource:// URIs.
Therefore, I would like to ask if I can put the patch (without the preference) on nightly builds and let users test if there is any breakage of functionalities?
Flags: needinfo?(bzbarsky)
This isn't a very good idea. Please first start looking into fixing the failures on try, because we already know various resources need to be loadable by content. I think with my patch " Alias gre-res to /gre/res and make it contentaccessible" a lot of these things actually work. Add-ons also need to load resource:// in the content, for example Adblock Plus.
Right, what comment 53 says.  You want to fix the unit test failures by exposing the relevant things as needed, or changing the tests to not depend on them if they really shouldn't be exposed.  You want to talk to Jorge about addon compat fallout, though I see that the patch makes addons opt-in instead of opt-out to the new blocking behavior, so maybe there isn't any.
Flags: needinfo?(bzbarsky) → needinfo?(jorge)
What's the expected impact for add-ons? resource:// URLs were meant to be content-accessible, so changing that assumption could cause a lot of breakage. Like with other add-on compat issues, I'd like to ask if it's necessary to fix before 57, since most legacy add-ons at this point are either migrating to WebExtensions or abandoned.
Flags: needinfo?(jorge) → needinfo?(cfu)

Comment 56

a month ago
mozreview-review
Comment on attachment 8866700 [details]
Bug 863246 - Content can only load resource:// URIs declared content-accessible in manifests

https://reviewboard.mozilla.org/r/138314/#review146700

::: commit-message-d8762:1
(Diff revision 4)
> +Bug 863246 - Deny resource:// access to web content

The commit message should say something about how you're marking resources based on whether they should be content accessible. This patch doesn't actually deny anything.

::: chrome/nsChromeRegistryChrome.cpp:931
(Diff revision 4)
> +  if ((flags & nsChromeRegistry::CONTENT_ACCESSIBLE)
> +      // Allow content access to extension resources by default.
> +      || (!(flags & nsChromeRegistry::CONTENT_INACCESSIBLE)
> +          && ((cx.mType == NS_EXTENSION_LOCATION) || (cx.mType == NS_BOOTSTRAPPED_LOCATION)))) {
> +    substitutionFlags |= nsIResProtocolHandler::ALLOW_CONTENT_ACCESS;
> +  }

This really should be broken into multiple statements, with comments for each case. Something like:

bool contentAccessible = false;
if (cx.mType == NS_EXTENSION_LOCATION || cx.mType == NS_BOOTSTRAPPED_LOCATION) {
  // This is a browser extension. By default, extension resources are
  // content-accessible unless the manifest opts out.
  contentAccessible = !(flags & nsChromeRegistry::CONTENT_INACCESSIBLE);
} else {
  // This manifest is part of Firefox. By default, Firefox resources are
  // not content-accessible unless the manifests opts in.
  contentAccessible = (flags & nsChromeRegistry::CONTENT_ACCESSIBLE);
}

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:136
(Diff revision 4)
>  
>    return NS_OK;
>  }
>  
>  nsresult
> -SubstitutingProtocolHandler::SendSubstitution(const nsACString& aRoot, nsIURI* aBaseURI)
> +SubstitutingProtocolHandler::SendSubstitution(const nsACString& aRoot, nsIURI* aBaseURI, uint32_t flags)

This should be aFlags.

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl:16
(Diff revision 4)
>   */
>  [scriptable, uuid(154c64fd-a69e-4105-89f8-bd7dfe621372)]
>  interface nsISubstitutingProtocolHandler : nsIProtocolHandler
>  {
>    /**
> +   * Content script may access files in this package

This should end with a period.

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl:32
(Diff revision 4)
>     * enforced.
>     */
>    [must_use] void setSubstitution(in ACString root, in nsIURI baseURI);
>  
>    /**
> +   * Same as setSubstitution, but with specific flags

Also should end with a period.
Attachment #8866700 - Flags: review?(wmccloskey) → review+

Comment 57

a month ago
mozreview-review
Comment on attachment 8866701 [details]
Bug 863246 - Add test case

https://reviewboard.mozilla.org/r/138316/#review146708

::: browser/components/resistfingerprinting/test/mochitest/test_uri_resource.html:37
(Diff revision 5)
> +      });
> +      dom = document.createElement("script");
> +      dom.onload = listener.bind(dom, !aPrefVal);
> +      dom.onerror = listener.bind(dom, aPrefVal);
> +      document.head.appendChild(dom);
> +      dom.src = "resource:///defaults/preferences/firefox.js";

This is a nice start, but you really need to test other cases. You should test both the addon case and the non-addon case. And for each of those, you should test resources that don't set contentaccessible as well as those that explicitly set it to both possible values.

Testing add-ons is a pain, but I think we actually need to do it here. Maybe ask rhelmer or Mossop how to do it.
Attachment #8866701 - Flags: review?(wmccloskey) → review-
(Assignee)

Comment 58

a month ago
Hi Tom and Boris,

I did not apply the manifest patch because I am afraid that it still exposes too much.
If the allowance of resource://gre/res seems fine to you, I will apply it.

Hi Jorge,

OK, I will check our milestones about this issue.
Besides, does the migration to WebExtensions imply add-ons will not depend on the use of resource:// URIs?
(I am sorry if this is a silly question but I am really not familiar with that)

Thank you very much!
Flags: needinfo?(cfu)
WebExtensions shouldn't depend on the behavior of resource://, as I understand it. ni Andy for confirmation.
Flags: needinfo?(amckay)

Comment 60

a month ago
An add-on developer writing a webextension will no longer be using resource:// URIs, so they don't need to worry about this change. Since billm is on the thread, I'm pretty sure any underlying WebExtensions issues are covered.

Agree with Jorge, landing in 57 would be nice to reduce breakage.
Flags: needinfo?(amckay)
(Assignee)

Comment 61

28 days ago
We will review the landing schedule.
Thank you very much!
(Assignee)

Comment 62

26 days ago
(In reply to Bill McCloskey (:billm) from comment #57)
> Comment on attachment 8866701 [details]
> Bug 863246 - Add test case
> 
> https://reviewboard.mozilla.org/r/138316/#review146708
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/test_uri_resource.
> html:37
> (Diff revision 5)
> > +      });
> > +      dom = document.createElement("script");
> > +      dom.onload = listener.bind(dom, !aPrefVal);
> > +      dom.onerror = listener.bind(dom, aPrefVal);
> > +      document.head.appendChild(dom);
> > +      dom.src = "resource:///defaults/preferences/firefox.js";
> 
> This is a nice start, but you really need to test other cases. You should
> test both the addon case and the non-addon case. And for each of those, you
> should test resources that don't set contentaccessible as well as those that
> explicitly set it to both possible values.

We decided to follow Jorge and Andy's suggestion and will land the patch in 57.
Should I still test the add-on case?

> Testing add-ons is a pain, but I think we actually need to do it here. Maybe
> ask rhelmer or Mossop how to do it.
Flags: needinfo?(wmccloskey)
> We decided to follow Jorge and Andy's suggestion and will land the patch in 57.
> Should I still test the add-on case?

All right, I guess you don't need an add-on test if you don't plan to land until 57.
Flags: needinfo?(wmccloskey)
Comment hidden (mozreview-request)
(Assignee)

Updated

21 days ago
Attachment #8868827 - Attachment is obsolete: true
(Assignee)

Updated

21 days ago
Attachment #8866701 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Move from milestone 1 to 2 according to comment 60.
Whiteboard: [tor][fingerprinting][fp:m1] → [tor][fingerprinting][fp:m2]
(Assignee)

Comment 67

20 days ago
I am trying to fix failed tests due to this change of policy of loading resource:// URIs by setting substitutions for specific hosts so that content can temporarily load required resources.
Take this test for example http://searchfox.org/mozilla-central/source/caps/tests/mochitest/test_bug292789.html#44
It tries to load resources under resource://gre and I add the following code to make it accessible

    SpecialPowers.Cc["@mozilla.org/network/protocol;1?name=resource"]
        .getService(SpecialPowers.Ci.nsIResProtocolHandler)
        .setSubstitution("gre", SpecialPowers.Services.io.newURI("resource://gre/"));

However, some hosts cannot be substituted according to nsResProtocolHandler::SetSubstitution http://searchfox.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.cpp#98
So, is it possible to remove the host filter in nsResProtocolHandler::SetSubstitution?
Flags: needinfo?(wmccloskey)
(In reply to Chung-Sheng Fu [:cfu] from comment #67)
> Take this test for example
> http://searchfox.org/mozilla-central/source/caps/tests/mochitest/
> test_bug292789.html#44

For this test, I think it would be best to have a preference that allows loads from resource URLs (like the "security.block_resource_uri" pref you added. To be clear though, we want this pref to be enabled by default. You would just turn if off using SpecialPowers.pushPrefEnv around the part of the test where the load should be allowed.
Flags: needinfo?(wmccloskey)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

14 days ago
Attachment #8876624 - Flags: review?(bzbarsky)
Attachment #8876625 - Flags: review?(bzbarsky)
Attachment #8876626 - Flags: review?(wmccloskey)
(Assignee)

Comment 77

14 days ago
There is still a preference named "security.all_resource_uri_content_accessible" now.
By default, it is false and content can only load resource:// URIs declared content-accessible in manifests.
Mochitests can call SpecialPowers.pushPrefEnv to set the preference true and load resource:// URIs they need.

Tom's patch that aliases resource://gre-res to resource://gre/res and sets it content-accessible is also applied.

The unit test for this patch is updated.
First it tries to load an image from resource://gre-res, which is content-accessible because of Tom's aliasing patch.
Then it tries to load an image from resource://gre and should be blocked.
At last, it calls SpecialPowers.pushPrefEnv to set "security.all_resource_uri_content_accessible" true, trying to load image from resource://gre again, and should be allowed this time.

Comment 78

13 days ago
mozreview-review
Comment on attachment 8876626 [details]
Bug 863246 - Add test cases

https://reviewboard.mozilla.org/r/147960/#review153110

::: browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:5
(Diff revision 2)
> +<!DOCTYPE html>
> +<meta charset="utf8">
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<script>
> +function test(aTest, aUri, aContentAccessible, aResolve) {

This function should return a promise.

::: browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:21
(Diff revision 2)
> +  document.body.appendChild(img);
> +}
> +
> +SimpleTest.waitForExplicitFinish();
> +document.addEventListener("DOMContentLoaded", function() {
> +  Promise.resolve(new Promise(function(aResolve) {

This logic is pretty complicated. Instead, please use add_task and async/await. It should look like this:

add_task(async function() {
  await test(...);
  await test(...);
  await SpecialPowers.pushPrefEnv(...);
  await test(...);
});
Attachment #8876626 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 79

12 days ago
(In reply to Bill McCloskey (:billm) from comment #78)
> Comment on attachment 8876626 [details]
> Bug 863246 - Add test cases
> 
> https://reviewboard.mozilla.org/r/147960/#review153110
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:5
> (Diff revision 2)
> > +<!DOCTYPE html>
> > +<meta charset="utf8">
> > +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> > +<script>
> > +function test(aTest, aUri, aContentAccessible, aResolve) {
> 
> This function should return a promise.
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:21
> (Diff revision 2)
> > +  document.body.appendChild(img);
> > +}
> > +
> > +SimpleTest.waitForExplicitFinish();
> > +document.addEventListener("DOMContentLoaded", function() {
> > +  Promise.resolve(new Promise(function(aResolve) {
> 
> This logic is pretty complicated. Instead, please use add_task and
> async/await. It should look like this:
> 
> add_task(async function() {
>   await test(...);
>   await test(...);
>   await SpecialPowers.pushPrefEnv(...);
>   await test(...);
> });

OK I will learn how to use add_task.
Thanks!
Comment on attachment 8876624 [details]
Bug 863246 - Content can only load resource:// URIs declared content-accessible in manifests

https://reviewboard.mozilla.org/r/147956/#review154102

r=me

::: caps/nsScriptSecurityManager.cpp:923
(Diff revision 2)
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                if (!ph) {
> +                    return NS_ERROR_DOM_BAD_URI;
> +                }
> +
> +                nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);

It might be a good idea to cache this, here or on nsContentUtils or something...  Followup is OK for that.
Attachment #8876624 - Flags: review?(bzbarsky) → review+
Comment on attachment 8876625 [details]
Bug 863246 - Alias resource://gre-res to resource://gre/res and make it content-accessible

https://reviewboard.mozilla.org/r/147958/#review154104

r=me
Attachment #8876625 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 82

10 days ago
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #80)
> Comment on attachment 8876624 [details]
> Bug 863246 - Content can only load resource:// URIs declared
> content-accessible in manifests
> 
> https://reviewboard.mozilla.org/r/147956/#review154102
> 
> r=me
> 
> ::: caps/nsScriptSecurityManager.cpp:923
> (Diff revision 2)
> > +                NS_ENSURE_SUCCESS(rv, rv);
> > +                if (!ph) {
> > +                    return NS_ERROR_DOM_BAD_URI;
> > +                }
> > +
> > +                nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);
> 
> It might be a good idea to cache this, here or on nsContentUtils or
> something...  Followup is OK for that.

I will learn how to cache the result of querying interface.
And I am going to fix test failures by setting the pref security.all_resource_uri_content_accessible true.

Thank you very much for your help!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 90

6 days ago
After reviewing several test failures, it seems better to me to have a general solution.
Is it a good idea to add security.all_resource_uri_content_accessible = true to testing/profiles/prefs_general.js?
Flags: needinfo?(bzbarsky)
It seems like it would be too easy to create tests that pass in the test harness but fail in an actual browser, and not because they're explicitly loading res:// bits that are not web-exposed (e.g. a plaintext test that fails because plaintext.css fails to get loaded outside the test harness).
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 92

5 days ago
OK, then I will keep fixing individual failures.
Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 days ago
Attachment #8879416 - Attachment is obsolete: true
(Assignee)

Comment 97

5 days ago
Sorry again.
I found that the "View Source" page requires style sheet from resource://gre-resources.
The folder contains only images and style sheets, and a hiddenWindow.html.
Can I make resource://gre-resources content-accessible in manifest?
Flags: needinfo?(bzbarsky)
I have no idea.  In particular, I don't know whether other things like extensions can place stuff in that folder.

If not, then this folder doesn't leak any information that is not available from the UA string version or object-detection techniques to determine the Firefox version, so probably OK to expose.
Flags: needinfo?(bzbarsky)

Comment 99

5 days ago
Keep in mind though that in https://bugzilla.mozilla.org/show_bug.cgi?id=1333651 the User Agent is spoofed from time to time.
Not sure if this wouldn't break it by revealing the original.
Yes, that's why I said "object-detection techniques".
I mean, the principled and safe thing to do is to add a new expose-gre-resources thing, move stuff that should be exposed there, etc.
You need to log in before you can comment on or make changes to this bug.