Opened 3 years ago

Last modified 4 months ago

#13818 assigned defect

[PATCH] Active tab looks ugly (inherits system color scheme only partially)

Reported by: gentoo_root Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords:
Cc: qbi, mcs, brade, chielkvard, scootergrisen Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I use Tor Browser 4.5-alpha-1 on KDE, my gtk+ theme is oxygen-gtk. As I found from looking into sources of Tor Browser, when it renders site content, it uses some stand-ins for native colors to avoid browser fingerprinting. And these stand-ins should not be used when rendering browser interface - the variable useStandinsForNativeColors in layout/style/nsRuleNode.cpp:890 (function SetColor):

bool useStandinsForNativeColors = aPresContext && !aPresContext->IsChrome();

But this condition is not enough to fully distinguish browser interface from site content. Look at the attached screenshot to see that left and right corners of active tab are lighter than middle of the tab - this is because the middle renders with system colors, and corners render with stand-ins while stand-ins should be really disabled for them.

I discovered that two files correspond for corners of the tab: chrome://browser/skin/tabbrowser/tab-selected-start.svg and chrome://browser/skin/tabbrowser/tab-selected-end.svg, and IsChrome() function returns false for this files, so stand-ins are used when they shouldn't.

I think that the attached patch should be used in order to handle correctly those two svg files.

Child Tickets

Attachments (7)

tor-browser-tabs-color-fix.patch (689 bytes) - added by gentoo_root 3 years ago.
Patch to fix tab color scheme
tor-browser-tabs-bug.png (29.4 KB) - added by gentoo_root 3 years ago.
How active tab looks like without a patch
tor-browser-tabs-bug-good.png (62.4 KB) - added by gentoo_root 3 years ago.
How active tab looks like with a patch
test.html (161 bytes) - added by gentoo_root 3 years ago.
Fingerprinting test
tor-browser-tabs-svg-test.png (50.8 KB) - added by gentoo_root 3 years ago.
How test.html renders in patched Tor Browser
tor-browser-tabs-svg-test1.png (25.2 KB) - added by gentoo_root 3 years ago.
How test.html renders in unpatched Tor Browser
tor-test-chrome-origin-image.patch (1.0 KB) - added by gentoo_root 3 years ago.
Patch to test IsChromeOriginImage

Download all attachments as: .zip

Change History (31)

Changed 3 years ago by gentoo_root

Patch to fix tab color scheme

Changed 3 years ago by gentoo_root

Attachment: tor-browser-tabs-bug.png added

How active tab looks like without a patch

Changed 3 years ago by gentoo_root

How active tab looks like with a patch

comment:1 Changed 3 years ago by gk

Cc: qbi mcs brade added
Keywords: MikePerry201411R added
Status: newneeds_review

That patch looks good to me. mcs, brade, what do you think?

#13446 is a duplicate.

comment:2 in reply to:  1 Changed 3 years ago by mcs

Replying to gk:

That patch looks good to me. mcs, brade, what do you think?

I think this patch will fix the problem. But brade and I wonder if it will allow fingerprinting? Remote pages can embed chrome:// content, which may allow access to the native colors via getComputedStyle() on the SVG content. Kathy and I will test this (unless someone else beats us to it).

comment:3 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201411R added; MikePerry201411R removed

Changed 3 years ago by gentoo_root

Attachment: test.html added

Fingerprinting test

Changed 3 years ago by gentoo_root

How test.html renders in patched Tor Browser

Changed 3 years ago by gentoo_root

How test.html renders in unpatched Tor Browser

comment:4 Changed 3 years ago by gentoo_root

It seems that my patch affects fingerprinting. I've attached test.html which renders differently with patched and unpatched Tor Browser (look at attached screenshots). Looks like we need to find a better way to solve this bug.

