Opened 4 years ago

Closed 4 years ago

#16874 closed defect (fixed)

https://sports.yahoo.com/dailyfantasy is broken with Tor Browser 5.0 on Windows

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-usability-website, tbb-5.0-regression
Cc: brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

As a user on the blog reports (https://blog.torproject.org/blog/tor-browser-501-released#comment-102347):

I am using TOR Browser as it ships; no modifications.
Here is the URL having the issues:

https://sports.yahoo.com/dailyfantasy

If you go to the middle "main" section titled "Daily Fantasy Sports Contests", the links in that section are not functioning, except for the "Enter Contest" button.
You should be able to click on "MLB" or "NFL" or "NBA", and it would take you to specific pages for just those leagues. Additionally, the slider button "Entry Fee" range from "$0 to $500" does not respond. You should be able to slide either end left or right to narrow your range.
Once you do click on "Enter Contest", you should be able to add players to roster by clicking on the "+" sign. However, nothing happens.

This happens with a vanilla Tor Browser 5.0.1 on Windows.

Child Tickets

Change History (20)

comment:1 Changed 4 years ago by gk

Summary: https://sports.yahoo.com/dailyfansty is broken with Tor Browser 5.0 on Windowshttps://sports.yahoo.com/dailyfantasy is broken with Tor Browser 5.0 on Windows

comment:2 Changed 4 years ago by gk

It is somewhat surprising but this is indeed a Windows only issue. Testing on OS X 10.6.8 and Linux boxes is working fine. The first alpha that breaks is 5.0a3.

comment:3 Changed 4 years ago by arthuredelstein

Browsing to https://sports.yahoo.com/dailyfantasy, I noticed that a window.Intl is undefined error appears in the JS console on Windows. This error is caused by our use of --without-intl-api in .mozconfig-mingw (our Windows build). So I wondered if this error might be causing the problem, but our TBB-4.5.3 builds also use --without-intl-api.

I then discovered that a polyfill.min.js script is loaded by the page in TBB-4.5.3 that compensates for the lack of a built-in window.Intl object. This file adds IntlPolyfill and Intl properties to the window object, such that Intl === IntlPolyfill returns true in the JS console. On TBB-5.0.2, the page attempts to load polyfill.min.js, but the server responds with an empty file (200 OK).

The request headers from the two browser versions are almost identical. One difference is the User-Agent, which is controlled by the general.useragent.override pref:

  • TBB-4.5.3 has Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
  • TBB-5.0.2 has Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Firefox/38.0

If I set the general.useragent.override in TBB-5.0.2 to empty, or to

  • Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Firefox/31.0 or
  • Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/38.0,

then the Yahoo server returns a non-empty polyfill.min.js and the links in https://sports.yahoo.com/dailyfantasy are no longer broken.

So there are two possible solutions to this bug: (1) cross-compile Firefox for Windows with the Intl API, and (2) change the User-Agent as above. Solution 1 may not be easy (see https://bugzilla.mozilla.org/show_bug.cgi?id=1019744#c19), but Solution 2 seems like an weird thing to do for an issue specific to this one site. On the other hand, since we have a habit of disabling dangerous new features, there might be an argument for not presenting a User-Agent that claims support for 38.0 features.

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:4 Changed 4 years ago by gk

Thanks for taking this off of my plate and nice analysis! :) So, I agree that (1) is the ideal solution but it probably won't happen in the near future. I am not so happy about (2) as I bet there are a bunch of sites out there that break due to sending such a weird UA and sending none is very likely breaking a lot. The side behavior doesn't actually make sense to me as we already needed to disable ICU (i.e. the Internationalization API) in Tor Browser based on ESR 31. I propose contacting Yahoo to find out what this is all about first and we can decide whether we want to do something based on the response.

comment:5 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201509 added

comment:6 in reply to:  4 ; Changed 4 years ago by arthuredelstein

Replying to gk:

Thanks for taking this off of my plate and nice analysis! :) So, I agree that (1) is the ideal solution but it probably won't happen in the near future. I am not so happy about (2) as I bet there are a bunch of sites out there that break due to sending such a weird UA and sending none is very likely breaking a lot.

