1403357 - U2F does not work with bitwarden

Status: RESOLVED INVALID

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053838

Steps to reproduce:

Attempt to register a U2F key (YubiKey NEO in my case) with bitwarden password manager (https://bitwarden.com).


Actual results:

Key is never read (does not light up).


Expected results:

Key should be read and registered.

bitwarden's U2F implementation uses the u2f-api.js library from Google. See https://github.com/bitwarden/web/blob/master/src/js/u2f-api.js

When attempting to register a key with bitwarden, the browser console just says "listening for key..." and no result is retured. Implementation can be found here: https://github.com/bitwarden/web/blob/master/src/app/settings/settingsTwoStepU2fController.js#L43

Comment 1

[Kyle]

This specific bug was opened as suggested in the origin U2F topic here: https://bugzilla.mozilla.org/show_bug.cgi?id=1065729

Comment 2

[Kyle]

If relevant, app id for this implementation's challenges is set to https://vault.bitwarden.com/app-id.json .

Comment 3

[andrew]

I am able to reproduce this issue with the Yubikey Series 4.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 4

[Kyle]

Debugging this more I can see that the problem comes from the u2f-api.js library.

Callstack in u2f ends up being register() -> getApiVersion() -> getPortSingleton_(). getPortSingleton_() never invokes it's callback to getApiVersion().

We never get past this line: https://github.com/bitwarden/web/blob/44a2d071aeab3b4d48faf703efda0a1d42741e02/src/js/u2f-api.js#L728

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This is at J.C.'s wheelhouse.

Comment 6

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

We only implement the high-level JS API of U2F v1.1 [1], not the low-level MessagePort based API from v1.0, so calls that attempt to do work via the MessagePort or PortSingleton indicate something bad happening.

Most of the sites that pull in old copies of u2f-api.js from Google "just work" because that API tries to overwrite Gecko's u2f object, which doesn't work, and Gecko's u2f object supports the same high-level interface that u2f-api.js does. 

Since you've seen a call-stack that still tries to talk via MessagePorts, it looks like Bitwarden might be approaching things a bit differently, though your code snippet you link to for Register looks generally fine. That said, perhaps Angular is doing some scoping that's protecting u2f-api.js from being superseded by Gecko's u2f object.

Is there any chance of raising this with the Bitwarden folks? I think they'll have to do some JS changes of some sort to make things work. Theirs is the only time I've ever run into this problem of the u2f-api.js not being overridden successfully, and there's not much we can do on the Firefox side short of re-implementing everything... and since this is just a stepping stone to W3C Web Authentication, that's not appetizing.


[1] https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#high-level-javascript-api

Comment 7

[Kyle]

I'm actually the lead developer of bitwarden and the one responsible for the U2F implementation on bitwarden. I'm no U2F expert, but I'm happy to work with you to help debug this further. This is odd because the existing implementation worked just fine the old Firefox U2F addon and still does with other browsers such as Chrome.

The existing u2f-api.js library used is the latest from https://github.com/google/u2f-ref-code/blob/master/u2f-gae-demo/war/js/u2f-api.js . Are you suggesting that this library is not necessary in Firefox since there is already a window.u2f implementation provided by the browser?

I'm not really sure how AngularJS could be scoping this incorrectly.

Another odd thing that I notice while stepping through the u2f-api.js code is that when it hits u2f.getPortSingleton_ fir the first time, the u2f.waitingForPort_ array already seems to have about 11 callback functions in it.

Comment 8

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

Sorry for the delay in replying, Kyle.

(In reply to Kyle from comment #7)
> The existing u2f-api.js library used is the latest from
> https://github.com/google/u2f-ref-code/blob/master/u2f-gae-demo/war/js/u2f-
> api.js . Are you suggesting that this library is not necessary in Firefox
> since there is already a window.u2f implementation provided by the browser?

Yeah. So if you hit up https://u2fdemo.appspot.com/ or https://u2f.bin.coffee/ and set breakpoints in u2f-api.js, what I find is that they never trigger because the script aborts on line 17 with:

TypeError: setting getter-only property "u2f"

... This is the behavior we're depending on when we hit sites loading in u2f-api.js - basically, that script should fail to execute. Otherwise, weird stuff happens -- which is how I'm now reading your situation. I'm no JS expert, so I'm not really sure why most of the time the script would abort at line 17

var u2f = u2f || {};

... and stop executing the u2f-api.js script, but not on your site, I guess? 


> I'm not really sure how AngularJS could be scoping this incorrectly.

Yes, you're right. So I'm wondering: when you load your page, do you see that TypeError in the console? 

> Another odd thing that I notice while stepping through the u2f-api.js code
> is that when it hits u2f.getPortSingleton_ fir the first time, the
> u2f.waitingForPort_ array already seems to have about 11 callback functions
> in it.

I don't know if this is normal or not, except that I can't get my Firefoxen to make it this far. :)

Comment 9

[Kyle]

That would make sense. Mine definitely does not throw an error. See the following screenshot: https://i.imgur.com/k8Ajhtm.png . In production for bitwarden the u2f-api.js library is minified and bundled with a lot of other application code. It looks like the minification process removes the var declaration from that line, so I am not sure if that changes anything. I guess it's a good thing it isn't erroring there at the moment or all other application code following inside that bundle would not work.

Would an acceptable solution be just to sniff the user agent for Firefox (and for the instance of window.u2f) and just not load u2f-api.js at all if so? I'll give that a try.

Comment 10

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

(In reply to Kyle from comment #9)
> That would make sense. Mine definitely does not throw an error. See the
> following screenshot: https://i.imgur.com/k8Ajhtm.png . In production for
> bitwarden the u2f-api.js library is minified and bundled with a lot of other
> application code. It looks like the minification process removes the var
> declaration from that line, so I am not sure if that changes anything. I
> guess it's a good thing it isn't erroring there at the moment or all other
> application code following inside that bundle would not work.

That makes sense. Fun... I knew when I realized to implement the high-level API we'd have to clobber the polyfill that we'd eventually hit problems. Here, finally, we do. Thanks for helping me debug it!

> Would an acceptable solution be just to sniff the user agent for Firefox
> (and for the instance of window.u2f) and just not load u2f-api.js at all if
> so? I'll give that a try.

That should work, yeah. The only part where there might be weirdness is if you're depending on any of the constants defined in u2f-api.js like u2f.ErrorCodes -- those aren't part of the spec, just the polyfill.

Comment 11

[Kyle]

Good news, it worked! See here for our modified version of u2f-api.js that aborts if an implementation is already supplied by the browser:

https://github.com/bitwarden/web/blob/master/src/js/u2f-api.js

Comment 12

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

(In reply to Kyle from comment #11)
> Good news, it worked! See here for our modified version of u2f-api.js that
> aborts if an implementation is already supplied by the browser:
> 
> https://github.com/bitwarden/web/blob/master/src/js/u2f-api.js

Awesome!

Sorry for the weirdnesses around this. These sorts of compat corner cases are things we're trying to get right with Web Authentication - interop sessions are so necessary.

Going to mark this resolved since it was fixed in the loaded Javascript. It might be worth documenting on the wiki though.