Opened 7 years ago

Closed 4 years ago

#8292 closed enhancement (fixed)

Alter behavior of getFirstPartyURI and consumers

Reported by: mikeperry Owned by: mikeperry
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, MikePerry201309
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor: None

Description

Our mozIFirstPartyUtil.getFirstPartyURI should log to the error console and emit a failure code if it either fails to get a URI, or if that URI has a NULL hostname. This will simplify the code that makes use of the API.

If the API call fails for ImageCache consumers, we should load but not cache the image (if possible). If the API call fails for DOM Storage, the DOM Storage calls should probably also fail. For the canvas, we should also fail the calls (See also #7265 for related canvas changes).

Child Tickets

Attachments (1)

8292-patch.txt (27.5 KB) - added by mcs 7 years ago.
revised patch: add missing braces

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by mikeperry

Parent ID: #6564

Unparenting so I can close the parent.

comment:2 Changed 7 years ago by mikeperry

Keywords: tbb-linkability added

Tagging this, because the types of bugs that might result from this additional complexity are linkability bugs. It would be wise to address this if we're also interested in solving those.

comment:3 Changed 7 years ago by mcs

Kathy Brade and I started to work on this. After changing mozIFirstPartyUtil.getFirstPartyURI() to return an error and log to the Error Console when the URI lacks a host, we discovered a couple of problems:

1) The image cache code generates a lot of calls to getFirstPartyURI() that involve chrome: and moz-anno: URIs, none of which have hosts. This results in excessive logging to the Error Console. For example, typing a single "a" in the URL bar causes getFirstPartyURI() to log 13 messages in my browser (due to chrome image load requests and favicon loads caused by browser history access).

2) Some built-in pages use DOM Storage, e.g., about:home. We previously allowed documents whose URIs lacked hosts to use local storage (no isolation). With the change outlined in this bug, that is no longer allowed. That might be OK, except the pages are not coded to handle that situation. E.g., about:home encounters an uncaught exception in its JS code and then fails to initialize its search feature.

Therefore, I think we need to come up with a more nuanced approach. Can we allow trusted pages to use facilities such as DOM Storage and the image cache even though their URIs lack hosts? Of course there would be no isolation for such pages, but that seems OK to me.

comment:4 Changed 7 years ago by mikeperry

Yes, I think it is fine to not log for trusted schemes, though we might want to give them a unique pseudo-host based on their scheme for purposes of cache isolation just in case.

However, beware of data: and javascript: urls. Those should still log.

comment:5 in reply to:  4 Changed 7 years ago by brade

Replying to mikeperry:

Yes, I think it is fine to not log for trusted schemes, though we might want to give them a unique pseudo-host based on their scheme for purposes of cache isolation just in case.

OK, here is a proposal:

1) Change getFirstPartyURI() to fail and log for all schemes that lack a host that we do not specifically "whitelist" (so schemes like chrome: and about: would not cause a failure but data: and javascript: would).

2) Add the following function to centralize generation of the pseudo hosts (this would be used to by the image loader, DOM storage, etc. to generate cache keys):

  /**
   * getFirstPartyHostForIsolation
   *
   * Obtain the host or pseudo-host for aFirstPartyURI.  Some examples:
   *    aFirstPartyURI                         Return Value
   *    --------------                         ------------
   *    https://news.google.com/nwshp?hl=en    "news.google.com"
   *    about:home                             "--NoFirstParty-abouthome--"
   *    chrome://browser/skin/Toolbar.png      "--NoFirstParty-chrome--"
   *    data:image/png;base64,AAABAA...        <error thrown>
   *
   * @param aFirstPartyURI
   *        The first party URI.
   *
   * @return host or pseudo host.
   *
   * @throws if the URI lacks a host and the scheme is not one for which
   *         we generate a pseudo host.
   */
  AUTF8String getFirstPartyHostForIsolation(in nsIURI aFirstPartyURI);

comment:6 Changed 7 years ago by mikeperry

That sounds good to me.

comment:7 Changed 7 years ago by mcs

Status: newneeds_review

Our proposed fix implements the GetFirstPartyHostForIsolation() proposal and modifies the image loader and DOM storage code to use it (the patch we attached a couple of weeks ago to issue 7265 addresses the canvas issues). We also modified the image loader to avoid putting images in the cache when we cannot isolate the requests (e.g., when the first party URI lacks a host). Mike, please review.

comment:8 Changed 7 years ago by mikeperry

Keywords: MikePerry201306 added