Sorry I wasn't clear -- the UA I was thinking of sending is
Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
which is the UA we were sending for TBB-4.5.3. The problem I see with sending the 38.0 UA is that it gives the false impression that we support all 38.0 features.

I agree we could try contacting Yahoo, though I haven't found any obvious way to do so.

comment:7 in reply to:  3 Changed 4 years ago by cypherpunks

Replying to arthuredelstein:

So there are two possible solutions to this bug: (1) cross-compile Firefox for Windows with the Intl API, and (2) change the User-Agent as above. Solution 1 may not be easy (see https://bugzilla.mozilla.org/show_bug.cgi?id=1019744#c19)

ICU can be cross-compiled (with some patches):
https://build.opensuse.org/package/show/windows:mingw:win64/mingw64-icu
https://apps.fedoraproject.org/packages/mingw-icu
https://aur.archlinux.org/packages/mingw-w64-icu/

comment:8 in reply to:  6 Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Thanks for taking this off of my plate and nice analysis! :) So, I agree that (1) is the ideal solution but it probably won't happen in the near future. I am not so happy about (2) as I bet there are a bunch of sites out there that break due to sending such a weird UA and sending none is very likely breaking a lot.

Sorry I wasn't clear -- the UA I was thinking of sending is
Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
which is the UA we were sending for TBB-4.5.3. The problem I see with sending the 38.0 UA is that it gives the false impression that we support all 38.0 features.

I see. The interesting thing is that ICU was already a feature of ESR 31. And switching back to ESR 31 leads very likely to users showing up here complaining about websites saying they should upgrade their browser...

I agree we could try contacting Yahoo, though I haven't found any obvious way to do so.

I tried for quite some while to find a way to send them something without the need for me to have a Yahoo account. I have a case number now! We'll see if that leads somewhere.

I guess what I'll do is spending some cycles on the proposed cross-compilation in the hope Jacek is fixing for us the remaining things once I got that somewhat working. :)

comment:9 Changed 4 years ago by gk

Yahoo wrote back:

I apologize for the inconvenience you have had with viewing Daily Fantasy on the Tor browser. Unfortunately, there isn't a tech that is available to speak to you about this. I would suggest to please use the most current version of Firefox non beta and this will resolve your issue.

That settles it I guess.

comment:10 Changed 4 years ago by mikeperry

What do we think about the patches the cypherpunk linked in comment:7?

comment:11 in reply to:  10 Changed 4 years ago by gk

Replying to mikeperry:

What do we think about the patches the cypherpunk linked in comment:7?

Cross-compiling ICU outside of the way Firefox is doing it seems to be working for me even without the patches. Just the Firefox thing is breaking and I am working on it in #13419. So, I guess the patches won't help directly. But maybe the will help me understand what goes wrong in the Firefox case.

Version 0, edited 4 years ago by gk (next)

comment:12 Changed 4 years ago by mikeperry

If we can't get ICU to build, another alternative is to write a stub (ideally in C++, but as a last resort in Torbutton) to fill in window.Intl with a simple object on Windows. I'm not sure I'll have time to do this myself, though.. :/

comment:13 Changed 4 years ago by mcs

Cc: brade mcs added

Kathy and I looked at this a little with an eye toward adding an Intl object. It looks like for the sports.yahoo.com page, minimally we would need to implement Intl.NumberFormat() and Intl.DateTimeFormat() and both of those would need to return an object that had a format() method. We do not have more time to work on this today, but will look at it again tomorrow unless someone beats us to it. The best solution would be to solve the cross-compile issues, but I suspect that is difficult too.

comment:14 Changed 4 years ago by arthuredelstein

I remembered I had written a function that injects scripts into content documents in Firefox, so I tried using it to inject the Intl.js polyfill from https://github.com/andyearnshaw/Intl.js/.

Here's the patch:
https://github.com/arthuredelstein/torbutton/commit/16874+1

