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

Document prevention of attacks on privacy #899

Merged
merged 9 commits into from
Jun 21, 2018
Merged

Conversation

emlun
Copy link
Member

@emlun emlun commented May 9, 2018

Fixes #743. Fixes #382.

#140 is related, but requires a normative requirement that authenticators make credential IDs look like opaque random data.


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 May 9, 2018
@emlun emlun added this to the PR milestone May 9, 2018
@emlun emlun self-assigned this May 9, 2018
@emlun
Copy link
Member Author

emlun commented May 9, 2018

I'm starting to feel like many of these more "explainer" type sections might belong better in a separate appendix. The point of them is to make the spec more accessible and easier to understand - but the spec is already gigantic, and having them inline in the spec might just make it even more intimidating. What do others think?

@selfissued
Copy link
Contributor

I understand your point about "explainer" sections, but moving them out of context to a different part of the spec makes it much less likely that implementers will read them and won't make the spec any smaller. Therefore, I'd keep them in-line.

@samuelweiler samuelweiler self-requested a review May 9, 2018 17:14
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Please apply the suggested corrections and clarifications.

index.bs Outdated
[INFORMATIVE]

Many aspects of the design of the [=Web Authentication API=] are motivated by privacy concerns. The main concern considered in
this specification is the protection of the user's personal identity, i.e., the identification of a physical person or a
Copy link
Contributor

Choose a reason for hiding this comment

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

physical person -> human being (replace all occurrences)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
Many aspects of the design of the [=Web Authentication API=] are motivated by privacy concerns. The main concern considered in
this specification is the protection of the user's personal identity, i.e., the identification of a physical person or a
correlation of separate identities as belonging to the same physical person. Although the [=Web Authentication API=] does not use
or provide and form of global identity, the following kinds of potentially correlatable identities are used:
Copy link
Contributor

Choose a reason for hiding this comment

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

and -> any

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated
private key=], and thus ownership of the [=public key credential=] identified by the [=credential ID=]. They are also visible
to the [=client=] in the communication with the [=authenticator=].

- The user's identities specific to each [=[RP]=], e.g. usernames, [=user handles=] and similar aliases.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what "similar aliases" refers to. Maybe just say "e.g., usernames and [=user handles=]. (Also note the comma after "e.g.".

Copy link
Member Author

Choose a reason for hiding this comment

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

Both done.

index.bs Outdated
- The user's biometric information, e.g., fingerprints or facial recognition data.

This is optionally used by the [=authenticator=] to perform [=user verification=]. It is not revealed to the [=[RP]=], but in
the case of [=platform authenticators=] it might be visible to the [=client=] depending on the implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the missing comma after "[=platform authenticators=].

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated
This is optionally used by the [=authenticator=] to perform [=user verification=]. It is not revealed to the [=[RP]=], but in
the case of [=platform authenticators=] it might be visible to the [=client=] depending on the implementation.

- The models of the user's [=authenticators=], e.g. product names.
Copy link
Contributor

Choose a reason for hiding this comment

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

A comma is always required after "e.g.". Please add it to all uses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

index.bs Outdated

