Add more requirements for ClientDataJSON serialisation. #1375
Conversation
ClientDataJSON is currently defined to be the JSON encoding of the CollectedClientData. This implies that validators require a full JSON parsing library to check needed entries in the ClientDataJSON such as the challenge, type, and origin. This is a problematic dependency in some cases. This change seeks to address that by being stricter about the encoding, while still generating JSON. Thus existing validators do not need to change but those willing to require recent WebAuthn-implementing browsers can avoid the full generality of JSON.
There was a problem hiding this comment.
Overall this looks OK—the motivating "let's not have implementers have to have a full JSON parser" is worthwhile—though the manner in which the algs are presented ought to be consistent with how we present algs in other portions of the spec, like:
- explicitly declare variables, such as the "partial result", e.g., as
|partialResult| - use constructs from infra.spec.whatwg.org, e.g., for "append" if it's arguably appropriate.
- use proper code point syntax.
- perhaps not use the
‘and’ - see also @emlun's comments
|
in 2020-02-26 f2f meeting, feedback is that this needs further Note:/comments targeted at implementers that:
|
This change reflects w3c/webauthn#1375 in code. Since that spec change isn't breaking, it's fine to implement this first as the format still conforms to general JSON. This change will allow some validators to test against it, prove out that it works, and then we'll report back results to the W3C group — hopefully resulting in the WebAuthn spec change being accepted. Change-Id: Icedf405bf7eb91b752493badccad892aa9822e9f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083394 Commit-Queue: Adam Langley <agl@chromium.org> Auto-Submit: Adam Langley <agl@chromium.org> Reviewed-by: Martin Kreichgauer <martinkr@google.com> Cr-Commit-Position: refs/heads/master@{#746098}
|
On 2020-03-11 call @agl reports that he has tugged the OpenSSH folks' sleeves and they are thinking about this..... more news later.... |
|
on 2020-03-18 call, @agl reports SSH people are chewing on it, still on hold. |
|
on 2020-04-08 call: @agl reports that ssh folk think this is ok. |
|
This is now good to review; we believe that it all works in practice. |
There was a problem hiding this comment.
Overall LGTM. There's a few very minor things I'll go ahead and change if no one objects, see below (@agl has passed 'assignee' of this PR on to me).
Other reviewers, please review so we can get this landed :)
|
As an outsider to this process (though one with some protocol design experience), this entire proposal/workaround seems deeply dangerous at worst, and an ugly wart at best. There should be no justification for not requiring a standard JSON serializer and parser, that is just asking for unknown and unforeseen trouble in a spec. JSON is used in all manners of security sensitive and even constrained protocols. Asking implementors to do manual serialization of part of a JSON document is almost guaranteed to result in one or more vulnerabilities. I’m disappointed that anyone from SSH is even asking for or expecting this kind of compromise just for their benefit. There are many formal and audited codebases that include JSON parsers, and even some parsers that will tokenize side-effect free without any memory allocations/operations. It took a lot for me to decide to say something here after this was pointed out to me since I’ve not participated in any of these processes and don’t have a full background, so I’ll understand if I’m off track. What I’ve read here though was shocking enough that I felt compelled to speak just in case I’m not the only observer who has these concerns and hope that the whole spec isn’t cobbled by a single particularly stubborn use-case. |
|
Do we also need to make comments around verification of extensions and attestations, given that an implementation without JSON is somewhat likely to also not have an implementation of CBOR? |
Apply Jeff's suggestions Co-Authored-By: =JeffH <jdhodges@google.com>
The differences stem from different sensitivities of different bits of code. Registering a new authenticator (during which attestation processing may be done) is usually less sensitive than pre-auth verification of a signature. Extension processing could be a problem, although the CBOR used is already significantly subsetted by CTAP2 and currently no authentication extensions are commonly used.
I roughly agree that JSON is not an unreasonably complex format, even in sensitive locations. I wrote a, I think, demonstrably memory-safe JSON parser as an example of how it could be done. However, it's not my decision and I must admit that, compared to SSH's length-prefixed bytestrings, JSON is a lot more heavyweight and would significantly increase the amount of code in the highly-sensitive preauth flow. (For those looking for a safe JSON parser in C, I suggest looking at the Wuffs framework, on which the Chromium's future JSON parser may well be based.)
I disagree here. While things could get hairy in general, the input strings here are either static values from the browser, base64url-encoded values, or punycoded origin strings. Of these, only the last is not trivially safe for plain template insertion. This is not a general data structure that's being encoded, it has a few, known keys and values. Lastly, there are few browser implementations and they have to do much more complex and subtle things to ensure safety in WebAuthn than this. It's worth emphasing that no site needs to worry about this, and are recommended not to worry about this. No site need touch anything other than a standard JSON parser. |
There was a problem hiding this comment.
What're your thoughts regarding the comment at
Line 2828 in 8d0060a
Think that should also link to the serialization rules, as now the tolerance is only to the reordering of the non-static elements?
Otherwise, I've worked through the algorithms several times by hand and assuming byte-matching for punycode domains, it works fine for me.
ClientDataJSON is currently defined to be the JSON encoding of the
CollectedClientData. This implies that validators require a full JSON
parsing library to check needed entries in the ClientDataJSON such as
the challenge, type, and origin.
This is a problematic dependency in some cases. This change seeks to
address that by being stricter about the encoding, while still
generating JSON. Thus existing validators do not need to change but
those Relying Parties' webapps that are willing to require the use recent
WebAuthn-implementing browsers can avoid the full generality of JSON.
Preview | Diff