Obviously, a fix for #13419 should be preferred to this approach. Nevertheless, this patch does fix the issue with https://sports.yahoo.com/dailyfantasy and it provides a presumably faithful implementation of Intl.NumberFormat and Intl.DateTimeFormat.

comment:15 Changed 4 years ago by mikeperry

I'm unfamiliar with the new Services.scriptloader.loadSubScript API, but looking it up, it seems common for people to use https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.Sandbox with it still, to cause the script to evaluate with content privileges..

Are you sure that what you did with XPCNativeWrapper.unwrap() instead is safe there?

There's two main sources of risk when doing stuff like this:

  1. Adding Intl.js from chrome could cause the Intl.js expandos to actually have chrome privileges inside the content window. This seems unlikely, but again I'm not sure.
  2. The polyfill itself may be running with chrome privs when installing itself, which means that if it touches an unwrapped window property, it may be induced to execute content code (in the form of a getter/setter, perhaps) that way, in a privileged setting.

For some reason, I can't seem to find any documentation on either of these risks. Perhaps that's because this new API is magically safe no matter how you use it? Maybe this is a question for #security on irc.mozilla.org, and/or some testing?

comment:16 in reply to:  15 Changed 4 years ago by arthuredelstein

Replying to mikeperry:

I'm unfamiliar with the new Services.scriptloader.loadSubScript API, but looking it up, it seems common for people to use https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.Sandbox with it still, to cause the script to evaluate with content privileges..

Are you sure that what you did with XPCNativeWrapper.unwrap() instead is safe there?

I don't why I had XPCNativeWrapper.unwrap() there -- I've removed it.

There's two main sources of risk when doing stuff like this:

  1. Adding Intl.js from chrome could cause the Intl.js expandos to actually have chrome privileges inside the content window. This seems unlikely, but again I'm not sure.
  2. The polyfill itself may be running with chrome privs when installing itself, which means that if it touches an unwrapped window property, it may be induced to execute content code (in the form of a getter/setter, perhaps) that way, in a privileged setting.

The documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader#loadSubScript%28%29 claims that "The loaded script is executed with the principal associated with the target object." So that's at least promising.

First I confirmed that the principal for the contentWindow object is non-chrome:

> Components.utils.getObjectPrincipal(gBrowser.selectedBrowser.contentWindow)
XPCWrappedNative_NoHelper { URI: XPCWrappedNative_NoHelper, origin: "https://arthuredelstein.github.io", jarPrefix: "", baseDomain: "arthuredelstein.github.io", appStatus: 0, appId: 0, isInBrowserElement: false, unknownAppId: false, isNullPrincipal: false }

Then for a more direct test, I added the line

console.log("Components.interfaces:", Components.interfaces);

to the top of Intl.js. In that case, every loaded page logs an error:

ReferenceError: Components is not defined

I didn't immediately think of any further tests that might approach the question in another way.

For some reason, I can't seem to find any documentation on either of these risks. Perhaps that's because this new API is magically safe no matter how you use it? Maybe this is a question for #security on irc.mozilla.org, and/or some testing?

You're right that we should be very careful about this. I've posted a question there. In the meantime, here's the new version without XPCNativeWrapper.unwrap:

https://github.com/arthuredelstein/torbutton/commit/16874+2

I suppose one alternative would be to insert a <SCRIPT src="resource://..."> element to the content page's DOM once it has started loading.

comment:17 Changed 4 years ago by arthuredelstein

Bobby Holley had a good suggestion here:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/OWGw4Ep7AEM

So here's a new version of the patch that uses contentWindow.eval(...) instead of Services.scriptloader.loadSubScript:
https://github.com/arthuredelstein/torbutton/commit/16874+3

comment:18 Changed 4 years ago by gk

Keywords: TorBrowserTeam201510 added; TorBrowserTeam201509 removed

Moving Tor Browser tickets to October 2015.

comment:19 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201510 removed

comment:20 Changed 4 years ago by gk

Resolution: fixed
Severity: Normal
Status: newclosed

Fixed by #13419 which will be available in 6.0a5.

Note: See TracTickets for help on using tickets.