1551282 - Pearson MyCloud breaks if FIDO U2F is not Chrome's implementation

Status: VERIFIED FIXED

(Core :: DOM: Web Authentication, defect, P1)

Description

[tracy.mckibben]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.131 Safari/537.36

Steps to reproduce:

Attempt to open https://mycloud.pearson.com using nightly

Actual results:

Partial page is displayed

Expected results:

Open same page in stable 66.0.5 (64-bit), compare

Comment 1

[Alice0775 White]

Attachment: Screenshot bad vs good

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=67251e93f756baf940c1ddc9a382b35b76dd4f75&tochange=e27fc0c01a979c6b8a423846e0461bdebe70eef4

Regressed by: e27fc0c01a979c6b8a423846e0461bdebe70eef4 J.C. Jones — Bug 1539541 - Enable FIDO U2F API, and permit registrations for Google Accounts r=keeler,qdot

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:No login form appears on Nightly68.0a1 and Firefox67.0b19

Comment 3

[J.C. Jones [:jcj] (he/they)]

Error in console is TypeError: setting getter-only property "u2f" which is fairly normal on FIDO U2F-supporting websites, but doesn't block load whenever I've seen it before.

It should be possible to make attempting to write over the read-only, FIDO-specified u2f attribute not a fatal error. Probably by making it overwritable.

A workaround would be to open about:config and set security.webauth.u2f back to false.

Comment 4

[J.C. Jones [:jcj] (he/they)]

Chrome gets away with this by not making window.u2f a read-only property, rather they require a polyfill (u2f-api.js) to set up their implementation.

A fast solution here may be to set [LenientSetter] on the webidl for U2F:
https://heycam.github.io/webidl/#LenientSetter

I'll produce a patch to test while inquiring wider.

Comment 5

[J.C. Jones [:jcj] (he/they)]

There's a try run with [LenientSetter] set:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ee69ffdebc5845ffb6947d8f224e6ffd15ec56f&selectedJob=246204770

However, this makes U2F stop working in most cases, as Chrome's polyfill overwrites Firefox's implementation, breaking it.

I'd need a mechanism that ignores errors caused by trying to write to window.u2f rather than permitting it. That could still break code obfuscators that try to use u2f as a variable, though.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Update: I don't see a programmatic fix for this. Generally, websites include the u2f-api.js polyfill [0][1] in its own file with 'use strict'[1] so that when it attempts to overwrite window.u2f a TypeError is thrown and the script returns. Then Firefox works fine.

Pearson includes further code in their login.scripts file [1] after the U2F polyfill, and that further code fails to run since the TypeError causes the script to abort early. That code is important.

The only way we could make Firefox compatible with the Chrome polyfill would be to ... well, be Chrome.

Pearson needs to adjust their login.scripts file.

In the mean time, affected users can toggle-off U2F support in about:config: "security.webauth.u2f"

Marking webcompat? to raise the issue for evangelism ahead of the 67 release. I'll also reach out to the webcompat teams.

[0] https://github.com/google/u2f-ref-code/tree/master/u2f-chrome-extension
[1] https://u2fdemo.appspot.com/js/u2f-api.js
[2] https://mycloud-login.pearson.com/auth/login.scripts

Comment 7

[J.C. Jones [:jcj] (he/they)]

Tweeted @ PearsonSupport: https://twitter.com/ciphercoffee/status/1128070821375668224

Comment 8

[J.C. Jones [:jcj] (he/they)]

Moving to WebCompat, as this isn't something we can fix.

(Sadly, this is an example that FIDO U2F's JS specification isn't really a standard.)

Comment 9

[Pascal Chevrel:pascalc]

Not actionable on our side, so not tracking for 67.

Comment 10

[Boris Zbarsky [:bzbarsky]]

However, this makes U2F stop working in most cases, as Chrome's polyfill overwrites Firefox's implementation, breaking it.

I'm not sure I follow this. [Replaceable] would have that behavior, but [LenientSetter] should just make the assignment a no-op (as it would be in non-strict mode), right? Why can we not do that?

Comment 11

[J.C. Jones [:jcj] (he/they)]

The effect of setting LenientSetter was that our builtin window.u2f was overwritten on my selection of u2f tests sites. For https://u2f.bin.coffee/, https://u2f.bin.coffee/u2f-api.js (the generally-used polyfill) successfully wiped out window.u2f. Same happened on https://u2fdemo.appspot.com/

Perhaps I didn't update the WebIDL properly?

https://hg.mozilla.org/try/rev/007fc044d540bf73897ab23498f5ae627767ffb1

Comment 12

[Boris Zbarsky [:bzbarsky]]

The change to the Web IDL is right After that change, the behavior in strict and non-strict mode should be identical: the setter should no-op, but not change the value of window.u2f...

Thank you for the link to the polyfill; now I understand what's going on. The polyfill itself is in strict mode, so without that change the polyfill throws on this line:

var u2f = u2f || {};

and then none of the rest of it runs. With the change, that line is a no-op, and I expect the issue are the lines that do:

u2f.register = function(appId, registerRequests, registeredKeys, callback, opt_timeoutSeconds) {
...

and

u2f.sign = function(appId, challenge, registeredKeys, callback, opt_timeoutSeconds) {
...

which end up shadowing the Gecko-provided properties of the same names on U2F.prototype. I'll think a bit about whether there's anything we can do here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think I have a possible fix. We'll have to see how web-compatible it ends up being...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1551282 and bug 1553436. Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object. r=jcj,smaug

There are two related problems this patch is trying to address. The first, and
simpler, one is bug 1553436: there are websites that use existing variables and
functions named "u2f" and adding a non-replaceable readonly property with that
name on Window breaks them. The fix for this is straightforward: mark the
property [Replaceable].

The second problem, covered by bug 1551282, involves sites that use the Google
U2F polyfill. The relevant parts of that polyfill look like this:

  'use strict';
  var u2f = u2f || {};
  u2f.register = some_function_that_only_works_right_in_Chrome;
  u2f.sign = some_function_that_only_works_right_in_Chrome;

The failure mode for that code before this fix is that the assignment to "u2f"
throws because it's a readonly property and we're in strict mode, so any code
the page concatenates in the same file after the polyfill does not get run.
That's what bug 1551282 is about. The [Replaceable] annotation fixes that
issue, because now the polyfill gets the value of window.u2f and then redefines
the property (via the [Replaceable] setter) to be a value property with that
value. So far, so good.

But then we need to prevent the sets of u2f.register
and u2f.sign from taking effect, because if they are allowed to happen, the
actual sign/register functionality on the page will not work in Firefox. We
can't just make the properties readonly, because then the sets will throw due
to being in strict mode, and we still have bug 1551282. The proposed fix is to
make these accessor properties with a no-op setter, which is exactly what
[LenientSetter] gives us.

The remaining question is what values the getters for the "register" and "sign"
properties should return. To preserve the ability to do
u2f.register()/u2f.sign(), we want to return callable values, and we want the
call to apply Web IDL argument processing to the arguments. The simplest way
to do this in implementation terms is to return IDL objects with a legacycaller
on them. The other option would be to return actual JS functions defined via a
JSNative that manually implements the Web IDL argument semantics. While this
is arguably closer to the spec for this, I doubt anyone really depends on these
objects actually being Function objects, and it would be a lot riskier to
backport that sort of handwritten binding code to beta, much less release, as I
expect we will need to do with this patch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9067116 [details]
Bug 1551282 and bug 1553436. Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object. r=jcj,smaug

Beta/Release Uplift Approval Request

• User impact if declined: Various sites do not work properly, whether due to this bug (concatenating other scripts with the u2f polyfill) or bug 1553436 (minified code that uses "u2f" for a random variable or function name being broken).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0. Also see bug 1553436 comment 0 and bug 1553436 comment 1.
• List of other uplifts needed: 1554354
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This changes the web-facing behavior of the window.u2f property pretty substantially, but in the smallest possible way that fixes bug 1553436. This part doesn't have any other alternatives that I can see.

It also changes pretty radically the exact sort of object that u2f.sign and u2f.register return. I am hoping this change is not one that anyone will notice in practice, because people just call these methods, but if sites are applying toString to these objects the behavior will change, and so will .length, etc.

We could change the fix so we return actual Function objects, but that would have its own risks due to needing to hand-write the Web IDL argument conversions instead of leveraging the existing codegen. I am not sure how comfortable I would be uplifting the result to release. Though maybe I could more or less copy/paste the generated code to mitigate the risk.

We could also possibly mitigate the issues I describe above by defining various things that normally live on Function.prototype or Function objects (e.g. length) on the new callables. There would still be some risk if someone does Function.prototype.toString.call(u2f.sign) (which will throw) or whatnot. I would be happier going the "paste the generated code and return actual Functions" route than trying to make things look like functions.

• String changes made/needed:

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/823ab2e5430a
and bug 1553436.  Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object.  r=jcj,smaug

Comment 17

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 823ab2e5430a (Bug 1551282) for Windows build bustages CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=c347f878342197b00ce58f30d86d1b436abfd6ac&selectedJob=248233672
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248233672&repo=autoland&lineNumber=37692

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b21953499bea
Backed out changeset 823ab2e5430a for Windows build bustages CLOSED TREE

Comment 19

[Boris Zbarsky [:bzbarsky]]

The Linux linker was OK with the extern declaration being for JSJitInfo but the definition being const JSJitInfo, but the Windows linker was not. I have to admit, the Windows linker behavior makes a lot more sense to me...

Relanded with that fixed.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96235b29702a
and bug 1553436.  Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object.  r=jcj,smaug

Comment 21

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/96235b29702a

Comment 22

[tracy.mckibben]

Following up as the originator of this bug - confirmed fixed. Thanks for the quick fix!

Comment 23

[Boris Zbarsky [:bzbarsky]]

You're welcome! Thank you for reporting it!

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9067116 [details]
Bug 1551282 and bug 1553436. Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object. r=jcj,smaug

fix u2f compat issue, approved for 68.0b5

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cad154da3d81

Comment 26

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 68.0a1 (2019.05.13) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 69.0a1 (2019.05.26) Win 10 x64, Ubuntu 16.04 x64 and macOS 10.14.5,
Pearson MyCloud page is displayed and there are no errors in browser console. I will verify tomorrow on beta 68.0b5 .

Comment 27

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 68.0b5 on Win 10 x64, Ubuntu 16.04 x64 and macOS 10.14.5.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

How common do you think this issue might be (both for this bug and for bug 1553436)? Should this drive a dot release (or even potentially be considered for inclusion in the Trailhead release?) If it can wait for a dot release subsequent to 67.0.1 (Trailhead) then that's what I'd prefer.

Comment 29

[Boris Zbarsky [:bzbarsky]]

How common do you think this issue might be (both for this bug and for bug 1553436)?

I don't know.

The issue described in this bug is probably somewhat rare; we only have one site that hit it so far.

The issue in bug 1553436 ... I don't know. It's claimed to be a general problem with GWT apps, which would make it somewhat common. We haven't had any other reports so far, though....

J.C., any thoughts?

My gut feeling is that a dot release subsequent to 67.0.1 is probably ok, given what we know so far.

Comment 30

[Boris Zbarsky [:bzbarsky]]

It's possible that bug 1552450 is a duplicate of bug 1553436, by the way: it's a GWT app and it's having "u2f" used as a constructor, which is a bit odd unless it's defining its own "u2f" thing.

Comment 31

[J.C. Jones [:jcj] (he/they)]

I agree with you, bz -- I think it's often enough to deserve being in a dot release, but I don't think it has to drive one or force its way into one in in a risky fashion.

It's one of those things where when a page fails to load, people likely retry in Chrome, sadly, and may not bother with a bug report.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, let's plan on uplift for 67.0.2. Thanks!

Comment 33

[Pascal Chevrel:pascalc]

Comment on attachment 9067116 [details]
Bug 1551282 and bug 1553436. Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object. r=jcj,smaug

P1 regression in 67 affecting webcompat, fix verified on beta, approved fo 67.0.2, thanks.

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/9bc4ce49c7bf

Comment 35

[Cristian Fogel [:cfogel]]

Adding QE+ for 67.0.2-build 2.

Comment 36

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 67.0.2-build2 on Win 10 x64, Ubuntu 16.04 x64 and macOS 10.14.5.