agl doesn't understand extensions

[agl]

(This is just a request for clarification to @selfissued, but I'm writing it as an issue in case it ends up being helpful for others to see it.)

In the course of implementing the appid extension, I hit a few points where my understanding is deficient:

Why are the client inputs contained in the CollectedClientData? I see that the RP could learn from this which extensions the client supports but, if so, a) why also echo a boolean to indicate this in appid and b) why echo the full input when a boolean would do? (Echoing the full input value is a pain because we're unmarshaling them from Javascript but then have to marshal internal structures back to JSON for this.)

Is the RP expected to verify that its inputs were faithfully reflected? The instructions say (step 7) to do a subset check, which could imply checking the full value, but it says the same thing about the authenticatorExtensions, and the RP can't check the value of those. (Unless it's supposed to verify the client's translation to CBOR? If so, why?)

Even if the RP does verify the extension fields in the CollectedClientData, that doesn't mean that the client passed those exact values to the authenticator via CTAP2 so I'm unclear why the RP should bother.

In a similar vein, why are the authenticator inputs included in the CollectedClientData too?

On the way back out, step 18.3 of these instructions says that [[clientExtensionsResults]] is an “ArrayBuffer … containing the bytes of assertionCreationData.clientExtensionResults”. But the latter is a AuthenticationExtensionsClientOutputs so how is that turned into a bytestring?

Lastly, why is getClientExtensionResults a function and not simply a member of the PublicKeyCredential with type AuthenticationExtensionsClientOutputs? That seems uniquely out of place in this API.

Thank you for any clarity you can provide.

[emlun]

Lastly, why is getClientExtensionResults a function and not simply a member of the PublicKeyCredential with type AuthenticationExtensionsClientOutputs? That seems uniquely out of place in this API.

This was discussed in #624. The premise for that discussion could be out of date at this point, though.

[agl]

This was discussed in #624. The premise for that discussion could be out of date at this point, though.

Thanks for the history. Since AuthenticationExtensionsClientOutputs is no longer a record, it may be that quirk is now vestigial.

[selfissued]

@agl since I'm not a JavaScript expert, can you write down exactly what current source text you are proposing to change to what different source text? You clearly have a specific change in mind.

[agl]

I don't have any specific change in mind, I just implemented support for appid yesterday and realised that I don't understand the reason for a lot of the current behaviour. For example, what bad thing can happen if RPs don't check the that the extensions in the CollectedClientData are a subset as they're supposed to? Since the empty set is a subset, can Chrome just leave these things empty and everything's cool?

If forced to pose the question in the form of a change: what objections arise from removing the clientExtensions and authenticatorExtensions from CollectedClientData?

[selfissued]

Sorry - I should have been clearer in my request. You seemed to want to change the syntax of getClientExtensionResults in some way. What was the specific syntax change you had in mind for it?

[agl]

It appears that the function-like nature of getClientExtensionResults is no longer needed and that we may be able to embed a AuthenticationExtensionsClientOutputs attribute in PublicKeyCredential now. Although that is a minor point and a technical, Javascript question that I'm not well placed to have an opinion on.

The main thrust is that, while implementing an extension, I have found that I don't understand the overall structure and am worried that, if I don't know why some of these things exist, I may be making poor implementation choices.

[arnar]

I'm fairly confused myself, so I did some digging in the history because as originally proposed, extensions were pretty simple.

• Extensions take only one input, a JS object.
• The client processing defines
  • (optionally) A JSONable value to be added to clientData
  • (optionally) A CBORable value to be sent as the extension input to the authnr
• Authenticator processing defines (optionally) an output value to be embedded in the signed data.

If forced to pose the question in the form of a change: what objections arise from removing the clientExtensions and authenticatorExtensions from CollectedClientData?

This was introduced in this commit for which I cannot find any PR or discussion. Previously the clientData only contained one field for a map of the optional client data additions made by extensions. I think I would have disagreed with that change if I reviewed it, but maybe there is a good reason that was discussed on the calls. Mike, do you remember?

Client was allowed to ignore any extension, but also to forward the input object to the authnr if the canonical JSON-CBOR conversion (from RFC7049) was possible.

Extension definitions themselves were expected to include sentinels if necessary to indicate to the RP that processing took place. This was explicitly called out, see e.g. here, but abandoned in this commit.

[selfissued]

clientExtensions and authenticatorExtensions are included in CollectedClientData so they contribute to the clientDataHash input. This cryptographically binds these input values to the request. The request would mean something different with different extension inputs, so it makes sense to do this.

Answering @arnar 's history questions, this was part of PR #425 , which @jyasskin thoroughly reviewed at the time. This was part of the fixes to issue @jyasskin 's issue #418 (along with PR #421 ). This was all actually cleanup to earlier PRs to address @jyasskin 's issue #270 (Processing model for extensions is very underdefined). PR #389 did most of the heavy lifting of making the extension model well-defined and useful. (Among other huge defects, there used to be no distinction between the client and authenticator inputs and outputs, even though the client ones are JavaScript and the authenticator ones are CBOR.)

Answering @agl 's question about getClientExtensionResults, I'd be fine replacing: AuthenticationExtensionsClientOutputs getClientExtensionResults(); with [SameObject] readonly attribute AuthenticationExtensionsClientOutputs clientExtensionResults; in PublicKeyCredential, provided a couple of browser experts such as yourself and @jcjones agree. If there's consensus to do this, I'll create a PR (coordinating with @nadalin about the repository being locked).

[emlun]

This cryptographically binds these input values to the request.

Who is the binding for? If we assume the RP trusts the authenticator but not the client, there's actually no guarantee to the RP that those authnr ext inputs are what was actually sent to the authenticator (unless their values can be verified using the corresponding authnr ext outputs, of course). For that the authehticator would need to sign over the extensions parameter to the authenticator ops (and possibly also return it?), but the signature currently only includes the authnr ext outputs.

For the client exts I suppose we need to assume the RP also trusts the client, so it seems reasonable to include the client ext inputs in the CollectedClientData. But then on the other hand there is no cryptographic proof of the integrity of the client ext outputs. For that the client ext outputs would also need to be included in the CollectedClientData.

[agl]

Indeed, for any authenticator-processed extensions, the RP can only trust the authenticator output and so that must contain the input to confirm it. For example, in the way that the txAuthSimple extension does.

For client extensions, I agree that does imply that the client is trusted but I can imagine that the RP doesn't trust their origin context. (I.e. the RP is a backend system which issues commands, but the webauthn requests are actually made by Javascript running in the origin, which may have been compromised by an XSS attack. I'm not sure to what extent this is a sensible thing to worry about, but I'll take it as a premise.)

In this case, echoing the extension inputs in the CollectedClientData eliminates the possibility that the origin context has manipulated the request. Except:

1. If a compromised origin context is something that we expect to defend against, there are a lot of fields in a creation request that are not echoed and so not protected. Thus it seems that if this threat was intended to be addressed, the solution is incomplete and protecting extensions is insufficient.
2. As emlun notes, the client's outputs can still be manipulated by the origin context. So, for example, a compromised origin context could falsely assert that biometricPerfBounds were acted upon.
3. The client doesn't echo the extensions as given, rather the subset that it supports. Thus the origin context can suppress extensions and make it look like the client doesn't support them.
4. The instructions say only that the RP should check that the echoed extensions are a subset, but it's unclear if that means that the values must be identical, esp in light of the fact that the RP is also instructed to check that the authenticator extensions are a subset, where it cannot check the values. If values aren't checked, a compromised origin context can still manipulate the contents of an extension.
5. Since the CollectedClientData is JSON serialised, that means that all extensions have to be JSON—i.e. no ArrayBuffers, WebCrypto objects etc. (Unless we also define a Javascript to JSON mapping for these types.)

I'm still unsure of a purpose for echoing the authenticator extension inputs.

[emlun]

4. The instructions say only that the RP should check that the echoed extensions are a subset, but it's unclear if that means that the values must be identical [...]

I read this to mean the extension IDs (the keys of the object) should be a subset of the requested extension IDs, but I agree this could be clearer. I also think the RP ops should probably have an additional step that vaguely specifies the extension output values are "as expected". I can take care of that in #804.

[selfissued]

Yes, this is about comparing the extension IDs. I can be more explicit about this if we decide to apply any edits before CR (or I can do it after CR).

[agl]

I don't think clarifying the existing processing moves the spec to a coherent place. I think the major decision is whether a compromised origin context is something we wish to defend against. If so:

1. The echoed extensions need to be included verbatim so that the RP can check that the origin context didn't manipulate them at all. That suggests to me that they should be specified in the first place as a JSON string to simply the comparison and to make it clear that they're limited to JSON values.
2. The client extensions outputs also need to be included to prevent the origin context from manipulating them. (See the case of the biometricPerfBounds example cited above.)
3. Other values from the credential request need to be included because, if this is an attack that we're defending against, then we should do it uniformly.

If we believe that a compromised origin context is not something that we defend against (which I think is a reasonable position too) then the echoed extensions should be dropped.

In both cases I believe the echoed authenticator inputs should be dropped.

It appears that we can simplify the getClientExtensionResults function into a mere field again if J. C. agrees. That's relatively minor, however.

[arnar]

I agree that binding the extension inputs isn't consistent with how we bind other inputs. I'd lean towards the previous attacker model that a compromised origin context isn't something we defend against.

Also agree binding authenticator inputs don't make sense. The authenticator outputs are the signed-over statements made by authenticators and should provide sufficient data to the RP to verify whatever the extension is to provide.

This part in the spec seems confusing as well:

Likewise, the client extension outputs are represented as a dictionary in the result of getClientExtensionResults() with extension identifiers as keys, and the client extension output value of each extension as the value. Like the client extension input, the client extension output is a value that can be encoded in JSON.

Extensions that require authenticator processing MUST define the process by which the client extension input can be used to determine the CBOR authenticator extension input and the process by which the CBOR authenticator extension output can be used to determine the client extension output.

Extensions can involve both client and authenticator processing. Client extension outputs should only depend on the former, and not authenticator outputs which are fully independent. Client outputs should be fully defined before authenticator is invoked. This also means there is no ambiguity that client outputs are JSON, and authenticator outputs are CBOR.

Originally, the client outputs were represented as additions to the clientData, which meant that the origin context was not able to alter them on the way out. I'm still unclear if that will still be the case just by changing getClientExtensionResults form a callable to a field? I believe not.

[equalsJeffH]

@selfissued noted:

clientExtensions and authenticatorExtensions are included in CollectedClientData so they contribute to the clientDataHash input. This cryptographically binds these input values to the request

@emlun replied (and @agl has been asking):

Who is the binding for?

Originally, in a hand-wavy fashion, the binding is (ostensibly) for the RP to be able to verify that the authenticator received what was sent and the returned data (back up through "the stack") is as expected. From the original W3C FIDO Submission Signature Format spec:

FIDO 2.0 signatures are bound to various contextual data. These data are observed, and added at different levels of the stack as a signature request passes from the server to the authenticator. In verifying a signature, the server checks these bindings against expected values

So I'll venture that assuring that protecting against the "origin context ... manipulat[ing] the request" is/was at least an implicit goal.

It is good to have fresh security-conscious eyes on the spec and noticing that we are lacking some clothing (thanks:)

If a compromised origin context is something that we expect to defend against, there are a lot of fields in a creation request that are not echoed and so not protected.

agreed, though might necessity of "echoing" of input options fields be assessed on a field-by-field basis?

the client's outputs can still be manipulated by the origin context

yeah, I'm not sure how one might guard against that.

The instructions say only that the RP should check that the echoed extensions are a subset...

See issue #804 (thx @emlun)

Since the CollectedClientData is JSON serialised, that means that all extensions have to be JSON—i.e. no ArrayBuffers, WebCrypto objects etc. (Unless we also define a Javascript to JSON mapping for these types.)

do you actually mean to say "WebIDL to Javascript" mapping?

Our understanding (from browser spec folk, e.g., @jyasskin) is that this is addressed by implicit conversion.

@selfissued wrote:

Answering @agl 's question about getClientExtensionResults, I'd be fine replacing: AuthenticationExtensionsClientOutputs getClientExtensionResults(); with [SameObject] readonly attribute AuthenticationExtensionsClientOutputs clientExtensionResults; in PublicKeyCredential...

That seems nominally fine by me.

@agl had originally noted (amongst a bunch of items):

On the way back out, step 18.3 of these instructions says that [[clientExtensionsResults]] is an “ArrayBuffer … containing the bytes of assertionCreationData.clientExtensionResults”. But the latter is a AuthenticationExtensionsClientOutputs so how is that turned into a bytestring?

If we change to the clientExtensionResults attribute, then it seems step 18.3 of these instructions would become, nominally:

clientExtensionResults A new AuthenticationExtensionsClientOutputs object associated with |global| whose members are copies of the members of assertionCreationData.clientExtensionResults.

..though I'm unsure of the "members are copies of" portion.

[agl]

So I'll venture that assuring that protecting against the "origin context ... manipulat[ing] the request" is/was at least an implicit goal.

I'm happy with any coherent stance although I will note that not worrying about a compromised origin context makes things simpler. Also, the user fundamentally interacts via the DOM so, if there's attacker Javascript running in the origin, it can wait until the user has authenticated and then simulate whatever actions it wishes on behalf of the authenticated user. Thus protecting against it in webauthn doesn't clearly translate to any obvious practical gain. (Unless someone has something in mind?)

agreed, though might necessity of "echoing" of input options fields be assessed on a field-by-field basis?

Yep, although looking at the list I'm not sure that I can point to any of the fields and say "certainly no need to worry about that one".

do you actually mean to say "WebIDL to Javascript" mapping?

Yes, thanks. I think I probably do.

Our understanding (from browser spec folk, e.g., @jyasskin) is that this is addressed by implicit conversion.

Cool, although I'll have to ask him about how we actually implement that in Chromium if the group goes this route. (While it's just plumbing to do it for extensions that we understand, I believe that browsers have to echo all extension inputs if the purpose is to verify fidelity of transmission.)

[equalsJeffH]

@agl wrote:

I'm happy with any coherent stance although I will note that not worrying about a compromised origin context makes things simpler.

agreed.

Also, the user fundamentally interacts via the DOM so, if there's attacker Javascript running in the origin, it can wait until the user has authenticated and then simulate whatever actions it wishes on behalf of the authenticated user.

Ah, yes, sigh.

Thus protecting against it in webauthn doesn't clearly translate to any obvious practice gain

agreed.

[arnar]

I'm with agl@ that I don't immediately see practical gain in handling compromised origin contexts. But if we were to do so:

[agl@] the client's outputs can still be manipulated by the origin context

[equalsJeffH@] I'm not sure how one might guard against that.

By putting them in CollectedClientData instead of PublicKeyCredential.

[emlun]

The current language in §9. WebAuthn Extensions also says the client extension outputs will be included in the client data:

The client platform performs client extension processing for each extension that it supports, and augments the client data as specified by each extension, by including the extension identifier and client extension output values.

[selfissued]

Fixed by PRs #810 and #811