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

Add privacy considerations about credential IDs #1250

Merged
merged 9 commits into from Oct 9, 2019

Conversation

emlun
Copy link
Member

@emlun emlun commented Jul 1, 2019

Fixes #1246. Fixes #1311.

Things to consider:

  • Does this belong in the spec or in separate explainer documentation?
  • Is this unnecessarily verbose?
  • This is all conjecture as I'm not aware of any quantification of the severity of this kind of information leak, nor of how effective the suggested mitigation would be. Should we refrain from putting that in?

Preview | Diff

@emlun emlun added type:editorial privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. labels Jul 1, 2019
@emlun emlun requested a review from equalsJeffH July 1, 2019 14:53
@emlun emlun self-assigned this Jul 1, 2019
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@equalsJeffH equalsJeffH requested review from agl and akshayku July 3, 2019 18:26
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

overall LGTM, tho there's subtleties wrt "sign-in" as compared to "re-authn" we probably ought to distinguish if we're going to go into this much use case detail....

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@equalsJeffH equalsJeffH added this to the L2-WD-02 milestone Jul 10, 2019
Copy link

@manger manger left a comment

Choose a reason for hiding this comment

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

Alternative suggested text for the whole "Privacy leak via credential IDs" section:

Identifying the specific authenticators that are acceptable at the start of an authentication ceremony (in {{PublicKeyCredentialRequestOptions/allowCredentials}}) is necessary if any authenticator uses a non-resident credential, and is useful even in other cases to specify a preference order or transport hints. In these situations the credential ids are revealed before the user is authenticated (or at least before the user is fully authenticated). A credential id is effectively a random value specific to one authenticator and one account at one RP so revealing an id does not allow correlations across accounts or RPs. A given type of authenticator, though, is likely to create credential ids of the same length at each RP. And a given individual is likely to register the same authenticators at each RP. Consequently, for an individual with an unusually large number of authenticators that use unusual credential id lengths, that set of lengths is a slight privacy leak that could correlate different accounts.

One way to mitigate even that slight privacy leak is to include additional imaginary, but plausible, credential ids (see discussion in [[#sctn-username-enumeration]]) so no set of credential id lengths stands out.

@emlun
Copy link
Member Author

emlun commented Aug 8, 2019

@manger Thanks for the suggestion, I'll see if I can incorporate some of that.

Copy link
Contributor

@agl agl left a comment

Choose a reason for hiding this comment

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

This looks fine to me, although I'd probably also note that disclosing credential IDs allows someone with only momentary access to an authenticator to confirm guesses about the owner's identity. Or, if a database can be built, query many possibilities to establish their identity.

(E.g. try ssh whoami.filippo.io for something somewhat similar.)

@emlun
Copy link
Member Author

emlun commented Aug 27, 2019

@agl Thanks, I added a mention of this.

@manger Going through your proposal again it seems like the main points are already addressed by the text as currently written, so I ended up not making any changes to it.

With that, unless anyone wants to re-review the addition of @agl's comment, I think we're good to merge as soon as @equalsJeffH finishes his review.

@maxhata
Copy link

maxhata commented Sep 5, 2019

In this case the {{PublicKeyCredentialRequestOptions/allowCredentials}} argument risks leaking [PII],
if the user can initiate an [=authentication ceremony=] by only providing a username.

"by only providing a username" may sound like it excludes the case where
a username is derived from "ambient credentials" such as cookies.
To eliminate this concern, how about removing "providing" or something else?

@emlun
Copy link
Member Author

emlun commented Sep 5, 2019

"by only providing a username" may sound like it excludes the case where
a username is derived from "ambient credentials" such as cookies.

It does exclude that case - unless the "ambient credential" is just an unauthenticated username with no session key, which wouldn't really make much sense.

@maxhata
Copy link

maxhata commented Sep 5, 2019

There are many other ways to use "ambien credentials". For example, you could store a hash of credentialIDs instead of username. All these cases should be part of this privacy concern since no matter what you do, you end up sending credentialIDs for a get().

@emlun
Copy link
Member Author

emlun commented Sep 5, 2019

@maxhata I don't think I quite understand what you mean, but I changed the quoted sentence to

In this case the allowCredentials argument risks leaking personally identifying information, since it exposes the user’s credential IDs to an unauthenticated caller.

This is both more general by not emphasising a particular interaction flow, and more precise in what the problem is, and I think it should also address your concern, right?

@maxhata
Copy link

maxhata commented Sep 5, 2019

This is how it started the discussion.

Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

this LGTM, thx @emlun !

@emlun
Copy link
Member Author

emlun commented Sep 11, 2019

Hm, looks I forgot that I branched PR #1270 off from this, and since that is now merged the diff here looks a bit strange. Sorry about the mess...

@equalsJeffH
Copy link
Contributor

are we waiting on merging this for @manger to approve?

@emlun
Copy link
Member Author

emlun commented Sep 11, 2019

There's also a review requested from @akshayku.

@akshayku
Copy link
Contributor

I need more time to see this. I started looking what we have specified regarding Credential ID privacy and there are more points to review related to username enumeration.

I will finish this by TPAC.

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
@akshayku akshayku merged commit c7e44f6 into master Oct 9, 2019
@emlun emlun deleted the issue-1246-credentialid-privacy-leak branch October 9, 2019 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. type:editorial
Projects
None yet
6 participants