-
Notifications
You must be signed in to change notification settings - Fork 167
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 consideration about terminating getAssertion early #687
Conversation
The previous language would have the procedure terminate as soon as there are no pending authenticator requests - including immediately at the beginning unless at least one authenticator is available at that time.
index.bs
Outdated
- A named [=public key credential|credential=] is not available. | ||
- A named [=public key credential|credential=] is available, but the user denies [=user consent|consent=] to use it. | ||
|
||
If the above cases are distinguishable, that leaks information by which a malicious [=[RP]=] could identify the user by probing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"that leaks information" -> "information is leaked"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds better, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks fine, thanks. Though:
- please merge-from-master
- perhaps we should land PR fix #322: flesh out Security Considerations (for now) #705 first since it creates a priv cons section, and add the proposed #getAssertion-privacy-considerations as a priv cons subsection?
- normative requirements ought to be in the algorithm itself, rather than buried in priv cons, pls see comment below.
index.bs
Outdated
If the above cases are distinguishable, information is leaked by which a malicious [=[RP]=] could identify the user by probing | ||
for which [=public key credential|credentials=] are available. In particular, the client MUST NOT return a "{{NotAllowedError}}" | ||
before the |lifetimeTimer| has expired, as that difference in timing would constitute such an information leak. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMV, the latter final normative sentence needs to be stated in the algorithm itself, rather than buried down here in Priv Cons.
Reviewed, modest changes requested: #687 (review) |
@equalsJeffH Please re-review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo nits. thx @emlun !
index.bs
Outdated
|
||
In order to protect the user from being identified without [=user consent|consent=], implementations of the | ||
{{PublicKeyCredential/[[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors)}} method need to take care to | ||
not leak information that could enable the [=[RP]=] to distinguish between the cases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/the cases/these cases/
index.bs
Outdated
not leak information that could enable the [=[RP]=] to distinguish between the cases: | ||
|
||
- A named [=public key credential|credential=] is not available. | ||
- A named [=public key credential|credential=] is available, but the user denies [=user consent|consent=] to use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/user denies/user does not/
index.bs
Outdated
|
||
If the above cases are distinguishable, information is leaked by which a malicious [=[RP]=] could identify the user by probing for | ||
which [=public key credential|credentials=] are available. For example, one such information leak is if the client returns a | ||
failure response as soon as the user denies [=user consent|consent=] to proceed with the operation. In this case the [=[RP]=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/user denies/user does not/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks :)
@equalsJeffH Reading this again, I'm inclined to revert this one change #687 (comment) to "the user denies consent", as the containing sentence is meant to express "if the client returns a failure in response to the user's action of refusing a prompt for consent". What do you think? |
sure, fine with me. |
As requested by @equalsJeffH at #736 (comment)
@selfissued please review so we can merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new privacy section seems fine. But it seems like an incorrect and unrelated change to delete the clause "issuedRequests not empty" in two places. Please don't merge this until this this additional change is either documented and agreed to or removed.
@@ -885,8 +885,8 @@ When this method is invoked, the user agent MUST execute the following algorithm | |||
|
|||
1. [=set/Append=] |authenticator| to |issuedRequests|. | |||
|
|||
1. [=While=] |issuedRequests| [=list/is not empty=], perform the following actions depending upon | |||
|lifetimeTimer| and responses from the authenticators: | |||
1. [=While=] |lifetimeTimer| has not expired, perform the following actions depending upon |lifetimeTimer| and responses from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the clause "issuedRequests not empty" deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #687 (comment)
@@ -1233,7 +1235,7 @@ When this method is invoked, the user agent MUST execute the following algorithm | |||
|
|||
1. [=set/Append=] |authenticator| to |issuedRequests|. | |||
|
|||
1. While |issuedRequests| [=list/is not empty=], perform the following actions depending upon |lifetimeTimer| | |||
1. [=While=] |lifetimeTimer| has not expired, perform the following actions depending upon |lifetimeTimer| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the clause "issuedRequests not empty" deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #687 (comment)
@selfissued Thank you for reviewing. The clause "issuedRequests is not empty" was changed to "lifetimeTimer has not expired" because with the previous language the operation would terminate immediately if no authenticator is present at the moment when the timer is started, but makeCredential step 19 and getAssertion step 17 are intended to be run asynchronously as authenticators appear within the timeout. This change is mostly orthogonal to the addition of the privacy considerations section, but it's necessary for the privacy consideration text to make any sense. I also believe we need to make this change for the algorithms themselves to make sense, even without the new privacy consideration. |
Thanks for the explanation, @emlun . Please proceed with the merge. |
@emlun wrote in #687 (comment):
Actually, upon further review, I do not believe this PR addressed issue #204, because the latter is in regards to #createCredential aka So we still have #204 to address. |
I believe with hot-plugging My reasoning is the following. The attack would previously be:
Whereas now, if an excluded credential is available, the browser will simply ignore that authenticator and instead wait for a new authenticator without an excluded credential to appear. If the browser were to violate the spec and return a
I suppose this conclusion could perhaps be combined with some external assumptions (like an upper limit on how many authenticators a user is likely to own) to narrow the field to some extent, possibly enough to identify a single user. But again, this should also not be possible if the browser never returns the Please let me know if you spot any errors in the above reasoning. :) |
Thanks @emlun for your detailed analysis above. I had not reviewed the diff closely enough and had focused on the added Authentication Ceremonies privacy considerations subsection rather than the revised lifetime timer stipulations in both #createCredential aka I agree with the overall conclusion of the detailed analysis above: a malicious RP (aka "the attacker") will not learn anything through seeding of the It seems, IIUC, there are a couple detail-level issues with the steps in the detailed analysis above, but they do not AFAICT affect this conclusion. Here they are for completeness' sake:
AFAICT, the browser/client-platform does not do this (see #createCredential steps 19.5 and 19.6), rather the authnr does this (see authenticatorMakeCredential step 3), and returns
nit: AFAICT the spec does not stipulate whether the browser, client platform, or authenticator prompts the user.
Ah, but the imperative "this step MUST NOT be executed before lifetimeTimer has expired" language this PR inserted into #createCredential step 21 means that the error is not returned before the timer expires, yes? (which makes the above analysis stronger). thanks again. |
Thanks for your detailed response!
Ah yes, that is correct. Either way, the end result is that the client ignores the authenticator in question and keeps waiting for another candidate.
Indeed, although authenticatorMakeCredential step 6 softly specifies that the authenticator does it if capable, and otherwise the client. I chose to keep it simple because I don't know of existing authenticator hardware that allows active denial of consent (as opposed to passive denial by timeout).
Assuming implementers read and implement all the steps, yes. :) |
Related issues: #184, #204 . Loosely related: #140 .
This adds a privacy consideration note to §5.1.4. Use an existing credential to make an assertion about the privacy implications of terminating before the timer expires.
The PR also includes some language changes to the waiting condition in both §5.1.3. Create a new credential and §5.1.4. Use an existing credential to make an assertion which are needed to make the privacy consideration text meaningful, and which were probably already intended.
Preview | Diff