It is also interesting that if I open tab-selected-start.svg directly (using chrome:// url), aPresContext->IsChromeOriginImage() == false, but when it's embedded on the page, IsChromeOriginImage() == true.

comment:5 in reply to:  4 Changed 3 years ago by mcs

Replying to gentoo_root:

It seems that my patch affects fingerprinting. I've attached test.html which renders differently with patched and unpatched Tor Browser (look at attached screenshots). Looks like we need to find a better way to solve this bug.

Just because it renders differently does not necessarily mean there is a fingerprintable leak of information. The question is, can a web site detect the difference? It may be that a site cannot access the chrome:// SVG document (and associated computed styles) due to the same origin security policy. brade and I are trying to test that now.

It is also interesting that if I open tab-selected-start.svg directly (using chrome:// url), aPresContext->IsChromeOriginImage() == false, but when it's embedded on the page, IsChromeOriginImage() == true.

Hmm. It looks to me like it would be the opposite (but maybe I am reading the Mozilla code wrong). Here is where mIsChromeOriginImage is set:

http://mxr.mozilla.org/mozilla-esr31/source/layout/base/nsPresContext.cpp#641

It will only be set to true if IsBeingUsedAsImage() returns true... which sounds like it would be true in the case where you load the SVG as a top-level document.

Changed 3 years ago by gentoo_root

Patch to test IsChromeOriginImage

comment:6 Changed 3 years ago by gentoo_root

It will only be set to true if IsBeingUsedAsImage() returns true... which sounds like it would be true in the case where you load the SVG as a top-level document.

I've just attached a patch (that applies on top of my first patch) to test IsChromeOriginImage. When I load chrome://browser/skin/tabbrowser/tab-selected-start.svg in browser, I see that lines on my console (repeating multiple times):

aPresContext->IsChromeOriginImage() == 0
URI: scheme="chrome"; host="browser"; path="/skin/tabbrowser/tab-selected-start.svg"

And when I load test.html, I see something like that:

aPresContext->IsChromeOriginImage() == 0
URI: scheme="file"; host=""; path="/tmp/.private/max/test.html"
aPresContext->IsChromeOriginImage() == 1
URI: scheme="chrome"; host="browser"; path="/skin/tabbrowser/tab-selected-start.svg"

So IsChromeOriginImage() == false when I load SVG as a top-level document, and true when I embed SVG into HTML page.

comment:7 Changed 3 years ago by mcs

Our tests so far show that a remote page that loads a chrome:// SVG will not be able to access the DOM (which should mean that it cannot access the rendered colors of elements within the SVG). The SVG image loads OK with an <img> tag but the same origin policy blocks loading via tags such as iframe and object. So maybe the original patch is safe.

We could also try checking nsContentUtils::IsCallerChrome() instead of aPresContext->IsChrome(). It bothers me that I cannot remember why we did not use that kind of check in the original patch for #6786.

comment:8 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201412R added; TorBrowserTeam201411R removed

comment:9 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201501R added; TorBrowserTeam201412R removed

comment:10 in reply to:  7 ; Changed 3 years ago by gk

Replying to mcs:

Our tests so far show that a remote page that loads a chrome:// SVG will not be able to access the DOM (which should mean that it cannot access the rendered colors of elements within the SVG). The SVG image loads OK with an <img> tag but the same origin policy blocks loading via tags such as iframe and object. So maybe the original patch is safe.

Thinking a bit about our discussion yesterday. What about blocking access to chrome:// by website content generally? That could be a defense in depth in this case and, personally, I think websites should have no business at all with the browser chrome. The question would then of course be what breaks. But I fear even if we find a solution that works now, subtle and hard to detect changes might force us to regret not taking a more direct and clear-cut approach.

To test a bit more my "we already have a fingerprinting issue"-claim I looked at the test.html on different machines and OSes and it seems that it gets rendered slightly different (the color values seem to not be the same across my systems/devices if I look close at the rendering). But as you mentioned above the question is then if a website could benefit from it (if it were an issue). Canvases should be blocked cross-origin as well. But I have not looked at it closer yet. It might be another reason for trying at least a more clear-cut approach.

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

Replying to gk:

Thinking a bit about our discussion yesterday. What about blocking access to chrome:// by website content generally? That could be a defense in depth in this case and, personally, I think websites should have no business at all with the browser chrome. The question would then of course be what breaks. But I fear even if we find a solution that works now, subtle and hard to detect changes might force us to regret not taking a more direct and clear-cut approach.

Defense in depth seems worth investigating. Kathy and I are going to perform an experiment where we disable all access to chrome:// images from content (there may be other things to block, but we will start with images). There are some hints in this lengthy Mozilla bug report about what might break:

https://bugzilla.mozilla.org/show_bug.cgi?id=292789

e.g.,

  • Remote XUL (not in Firefox since 4.0)
  • Icons in ftp:// listings
  • View source (perhaps CSS-related and not image related?)
  • Feed reader (possibly referring to Thunderbird)
  • Emoticons in (Thunderbird?) editor.

In any case, we will try it and see what happens.

comment:12 Changed 3 years ago by mcs

I forgot to mention that only chrome content that is registered with the contentaccessible=yes flag is accessible from content, so not everything is available to content. But a lot of stuff is.

comment:13 Changed 3 years ago by mcs

OK, as an experiment Kathy and I modified nsScriptSecurityManager::CheckLoadURIWithPrincipal() to block access to chrome:, resource:, and moz-icon: URLs from content (without our change, access is allowed to URLs that are "whitelisted" via the contentaccessible=yes flag chrome registration flag). So what did we break? Some testing on Mac OS revealed the following:

  • FTP listings are very ugly (no icons, no styling).
  • Fav icons are OK.
  • View source is ugly (no stylesheet).
  • The feed reader is broken (JS and CSS not loaded).
  • pdf.js seems to work OK. This is because the security principal is resource://pdf.js/web/viewer.html. In contrast, the security principal that is passed into CheckLoadURIWithPrincipal() when loading an FTP listing is the ftp: URL itself.

So... Kathy and I conclude that a lot of things will break if we completely disable access to chrome:, resource:, and moz-icon: from content. I think it would be a good idea for Mozilla to clean up their architecture and code in this area; it would be a lot for us to take on.

I see that Mike filed #14205 for the general issue of dependance upon IsCallerChrome() and presumably related calls such as presContext->IsChrome().

For this specific bug, Kathy and I think the original patch is OK and should be merged.

comment:14 Changed 3 years ago by mcs

One more thing: Kathy reminded me that gk asked during the last tbb-dev meeting what kind of testing we did to convince ourselves that this patch will not introduce a fingerprinting vector. We created a web page that tried to load chrome://browser/skin/tabbrowser/tab-selected-start.svg several ways:

  • <img src="...">
  • <iframe src="...">
  • <object type="data/svg+xml" ...>

The only image load this was not blocked is the <img>-based one, and (as far as I know) the SVG DOM cannot be accessed from an <img> tag (so there is no way for a web page to ask for CSS computed style in order to grab the color values we use).

comment:15 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201502R added; TorBrowserTeam201501R removed

comment:16 Changed 3 years ago by mikeperry

Well, I am also worried about sizing differences in these chrome/resource svgs. If you can render them, you may be able to detect those differences even if you can't inspect the DOM.

I am also curious what the overlap is between the issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1120398 (aka #14390) and this bug.

comment:17 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201503R added; TorBrowserTeam201502R removed

comment:18 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201504 added

comment:19 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201504R added; TorBrowserTeam201503R removed

comment:20 Changed 3 years ago by mikeperry

Keywords: ff38-esr added; TorBrowserTeam201504 TorBrowserTeam201504R removed

The potential sizing differences still worries me, as does the potential issues with leaking to content.

We should re-invesitate this at some point during Firefox 38-esr. The UI layout will likely change on that timescale, as well, which may impact this behavior.

comment:21 Changed 23 months ago by mcs

Cc: chielkvard added
Severity: Normal

I closed #18155 as a duplicate.
We should take a fresh look at this ticket because, while this is only a cosmetic issue, the tabs are ugly.

comment:22 Changed 20 months ago by mcs

Keywords: ff38-esr removed
Owner: changed from tbb-team to mcs
Status: needs_reviewassigned

comment:23 Changed 11 months ago by gk

Cc: scootergrisen added

Resolved #21252 as a duplicate.

comment:24 Changed 4 months ago by cypherpunks

Closed #23154 as a duplicate.

Note: See TracTickets for help on using tickets.