- The [=client=] also ensures that the existence of a [=public key credential=] is not revealed to the [=[RP]=] without [=user
consent=]. This is detailed further in [[#sec-make-credential-privacy]] and [[#sec-assertion-privacy]]. A malicious [=[RP]=]
thus cannot silently identify a user even if the user has a [=public key credential=] registered and available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comma between "user" and "even".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
thus cannot silently identify a user even if the user has a [=public key credential=] registered and available.

- [=Authenticators=] ensure that the [=credential IDs=] and [=credential public keys=] of different [=public key credentials=] are
not correlatable as having the same [=managing authenticator=]. A pair of malicious [=[RPS]=] thus cannot correlate users
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrase "are not correlatable as having the same [=managing authenticator=]" is awkward and hard to understand. Do you mean "are not correlatable as belonging to the same [=authenticator=]"? Or perhaps "are not correlatable as belonging to the same user"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done; I went with "correlatable as belonging to the same user".

index.bs Outdated
or a small group of [=authenticators=]. This is detailed further in [[#sec-attestation-privacy]]. A pair of malicious
[=[RPS]=] thus cannot correlate users between their systems by tracking individual [=authenticators=].

Additionally, a [=public key credential=] with a [=client-side-resident credential private key=] can optionally include a [=user
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any target in the spec that "[=client-side-resident credential private key=]" refers to. Please review the uses of "[= ... =]" in this PR and ensure that all of them refer to locations with matching locations.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reference did resolve successfully, but other links to it capitalize each of the (non-hyphenated) words, so I changed to that here too.

@emlun
Copy link
Member Author

emlun commented Jun 4, 2018

@selfissued Please re-review!

@nadalin
Copy link
Contributor

nadalin commented Jun 6, 2018

@equalsJeffH Please review

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 is looking overall good, tho have some detail-level comments/suggestions to discuss, thx!

[INFORMATIVE]

Many aspects of the design of the [=Web Authentication API=] are motivated by privacy concerns. The main concern considered in
this specification is the protection of the user's personal identity, i.e., the identification of a human being or a correlation
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i suggest the term "natural person" rather than "human being"
https://en.wikipedia.org/wiki/Natural_person

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall I also add that as a non-normative definition reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done; I'm not sure it belongs in the bibliography, though, so I didn't add it to there.

Copy link
Member

Choose a reason for hiding this comment

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

  1. I'm nervous about using legal terms unless a lawyer has reviewed them.
  2. "Historically, a human being was not necessarily a natural person in some jurisdictions where slavery existed (subject of a property right) rather than a person." makes me think we'd prefer "human being" here anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid points. @equalsJeffH What do you think, do we revert this?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh geeze.... I'd still use natural person but just not link it to wikipedia, or sure, use human being....

Copy link
Contributor

Choose a reason for hiding this comment

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

after grovelling in the spec on another PR, i note that we use "human" in other terms/phrases, such as human-palatable, so I'm thinking "human being" is fine. silly me.

index.bs Outdated
Many aspects of the design of the [=Web Authentication API=] are motivated by privacy concerns. The main concern considered in
this specification is the protection of the user's personal identity, i.e., the identification of a human being or a correlation
of separate identities as belonging to the same human being. Although the [=Web Authentication API=] does not use or provide any
form of global identity, the following kinds of potentially correlatable identities are used:
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, I'd tend to term these things below as "identifiers" rather than "identities" -- i.e., an identity is a collection of attributes where identifiers are among the attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, good point.

index.bs Outdated
- The user's [=credential IDs=] and [=credential public keys=].

These are registered by the [=[RP]=] and subsequently used by the user to prove possession of the corresponding [=credential
private key=], and thus ownership of the [=public key credential=] identified by the [=credential ID=]. They are also visible
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
s/ ownership of / correlation with /

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not clear to me how that would improve this sentence - care to explain what you mean?

Copy link
Contributor

@equalsJeffH equalsJeffH Jun 13, 2018

Choose a reason for hiding this comment

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

well... "ownership" has connotations of physical possession. however, the public key is handed to the RP who can do with it what they will. It correlates to the cred source and the cred private key therein. such correlation is proved by verifying signatures produced by the cred priv key, using the cred pub key held by the verifier.

so to me, we're using the notion of "ownership" inappropriately here. (and in computer/internet world in general we often misuse the notion of "own")

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I think the word "correlation" makes it feel like this has something to do with the non-correlatability we talk about elsewhere, which might be confusing.

Would it perhaps be better to just remove the whole ", and thus ownership [...]" subordinate clause?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, yeah wrt "correlation".

Would it perhaps be better to just remove the whole ", and thus ownership [...]" subordinate clause?

works for me, thx.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will do.

index.bs Outdated
These identities are obviously used by each [=[RP]=] to identify a user in their system. They are also visible to the
[=client=] in the communication with the [=authenticator=].

- The user's biometric information, e.g., fingerprints or facial recognition data.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ biometric information / biometric characteristic(s) /
cf. [ISOBiometricVocabulary] http://standards.iso.org/ittf/PubliclyAvailableStandards/c055194_ISOIEC_2382-37_2012.zip

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
[INFORMATIVE]

Although [=Credential IDs=] and [=credential public keys=] are necessarily shared with the [=[RP]=] to enable strong
authentication, the [=public key credentials=] employed in this specification are designed to be minimally identifying and never
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
s/ the [=public key credentials=] employed in this specification / they /
s/ never / not /

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
and not users directly.

- Each [=public key credential=] is strictly bound to a specific [=[RP]=], and the [=client=] ensures that its existence is never
revealed to other [=[RPS]=]. A malicious [=[RP]=] thus cannot ask the [=client=] to reveal other identities owned by the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
s/ to reveal other identities owned by the user / to reveal a user's other identities /

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated

[=Biometric authenticators=] perform the [=biometric recognition=] internally in the [=authenticator=] - though for [=platform
authenticators=] the biometric data might also be visible to the [=client=], depending on the implementation. Biometric data is
not revealed to the [=[RP]=]; it is only used as an additional layer of security for unlocking use of the [=authenticator=]. A
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, for "it is only used as an additional layer of security for unlocking use of the [=authenticator=]", suggest:

it is used only locally to obtain [=user consent=] to create and [=registration|register=], or [=authentication|authenticate=] using, a [=public key credential=].

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I think that formulation might be inaccurate, though - biometrics are not only used to obtain consent, but also as an additional authentication factor. That may not be relevant in this section, but I wouldn't like to suggest that they're used only as a way to obtain user consent (which could be interpreted to just mean "user presence").

But I like the initial rewording "it is used only localy", at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, yeah, that formulation is dancing around the "first multi-factor" aka "first factor" (of a multi-factor authn) notions (which themselves are something else to clean up and clarify in the spec, cf. issue #462). our dfn for user consent does link off to further terms and leads to user verification.

So here's a reformulation using user verification instead:

... it is used only locally to perform [=user verification=] authorizing the creation and [=registration=] of, or [=authentication=] using, a [=public key credential=].

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that looks good to me.

index.bs Outdated
authenticators=] the biometric data might also be visible to the [=client=], depending on the implementation. Biometric data is
not revealed to the [=[RP]=]; it is only used as an additional layer of security for unlocking use of the [=authenticator=]. A
malicious [=[RP]=] therefore cannot discover the user's personal identity via biometric data, and a security breach at a [=[RP]=]
cannot expose biometric data for an attacker to use for forging logins at a different [=[RP]=].
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
/ at a different [=[RP]=] / at other [=[RPS]=] /

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
malicious [=[RP]=] therefore cannot discover the user's personal identity via biometric data, and a security breach at a [=[RP]=]
cannot expose biometric data for an attacker to use for forging logins at a different [=[RP]=].

In the case where a [=[RP]=] requires [=biometric recognition=], this is instead achieved by the [=biometric authenticator=]
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, i'm not sure why you're using the word "instead" here? IIUC, the [=UV=] [=flag=] is set by an authnr whenever it successfully performs user verification?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, good point. What I meant was that the authenticator sets the (verifiably authentic) UV flag instead of sending the biometric data to the server, but you're right that the right hand side of that "instead" isn't clear enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reformulated this a bit, does that look better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also considering perhaps mentioning the uvi and uvm extensions here, what do you think about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I hesitate to mention those extensions here, mostly due to being unsure of whether they will be implemented (?).

index.bs Outdated
cannot expose biometric data for an attacker to use for forging logins at a different [=[RP]=].

In the case where a [=[RP]=] requires [=biometric recognition=], this is instead achieved by the [=biometric authenticator=]
setting the [=UV=] [=flag=] in the signed [=assertion=] response. The [=[RP]=] can trust the authenticity of this bit as long as
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, however, if an RP is not checking attstn, then it is "trusting" the entire contraption here, and certainly "can" do that. Am not sure how to rephrase this. Perhaps it does not really need to be said?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's worthwhile to point out how this is dfferent from the usual kind of (completely worthless) client-side authentication. And if the RP is not checking attestation, then the RP cannot trust that the UV flag is authentic, whether or not the RP chooses to act upon its value anyway. If the RP actually requires UV, then the RP MUST verify the attestation statement - otherwise that requirement is nothing more than wishful thinking.

Copy link
Contributor

Choose a reason for hiding this comment

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

see thread below #899 (review) for resolution

@nadalin
Copy link
Contributor

nadalin commented Jun 13, 2018

@emlun Please merge

@emlun
Copy link
Member Author

emlun commented Jun 13, 2018

There are still a few more unresolved discussion points in #899 (review).

index.bs Outdated
user's personal identity via biometric data, and a security breach at a [=[RP]=] cannot expose biometric data for an attacker to
use for forging logins at other [=[RPS]=].

In the case where a [=[RP]=] requires [=biometric recognition=], this is achieved by the [=biometric authenticator=] setting the
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps rephrase as:

... , this is performed locally by the [=biometric authenticator=] performing [=user verification=] and then signaling the result by setting the [=UV=] [=flag=] in the signed [=assertion=] response, instead of revealing the biometric data itself to the [=[RP]=].

I'm not sure about this part because there's going to be lots of RPs who "trust" webauthn assertions even though they do not verify attestations:

The [=[RP]=]
+can trust the authenticity of the [=UV=] [=flag=] as long as it trusts the security guarantees of the [=attestation certificate=]
+presented when the [=biometric authenticator=] was [=registration|registered=].

Copy link
Member Author

@emlun emlun Jun 13, 2018

Choose a reason for hiding this comment

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

Thanks, that formulation looks very good!

I'm not sure about this part because there's going to be lots of RPs who "trust" webauthn assertions even though they do not verify attestations:

That is true, and the assertion as a whole can indeed be reasonably trusted without verifying the attestation statement (since a MitM attack on it, while theoretically possible, would be extremely difficult to maintain). The RP can't trust that the authenticator provides any given level of protection for the private key, but it can trust that something on the client side has possession of a particular private key. However, this part is specifically concerned with the authenticity of the UV bit, which can only be trusted if there's a trusted attestation signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed on most of the above, but...

However, this part is specifically concerned with the authenticity of the UV bit, which can only be trusted if there's a trusted attestation signature.

well, i disagree with that -- trust is in the eye of the RP. If an RP leaves AttestationConveyancePreference defaulting to none, it's still totally their decision whether to "trust" the UV bit flag, and "how much" to trust it. Amongst RPs performing "risk based authn", "trust" is a continuum. Amongst less sophisticated long-tail RPs, even with ignoring attestation, webauthn/fido-based authn is WAY better than shared secret username/password.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, true. The point I'm trying to make is that this statement is in the context of "RP requires biometric recognition", which I would expect means a hard requirement that absolutely must be satisfied, and I would say risk-based authn falls outside that.

But ok, perhaps the word "trust" is the issue here. Maybe something like one of these?

  • The RP can verify that the UV flag is authentic if the RP trusts the biometric authenticator's attestation certificate, presented when the credential was registered.
  • The UV flag is guaranteed to be accurate if the RP trusts the biometric authenticator's attestation certificate, presented when the credential was registered.
  • The biometric authenticator's attestation certificate, presented when the credential was registered, can attest to the accuracy of the UV flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, it seems to me that "guaranteeing" that the value of the UV is "accurate" is multi-faceted, beyond simply the presence of verifiable attestation. Even if one has verified the attestation, retrieved the corresponding authnr metadata, verified metadata sigs, verified that the authnr metadata asserts that the authnr performs user verification, it is possible for the authnr to have non-negligible (for some definition of negligible) False Acceptance Rate (i.e., is a lousy UV impl). What the ecosystem has to (ostensibly) protect against the latter scenario are the FIDO authnr and biometric certification programs, so add "verifying that the authnr is certified (perhaps to some particular "level")" to the list of things for the RP to verify, but that still is not a 100% guarantee. Note also not all authnrs will be certified.

The point I'm trying to make is that this statement is in the context of "RP requires biometric recognition", which I would expect means a hard requirement that absolutely must be satisfied...

well, in practical terms there is some wiggle room even if one has "a hard requirement that absolutely must be satisfied" because no biometric recognition system will be perfect. See: https://drafts.fidoalliance.org/biometrics/requirements/latest/Biometrics-Requirements-v1.0-wd-20180524.html#FARReq

I suspect that for simplistic low-value RPs (and possibly large RPs where user is involved in low-value interactions at that time), simply checking the assertion sig is valid and relying on UV flag value is Good Enough, whether or not the RP paid any attention to attestation.

For high-value RPs (or more generally, RP where user is involved in a high-value interaction), they can perform all the above checks and assign a higher score to the value of the UV flag in their overall assessment of the state of the particular interaction.

I'm not sure where we ought to document such detailed deployment considerations. And because "trust" is complex & subtle & RP-specific, I'm hesitant to make simplistic statements about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh boy, this turned out to be a gnarly one, didn't it... :) Thanks, you are indeed right on all points. I feel like what you suggested in #899 (comment) might be the way to go in the end, and dodge the issue instead:

Perhaps it does not really need to be said?

I would have liked to clearly point out how this is different from usual client side authentication, but that doesn't seem important enough to hold this up over if you still agree that deleting the last sentence is a reasonable way to go. I also notice that the first sentence does mention that the flag is signed over, so there's at least a hint of it still there.

Copy link
Contributor

Choose a reason for hiding this comment

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

you still agree that deleting the last sentence is a reasonable way to go

yep, fine by me, thanks.

the flag is signed over, so there's at least a hint of it still there.

yep.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

- The identities of the user's [=authenticators=], e.g., serial numbers.

This is possibly used by the [=client=] to enable communication with the [=authenticator=], but is not exposed to the
[=[RP]=].
Copy link
Contributor

Choose a reason for hiding this comment

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

wrt "The identities of the user's [=authenticators=]..." -- this confused me because it is separate from the immediately above models of authnrs revealed thru attestation, but this one does not mention attestation -- so is this referring to info that is revealed at he OS platform and transport layers which the [=client=] may see? if so, perhaps clarify?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is just a brain fart on my part - the WebAuthn spec says absolutely nothing about this, so I agree it's probably confusing to even bring it up. The OS may be able to see serial numbers and such on the device level (YubiKeys provide APIs for querying for it, for example), but it should never be exposed to the RP.

So I'm thinking I'll just delete this bullet. That sound good?

index.bs Outdated
malicious [=[RP]=] therefore cannot discover the user's personal identity via biometric data, and a security breach at a [=[RP]=]
cannot expose biometric data for an attacker to use for forging logins at a different [=[RP]=].

In the case where a [=[RP]=] requires [=biometric recognition=], this is instead achieved by the [=biometric authenticator=]
Copy link
Contributor

Choose a reason for hiding this comment

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

I hesitate to mention those extensions here, mostly due to being unsure of whether they will be implemented (?).

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.

i think this has come out really well @emlun -- just one new comment above. I also replied in a couple of our threads from yesterday on the prior state of this.

index.bs Outdated

In the case where a [=[RP]=] requires [=biometric recognition=], this is performed locally by the [=biometric authenticator=]
perfoming [=user verification=] and then signaling the result by setting the [=UV=] [=flag=] in the signed [=assertion=] response,
instead of revealing the biometric data itself to the [=[RP]=]. The [=[RP]=] can trust the authenticity of the [=UV=] [=flag=] as
Copy link
Contributor

Choose a reason for hiding this comment

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

so we'll delete this final sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, done.

index.bs Outdated
[INFORMATIVE]

Many aspects of the design of the [=Web Authentication API=] are motivated by privacy concerns. The main concern considered in
this specification is the protection of the user's personal identity, i.e., the identification of a [=natural person=] or a
Copy link
Contributor

Choose a reason for hiding this comment

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

see #899 (comment) -- human being is fine, silly me. sorry/thx.

Copy link
Member

@samuelweiler samuelweiler left a comment

Choose a reason for hiding this comment

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

I think this would resolve issue 382. I'm less sure that it resolves 743 - do we need "things implementers might need to do"? More generally: wow, this is verbose. But I don't really have a problem with it. I'm going to mark it as approved, since it doesn't seem to do any harm

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.

LGTM, let's merge it, thx @emlun!

@emlun emlun merged commit 2b52465 into master Jun 21, 2018
@emlun emlun deleted the issue-743-de-anon-priv-cons branch June 21, 2018 09:59
WebAuthnBot pushed a commit that referenced this pull request Jun 21, 2018
WebAuthnBot pushed a commit that referenced this pull request Jun 21, 2018
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
Development

Successfully merging this pull request may close these issues.

6 participants