resource:// URIs leak information (Tor 8725)

ASSIGNED
Assigned to

Status

()

Core
Security
P1
normal
ASSIGNED
4 years ago
3 hours 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:m1])

MozReview Requests

()

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

Attachments

(4 attachments, 4 obsolete attachments)

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

Comment 9

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

10 months ago
Whiteboard: [tor]

Comment 11

10 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

10 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

10 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

10 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

10 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

9 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

9 months ago

Updated

9 months ago
Assignee: huseby → evilpies

Updated

9 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

9 months ago
Attachment #8783214 - Attachment is obsolete: true
Blocks: 903959

Updated

9 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

4 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

18 days ago
Priority: P2 → P1
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][fp:m1]

Updated

18 days ago
Assignee: nobody → cfu
(Assignee)

Updated

16 days 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

8 days ago
Attachment #8786306 - Attachment is obsolete: true
(Assignee)

Updated

8 days ago
Attachment #8783218 - Attachment is obsolete: true
(Assignee)

Updated

8 days ago
Attachment #8866700 - Flags: review?(wmccloskey)
(Assignee)

Updated

8 days ago
Attachment #8868827 - Flags: review?(bobbyholley)
Attachment #8866701 - Flags: review?(wmccloskey)
(Assignee)

Comment 42

8 days 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

2 days 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

2 days 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 day 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

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

6 hours ago
mozreview-review
Comment on attachment 8866700 [details]
Bug 863246 - Deny resource:// access to web content

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

6 hours 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

3 hours 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)
You need to log in before you can comment on or make changes to this bug.