comment:9 Changed 7 years ago by mikeperry

Keywords: MikePerry201307 added; MikePerry201306 removed

comment:10 Changed 7 years ago by mikeperry

Status: needs_reviewneeds_revision

There is an issue with this patch. It looks like you missed a brace pair around an NS_ADDREF() ThirdPartyUtil::GetFirstPartyURI(). Did/Can you test this in a debug build for a while to check that there's no other potential issues like that? If you need some tips hacking gitian to do this, I can give more detailed instructions, but it should be fairly easy to change the 'versions' file to specify a tor-browser.git branch that you create+commit to, to add this patch and to edit the .mozconfig.

Also, maybe we want to try to solve #9336 as part of this? Perhaps the improved logging here will help quickly diagnose that issue?

comment:11 in reply to:  10 ; Changed 7 years ago by mcs

Replying to mikeperry:

There is an issue with this patch. It looks like you missed a brace pair around an NS_ADDREF() ThirdPartyUtil::GetFirstPartyURI(). Did/Can you test this in a debug build for a while to check that there's no other potential issues like that? If you need some tips hacking gitian to do this, I can give more detailed instructions, but it should be fairly easy to change the 'versions' file to specify a tor-browser.git branch that you create+commit to, to add this patch and to edit the .mozconfig.

Thanks for your review. We did test quite a bit with a debug build (but we built it outside of the gitian build method; I guess we should create a remote tor-browser.git branch on torproject.org to make things simpler). You are correct about the missing braces -- sorry! I don't think the failure path is taken often so the missing braces had no bad effect during our testing. Did you see a failed assert or other log message when running the patch in a debug build?

Also, maybe we want to try to solve #9336 as part of this? Perhaps the improved logging here will help quickly diagnose that issue?

We will take a look at #9336 early next week in conjunction with polishing this fix.

comment:12 in reply to:  11 Changed 7 years ago by mcs

Replying to mcs:

We will take a look at #9336 early next week in conjunction with polishing this fix.

We took a look (see comments in #9336). That issue seems somewhat unrelated. We will post a revised patch here soon.

Changed 7 years ago by mcs

Attachment: 8292-patch.txt added

revised patch: add missing braces

comment:13 Changed 7 years ago by mcs

Status: needs_revisionneeds_review

Revised patch posted. Our testing hasn't revealed any problems, but more field testing would be good. Here are the recent changes:

--- a/content/base/src/ThirdPartyUtil.cpp
+++ b/content/base/src/ThirdPartyUtil.cpp
@@ -329,19 +329,20 @@ ThirdPartyUtil::GetFirstPartyURI(nsIChannel *aChannel,
       // At this point, about: and chrome: URLs have been mapped to file: or
       // jar: URLs.  Try to recover the original URL.
       nsCAutoString scheme;
       nsresult rv2 = (*aOutput)->GetScheme(scheme);
       NS_ENSURE_SUCCESS(rv2, rv2);
       if (scheme.Equals("file") || scheme.Equals("jar")) {
         nsCOMPtr<nsIURI> originalURI;
         rv2 = aChannel->GetOriginalURI(getter_AddRefs(originalURI));
-        if (NS_SUCCEEDED(rv2) && originalURI)
+        if (NS_SUCCEEDED(rv2) && originalURI) {
           NS_RELEASE(*aOutput);
           NS_ADDREF(*aOutput = originalURI);
+        }
       }
     }
   }

   // If the channel was missing, closed or broken, try the
   // window hierarchy directly.
   //
   // This might fail to work for first-party loads themselves, but

comment:14 Changed 7 years ago by mikeperry

Keywords: MikePerry201308 added; MikePerry201307 removed

Decided to hold off on this till after 3.0a3 (which is a security fix which we should try to avoid risking destabilizing).

comment:15 Changed 6 years ago by mikeperry

Keywords: MikePerry201309 added; MikePerry201308 removed

comment:16 Changed 6 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, this is merged. Thanks!

FTR: I also broke up the merge into a series of fixup! commits that should make it easier to rebase the new bits onto Firefox 24. I've been doing this with all of the new patches since we started rebasing. You can see this in:
https://gitweb.torproject.org/tor-browser.git/shortlog/refs/heads/tor-browser-17.0.9esr-1

comment:17 Changed 4 years ago by bugzilla

Component: Firefox Patch IssuesTor Browser
Resolution: fixed
Severity: Normal
Sponsor: None
Status: closedreopened

comment:18 Changed 4 years ago by gk

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.