1406458 - WebAuthn: Add extension types

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P1)

Description

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

We'll need to be able to support certain Web Authentication extensions, so we need to add all the WebIDL related to extensions. (That WebIDL also is required to pass the Web Platform Tests.)

Comment 1

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

[@bz - This is _not_ time-critical, as the only extension we have any current plans to handle is Bug 1406471, and we could ship without it.]

The Web Authentication extensions WebIDL type is a "typedef record<DOMString, any>". Adding this to our WebIDL prompts an assertion in "/Users/jcjones/hg/mozilla-central/dom/bindings/Codegen.py", line 13694, in getMemberTrace, which has the comments triggered on this being a record type:

        elif type.isRecord():
            # If you implement this, add a record<DOMString, object> to
            # TestInterfaceJSDictionary and test it in test_bug1036214.html
            # to make sure we end up with the correct security properties.
            assert False


Looking in test_bug1036214.html (and Bug 1036214 itself) I see tests for pingPongMap which is of the same record<DOMString, any> construction:

  DOMString pingPongMap(record<DOMString, any> map);

However, I don't understand exactly the security situation I need to be confident in, so I need some help.

The general model for extensions is that going through the WebIDL will be dictionaries with a variety of keys [3]. In the extension field, we should take an input dictionary, run it through a whitelist of types, validate it [2], encode it to CBOR, and supply it (if appropriate) to the physical token via the wire protocol. 

Similarly, extensions received on the wire protocol from the physical token have to be evaluated against a whitelist, validated, converted from CBOR to a JS dictionary, and then returned to the client. (Note: return mechanism has a bug [4])

"Validation" of each of these things on the whitelist is unique per-extension.

When it comes time to implement this, I'm not clear on what I need to do to proceed here with regard to test_bug1036214.html or the TestInterfaceJSDictionary WebIDL type that already has a record<DOMString, any> defined.

What would my next steps be?

Thanks!

[1] https://w3c.github.io/webauthn/#iface-authentication-extensions
[2] https://w3c.github.io/webauthn/#client-extension-processing
[3] https://w3c.github.io/webauthn/#sctn-example-extension
[4] https://github.com/w3c/webauthn/issues/624

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem is not having a record<DOMString, any>.  As you noticed, we have those already and we check that the right things happen.

The problem is having a _dictionary_ with a member of type "record<DOMString, any>".  Right now we don't support those.  Supporting them involves changing the code you quote to actually trace the JS::Values stored in the record, as a first step.

The security bits involved are these: if page A has a cross-origin object (a Window or Location in practice, coming from a different-origin page B) we want to make sure that it can't pass that object to JS-implemented APIs.  Bug 1036214 addressed that by adding security checks in such APIs.  At the time, we could not verify whether those security checks do the right thing with a dictionary containing a record type, because we didn't support such types, so couldn't test the behavior.

So what the comment says to do is that if we add such support then we should add a record type whose values are "any" or "object" to TestInterfaceJSDictionary and then examine the resulting generated code to make sure that the right CallerSubsumes() checks are happening on the values going into that record.  Also add tests to test_bug1036214.html that try to pass a cross-origin object as a value in a record to a JS-implemented WebIDL API and make sure such attempts fail.

None of that has anything to do with the actual things you end up doing with the JS::Value in your implementation of the extension bits.  That will need its own careful security checking...  At first glance, all that's supposed to happen is "encode the given Value to CBOR", but this process is badly underspecified in the spec right now.  It probably needs to be specified.

> or the TestInterfaceJSDictionary WebIDL type that already has a record<DOMString, any> defined

It doesn't have such a thing defined right now, because we don't support it in dictionaries right now...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, the most important question: next steps.  Either your or someone else (me, qdot, peterv are the obvious options) adds support for needs-tracing record types in dictionaries.  The first step there is adding the tracing code.  The second step is adding the tests described above and auditing the generated code, both for the "pass the dictionary" bit and for the tracing, to make sure it looks reasonable.

Comment 4

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

Thanks, bz. As I said, this isn't time critical. The only extension we're aiming toward is the one to add FIDO U2F compatibility [1], which only takes in really a record<DOMString, DOMString> and emits record<DOMString, boolean> - I'm not sure how much that simplifies things, if at all.


[1] https://w3c.github.io/webauthn/#sctn-appid-extension

Comment 5

[Boris Zbarsky [:bzbarsky]]

There is no problem with either of those in a dictionary.  The only problem is with "object" or "any" types as the values of a record.

That said, if the spec says "record<DOMString, any>" and you need to end up converting the Values to strings in the spec prose, effectively, then (1) the spec needs to define how this happens and (2) declaring it as "record<DOMString, DOMString>" will give observably different behavior from in some edge cases...

Comment 6

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

Passing to Tim, who has more free time to run this down.

Comment 7

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

TPAC update -- OK, good news everyone. We're going to remove this dictionary and define extensions in the WebIDL as a BufferSource of the raw CBOR data.

Tim: We'll need to handle enough CBOR decoding to identify the extensions we care about, and discard ones we don't.

I'll update when the spec change is in place.

Comment 8

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

Update: We might not do this CBOR thing. But the good news is that we ignore extensions right now just fine, so our implementation effectively strips all extensions safely. Unless the web platform tests define some extensions as mandatory -- which is not currently the case -- this bug shouldn't be an issue for compatibility.

Comment 9

[Phabricator Automation]

Attachment: Bug 1406458 - Add WebAuthn extension types r=jcj

Comment 10

[Phabricator Automation]

Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D555

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

Andrea, can you please sign off the WebIDL changes here? Thanks!

Comment 12

[Phabricator Automation]

Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

Andrea Marchesini [:baku] has approved the revision.

https://phabricator.services.mozilla.com/D555

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24c555a59d8
Add WebAuthn extension types r=jcj,baku

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d24c555a59d8