Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TokenBindingId value doesn't permit the U2F "unused" value #798

Closed
kpaulh opened this issue Feb 12, 2018 · 5 comments
Closed

TokenBindingId value doesn't permit the U2F "unused" value #798

kpaulh opened this issue Feb 12, 2018 · 5 comments
Assignees

Comments

@kpaulh
Copy link
Contributor

kpaulh commented Feb 12, 2018

Currently the WebAuthn spec defines the TB ID as just a base64url value. This does not allow for a sentinel value that indicates that the "client supports token binding, but did not use it because server claimed no support". This value was called "unused" in U2F.

Working on a PR to rectify this.

I'm sorry this went unnoticed until now. I don't think anyone else supports TokenBinding yet (?) and I think it will be a small change.

@kpaulh kpaulh changed the title TokenBindingId value doesn't permit the U2F "unsued" TokenBindingId value doesn't permit the U2F "unsued" value Feb 12, 2018
@kpaulh kpaulh changed the title TokenBindingId value doesn't permit the U2F "unsued" value TokenBindingId value doesn't permit the U2F "unused" value Feb 12, 2018
@kpaulh
Copy link
Contributor Author

kpaulh commented Feb 12, 2018

To expound some more:

The presence of the TokenBindingId indicates that the client and server negotiated and is using Token Binding, and the RP can use the ID to detect a MITM.

The absence of the TokenBindingId, however, doesn't permit the RP to distinguish between these two cases:

  • The client does not support token binding [reasonable situation
  • The client does support TB, but is masked by a MiTM that tells the server they don't [bad situation]

To distinguish between the three total states, we're proposing something along the lines of replacing
"DOMString TokenBindingID" in the CollectedClientData with

dictionary TokenBinding {
TokenBindingStatus status;
DOMString tokenBindingId;
};

enum TokenBIndingStatus { "present", "supported", "not supported" };

Open to suggestions, of course!

@emlun
Copy link
Member

emlun commented Feb 13, 2018

Seems reasonable to me. I agree with making it a separate property instead of a magic value.

If we rename the CollectedClientData property from tokenBindingId to tokenBinding, we could call the TokenBinding property just id instead of tokenBindingId.

status: "present" seems a bit redundant, since that would be implied if the tokenBindingId property exists. So perhaps it could even be reduced to this?

dictionary TokenBinding {
  optional boolean supported;
  optional DOMString id;
};

Then we would have the cases

  • { "tokenBinding": { "id": "abcd" } }: supported and used;
  • { "tokenBinding": { "supported": true } }: supported but not used;
  • {}, { "tokenBinding": {} }, { "tokenBinding": { "supported": false } }: not supported.

But perhaps this makes the logic too convoluted. @kpaulh's proposal does look good to me as is.

@kpaulh
Copy link
Contributor Author

kpaulh commented Feb 13, 2018

We did consider that reduction of the status, but were thinking that the discrete states would be clearer and reduce misunderstanding. I think tokenBindingId -> id is a good change, though. Thanks!

agl added a commit to agl/webauthn that referenced this issue Feb 14, 2018
The existing string was not able to express the ternary nature of token
binding for a given connection. See referenced bug for discussion.

Fixes w3c#798
@agl
Copy link
Contributor

agl commented Feb 14, 2018

Kim is out at the moment so I did something that I hope is correct in #802.

@samuelweiler samuelweiler added this to the CR milestone Feb 14, 2018
@nadalin nadalin modified the milestones: CR, Last Working Draft Feb 14, 2018
@kpaulh
Copy link
Contributor Author

kpaulh commented Feb 15, 2018

+1 Thanks, Adam!

kpaulh pushed a commit to kpaulh/webauthn that referenced this issue Apr 6, 2018
The existing string was not able to express the ternary nature of token
binding for a given connection. See referenced bug for discussion.

Fixes w3c#798
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants