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

Security Considerations for Unsigned Credential ID #766

Merged
merged 6 commits into from
Jan 26, 2018

Conversation

selfissued
Copy link
Contributor

@selfissued selfissued commented Jan 26, 2018

Fixes #394


Preview | Diff

@selfissued selfissued added this to the CR milestone Jan 26, 2018
@selfissued selfissued self-assigned this Jan 26, 2018
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, but see comments.

index.bs Outdated

The credential ID is not signed.
This is not a problem because all that would happen if an authticator returns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: authticator

index.bs Outdated

The credential ID is not signed.
This is not a problem because all that would happen if an authticator returns
the wrong crednetial ID is that the RP will not look up the correct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: crednetial
Suggest: RP -> [=[RP] =]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more interesting case to point out is if a MitM were to manipulate the credential ID (the conclusion is the same), so I would probably write this as something like "[...] if an authenticator returns the wrong credential ID, or if an attacker intercepts and manipulates the credential ID, is that the RP [...]"

@emlun
Copy link
Member

emlun commented Jan 26, 2018

Also, perhaps link some of the terms throughout? I can make a PR of that into this PR if you can't run bikeshed.

@selfissued
Copy link
Contributor Author

@emlun if you can create a PR into my PR addressing the issues you raise that would great. Then I could apply it in my morning and merge the combination into master.

@selfissued selfissued merged commit e5c8c4f into w3c:master Jan 26, 2018
WebAuthnBot pushed a commit that referenced this pull request Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants