-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Customizing displayName and potential PII leak in user handle #29
Comments
Thanks for the kind words. Regarding the GDPR, I don't know what this has to do with it, so I'll skip that part, but you are welcome to enlighten me. Concerning the Concerning the Citing https://w3c.github.io/webauthn/#sctn-user-handle-privacy:
Consequently, if someone finds or steals an USB security stick, it may expose these user IDs/handles. Since these are hashed usernames, an attacker may deduct the corresponding plain usernames with rainbow tables or plain brute forcing. What is not said, is that this id/handle can also be used to overwrite the credential on the authenticator. This can be useful in a variety of situations, from avoiding creating duplicate credentials for a same user to "resetting" it for whatever reason. Perhaps it could also be used in the future to delete the credential? (...since that's currently impossible 😕 ). Also, this is done during registration, and each user should have a single user handle, so registering multiple devices would need to first poll the server "create or get user id given username". However, the latter appears to be an even greater privacy/security issue to me. However, if user verification is turned off (BTW, it's turned on by default in this lib but turned off by default in the standard), it might even turn out as vulnerability since the attacker also may have found out the username and has the usb key storing the credentials. I consider this a bigger issue than the privacy one. The alternative would be to simply put some random string in the id. This would lead to multiple user handle when registering multiple devices and duplicate credentials in the case of accidental re-registration of a device. Since this against what the specs mandates (a single user id per account) I guess it would lead to other issues. |
I think I'm understanding the reasoning (still new to this area). Do Discoverable Credentials not cover this case where you don't want to pull a consistent opaque user id from the server? My general feedback would be that deviating from a security related spec in any way is risky, especially for a public lib. It might be worth checking in with some people knowledgeable in this field to confirm the approach taken is ok. Otherwise you'd want to at least make it clear to people using this lib that you're not following that one piece of the spec and why. Allows them to make an informed choice. As for GDPR, if there's a chance a company has leaked PII of their customers then they have a duty to disclose that, at least in the EU. I'd say this one is potentially very minor if people are passing in public usernames (that are then hashed). But there would be a chance of devs passing in an email address I'd imagine. You'd want to make it clear not to do that. Anyway this was just a heads up, I'll leave it with you and again thanks for the work on this. |
It's unrelated. "Discoverable" means you can trigger a browser/platform specific UI (if available) to pick one of the existing discoverable credentials.
Agreed. I'm also tempted to put a random value inside. Although the protocol mandates one account / one user id, the id itself is useless in practice. ... Other than to overwrite the credentials. It might be a worthy sacrifice.
Well, this is related to the company's services. If a user looses it's own USB stick, the company cannot be accountable for it.
Well, it's like loosing an USB stick with some stuff on it. I personally see the privacy issue of "with enough malicious effort the username/email can be brute forced" as a joke. The security implications on the other hand are worthy of attention.... In particular if user verification is turned off during authentication. |
After sleeping over it for some time and careful thoughts, I decided to replace the current |
...oh man ...it sucks that it creates new credentials every time instead of overwriting them ...it results in the "discoverable" autofill UI to be overcrowded 🤒 This will apply to all the demos and so on since they don't store credentials server side, but the credentials client side will pile up each time the user tries out any of the demos. ...quite annoying. |
As a side note, for completeness, it reminds me of w3c/webauthn#1763 which I asked in the past to the spec authors. The reasoning of anonymizing the user ID can be summarized as follows:
|
Well, in the end I simply added the a salt ...well, kind of, it's more a public constant. That way, credentials don't pile up upon creation when the same username is used. Since it is not a private salt, it still makes brute forcing the username from the hash theoretically possible, but this raises the difficulty bar of hacking the username of usb sticks you pick up on the street. It's not a perfect, ideal solution but I consider it "good enough". |
I'll be straight, a public salt in this context is not much use given the js src is public on any website that uses this, plus it's OSS. The important detail for people using your lib (again, great lib!) is the original instruction from the spec is still not followed.
As this update still goes against that guidance, the recommendation of at least making this very visible to anyone considering this library would a fair thing to do. Maybe allowing devs to pass a custom RE the demos, can you use local storage for a generated id? In all other real world cases a Relaying Party is needed, that's where opaque id state should really be managed. |
Actually, I agree with all you said. 🙄 ...you are right, I'll reopen it.
Well, not really. People often use "private browsing" to try it, and may also try various browsers, so you'll end up with clean local storages each time, leading to lots of credentials anyway. |
Added |
I tried to use argon2 to make a hash with a secret (pepper) but the user handle was rejected for being more than 64 bytes. |
that's a limitation of the webauthn protocol itself. 64 bytes is already quite long for a user id/handle. You could just take a sha256/512 for instance. You may also find this interesting: https://stackoverflow.com/questions/16891729/best-practices-salting-peppering-passwords |
Thanks for replying, @dagnelies. It's kinda looking like the best solution may be just storing a separate ID in the database, and using that as the handle. |
First off thanks for the work on this library, I can see a lot of thought has gone into it and the tools around it.
I have a question regarding the
username
parameter when registering on the client, specifically this line.https://github.com/passwordless-id/webauthn/blob/main/src/client.ts#L84
A plain client side
sha256
would still be seen as PII as no secret salt was included from the server. I double checked the spec after seeing this and it confirmed it.https://w3c.github.io/webauthn/#sctn-user-handle-privacy
If I'm correct and not misread your code, then I believe you'll need to open up the registration to allow provision of the handle + the PII (displayName, name) separately, and make it clear the handle must not be a raw username or similar.
You may want to let user's of the library know this issue as PII could possibly be leaked (may need GDPR disclosures for those businesses?) so would need clients to re-register customer passkeys? I'm not entirely sure the flows there, still getting to grips with those.
The text was updated successfully, but these errors were encountered: