resource:// URIs leak information (Tor 8725)

NEW
Unassigned

Status

()

Core
Security
P2
normal
4 years ago
a month ago

People

(Reporter: Tom Adams, Unassigned)

Tracking

(Blocks: 3 bugs)

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor][fingerprinting])

Attachments

(3 attachments, 2 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

11 months ago
Attachment #739040 - Attachment mime type: text/plain → text/html

Comment 2

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

Comment 7

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

10 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

9 months ago
Whiteboard: [tor]
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

9 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

9 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

9 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

9 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

8 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

8 months ago

Updated

8 months ago
Assignee: huseby → evilpies

Updated

8 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

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

Updated

8 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

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