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

11.2 define the mitigation #142

Closed
samuelweiler opened this issue Oct 7, 2021 · 6 comments
Closed

11.2 define the mitigation #142

samuelweiler opened this issue Oct 7, 2021 · 6 comments
Labels
privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on.

Comments

@samuelweiler
Copy link
Member

The attack in 10.2 is clear, as is the goal ("make sure not to enable malicious callers... to distinguish between these cases").

But the methods of getting there are not defined. Instead of "If the URL is accessed then the caller can conclude that at least one of the passed credentials is available to the user", how about an instruction: "do not load the URL until"? Presumably that instruction should be elsewhere in the doc, to be read contemporaneously with other process test. Are any other mitigations needed to achieve the goal?

@samuelweiler samuelweiler added the privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on. label Oct 7, 2021
@stephenmcgruer
Copy link
Collaborator

The example privacy leak in 10.2 is just that, an example. As with WebAuthn, there are many awkward edge cases that a user agent could accidentally leak credential existence, including in ways that are deliberately not specified (e.g. the UAs choice to show or not show a user under certain circumstances). That is why, like WebAuthn, we are describing the general concern and giving an example to user agents to consider.

The mitigation to the example leak in 10.2 is explicitly spec'd in https://w3c.github.io/secure-payment-confirmation/#sctn-steps-to-check-if-a-payment-can-be-made, fyi.

@stephenmcgruer stephenmcgruer changed the title 10.2 define the mitigation 11.2 define the mitigation Mar 2, 2022
@stephenmcgruer
Copy link
Collaborator

(Note: this is now 11.2)

stephenmcgruer added a commit that referenced this issue Mar 2, 2022
The example was misleading, as it is already explicitly disallowed by the spec
(see 4.1.4, step 8). Removing it hopefully makes it clear that 11.2 is about a
general class of risk that SPC (and WebAuthn) has as a technology, not a
specific attack with a specific mitigation.

See #142
stephenmcgruer added a commit that referenced this issue Mar 2, 2022
The example was misleading, as it is already explicitly disallowed by the spec
(see 4.1.4, step 8). Removing it hopefully makes it clear that 11.2 is about a
general class of risk that SPC (and WebAuthn) has as a technology, not a
specific attack with a specific mitigation.

See #142
stephenmcgruer added a commit that referenced this issue Apr 27, 2022
Before this PR, the spec was vague about the timing attack and also didn't
specify at all that an implementation must return a NotAllowedError instead of
the normal PaymentRequest NotSupportedError in the case of
no-matching-credentials. Whilst we don't want to enforce that UAs show a dialog
here (they may decide, for example, that a delayed response instead is
sufficiently privacy preserving), this PR does try to make the actual concern
clearer and more normative, and add a normative requirement for the return
value.

Should also improve the situation called out in
#142
stephenmcgruer added a commit that referenced this issue Apr 27, 2022
Before this PR, the spec was vague about the timing attack and also didn't
specify at all that an implementation must return a NotAllowedError instead of
the normal PaymentRequest NotSupportedError in the case of
no-matching-credentials. Whilst we don't want to enforce that UAs show a dialog
here (they may decide, for example, that a delayed response instead is
sufficiently privacy preserving), this PR does try to make the actual concern
clearer and more normative, and add a normative requirement for the return
value.

Should also improve the situation called out in
#142
@samuelweiler
Copy link
Member Author

samuelweiler commented May 4, 2022

I'm glad to see the explicit instructions in what is now 4.1.4.

  1. I'd like 11.2 to point at 4.1.4 (e.g. "here's now to achieve this")

  2. Are the instructions in 4.1.4 sufficient to mitigate all leaks? From your text, I'm inferring that is it not. What else is needed?

@stephenmcgruer
Copy link
Collaborator

stephenmcgruer commented May 4, 2022

I'd like 11.2 to point at 4.1.4 (e.g. "here's now to achieve this"

Happy to send a PR for this.

Are the instructions in 4.1.4 sufficient to mitigate all leaks? From your text, I'm inferring that is it not. What else is needed?

This space is quite complex, and to be clear I'm following WebAuthn's lead here. My goal is that implementors know to be careful here, because seemingly innocent decisions can breach the privacy model by accident. I would like to confidently say that we've covered all the ways this can happen, but cannot be sure.

I think ultimately there's a bit of a meta question of what a S&P section should contain. We copied WebAuthn's style, but perhaps that was the wrong choice.

stephenmcgruer added a commit that referenced this issue May 4, 2022
Requested by PING in issue #142
@stephenmcgruer
Copy link
Collaborator

As per the May 4th WPWG remote meeting discussion with PING, the remaining steps for this issue are to have 11.2 point at 4.1.4, and then we should be clear to close it out. PING recommends keeping section 11.2, to serve the purpose of explaining a risk and how the spec mitigates it.

stephenmcgruer added a commit that referenced this issue May 4, 2022
@ianbjacobs
Copy link
Collaborator

Thank you, @samuelweiler, for the recommendation to tighten up the normative language, now integrated. We hope this resolves the current issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on.
Projects
None yet
Development

No branches or pull requests

3 participants