Closed Bug 1406458 Opened 5 years ago Closed 5 years ago

WebAuthn: Add extension types


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




Tracking Status
firefox57 --- unaffected
firefox60 --- fixed


(Reporter: jcj, Assigned: ttaubert)



(Whiteboard: [webauthn][webauthn-wd07])


(1 file)

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.)
Blocks: 1406471
Assignee: nobody → jjones
[@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/", 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?


Flags: needinfo?(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...
Flags: needinfo?(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.
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.

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...
Passing to Tim, who has more free time to run this down.
Assignee: jjones → ttaubert
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.
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.
Depends on: 1436308
Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

J.C. Jones [:jcj] has approved the revision.
Attachment #8948945 - Flags: review+
Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

Andrea, can you please sign off the WebIDL changes here? Thanks!
Attachment #8948945 - Flags: review?(amarchesini)
Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

Andrea Marchesini [:baku] has approved the revision.
Attachment #8948945 - Flags: review+
Comment on attachment 8948945 [details]
Bug 1406458 - Add WebAuthn extension types r=jcj

Attachment #8948945 - Flags: review?(amarchesini)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla60
You need to log in before you can comment on or make changes to this bug.