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

Spec: Pose privacy considerations #761

Merged
merged 7 commits into from
Jul 22, 2019
Merged

Spec: Pose privacy considerations #761

merged 7 commits into from
Jul 22, 2019

Conversation

toji
Copy link
Member

@toji toji commented Jul 9, 2019

Describes the mitigations and checks that must be used when returning
pose data from the API. Also includes discussion of data adjustments
and the privacy considerations for returning immersive sessions. Pulls
heavily from privacy-security-explainer.md.

@toji toji requested a review from NellWaliczek July 9, 2019 19:18
@toji toji added this to the June 2019 milestone Jul 10, 2019
@toji
Copy link
Member Author

toji commented Jul 10, 2019

@johnpallett: Can't add you as a reviewer directly, but could you take a look at this?

Copy link
Contributor

@johnpallett johnpallett left a comment

Choose a reason for hiding this comment

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

Thanks @toji left some feedback inline. Speaking of inline, a core question below is whether we need an algorithm for inline sessions that checks sensitive data access requirements, in the case where sensitive data is exposed - I couldn't find such an algorithm (though I might have missed it). Details below.

@toji
Copy link
Member Author

toji commented Jul 12, 2019

Okay, restructured the doc a bit based on John's comments and a quick call with him (thanks!) Hopefully this addressed the concerns raised and puts us in a bit better position for applying definitions like the document's "trustworthiness" in more places.

toji referenced this pull request Jul 12, 2019
Pulls a lot of text directly out of privacy-security-explainer.md to
define some of the various terms necessary to discuss user consent and
the data that it protects.
Copy link
Contributor

@johnpallett johnpallett left a comment

Choose a reason for hiding this comment

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

@toji per #768 I'd suggest we hold off on the 'underlying sensor feature policy' restriction until we can be more precise about how it should be implemented. This is the opposite of what I'd suggested before, but @ddorwin raises a good point about an anti-pattern of developers adding unnecessary feature policies to iframes for sensors that may or may not be necessary. @NellWaliczek curious to get your thoughts on this.

index.bs Outdated
1. If |document| is not of the [=same origin-domain=] as the [=active document=], return <code>false</code>
1. If |document| is not a [=responsible=] document, return <code>false</code>
1. If |document| is not [=active and focused=], return <code>false</code>
1. If |document|'s origin is not allowed to use the WebXR [[#feature-policy|feature policy]] or the [=underlying sensors=]' feature policies, return <code>false</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #768 we may want to hold off on the underlying sensor requirement to avoid the anti-pattern of developers enabling sensors for hardware that doesn't need them. I realize this is the opposite of what I'd proposed before.

I'd suggest:

Suggested change
1. If |document|'s origin is not allowed to use the WebXR [[#feature-policy|feature policy]] or the [=underlying sensors=]' feature policies, return <code>false</code>
1. If |document|'s origin is not allowed to use the WebXR [[#feature-policy|feature policy]] return <code>false</code>

Copy link
Member

Choose a reason for hiding this comment

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

@johnpallett Is there already an issue tracking the need to make a decision on this topic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question; #768 contains the considerations and raises the question. It could be used either to track a decision about whether we should remove the requirement in the short-term, or could be used to find a longer-term solution. @ddorwin @NellWaliczek do you have a preference for how that conversation is structured?

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 going to accept John's suggestion here because it seems like the overall lowest impact way to proceed forward for the following reason:

The intent behind this was to prevent pages from "backdooring" certain bits of accelerometer/gyro sensor data that may reveal things like pin entry on a 2D page. I think it's reasonable to say that this should not be a concern for immersive-vr because the context in which the sensors are being used is significantly different and has a different set of threat vectors (the handling of which is what this entire section is about.) The biggest concerning scenario, as such, is sensor-based tracking in inline mode. Fortunately for us this is not something with guaranteed availability. It may be rejected by the user or the UA, or simply not be supported by the device at all (desktop.) As such, if we were to introduce a similar restriction down the line the actual impact of it would be that some mobile devices start displaying the same inline behavior as a desktop device. That seems like a pretty soft failure, as these things go. Plus, it gives a clear indication that something has stopped working and gives us an opportunity to communicate why in the console at that point. If we went the other way and decided that these additional feature policies were not, in fact, necessary we would have a much more difficult time communicating to developers that they have now exposed certain features unnecessarily, since nothing would indicate that there was a problem to look into and it would be a mighty heuristics challenge to identify scenarios in which the developer ONLY was using those feature policies for WebXR and as such we could safely tell them to remove them.

index.bs Outdated
To determine if an <dfn>inline session request is allowed</dfn> the user agent MUST run the following steps:

1. If the requesting document is not [=responsible=], return <code>false</code>
1. If requesting document's origin is not allowed to use the WebXR [[#feature-policy|feature policy]] or the [=underlying sensors=]' feature policies, return <code>false</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #768 we may want to hold off on the underlying sensor requirement to avoid the anti-pattern of developers enabling sensors for hardware that doesn't need them. I realize this is the opposite of what I'd proposed before.

I'd suggest:

Suggested change
1. If requesting document's origin is not allowed to use the WebXR [[#feature-policy|feature policy]] or the [=underlying sensors=]' feature policies, return <code>false</code>
1. If requesting document's origin is not allowed to use the WebXR [[#feature-policy|feature policy]], return <code>false</code>

index.bs Outdated
@@ -2196,7 +2277,7 @@ The feature identifier for this feature is <code>"xr"</code>.

The [=default allowlist=] for this feature is <code>["self"]</code>.

In addition to the <code>"xr"</code> feature policy, feature policies for underlying sensors must also be respected if a site could isolate and extract sensor data that would otherwise be blocked by those feature policies.
In addition to the <code>"xr"</code> feature policy, feature policies for <dfn>underlying sensors</dfn> must also be respected if a site could isolate and extract sensor data that would otherwise be explicitly blocked by those feature policies' origin allow list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #768 we may want to hold off on the underlying sensor requirement to avoid the anti-pattern of developers enabling sensors for hardware that doesn't need them. I realize this is the opposite of what I'd proposed before.

I'd suggest we remove this line.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together and for your patience in waiting for my review!

1. If |document| is not of a [=secure context=], return <code>false</code>
1. If |document|'s origin is blocked by [[#feature-policy|feature policy]], return <code>false</code>
1. If the [=currently focused area=] does not belong to |document|, return <code>false</code>
1. If |document| is not of the [=same origin-domain=] as the [=active document=], return <code>false</code>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's an unintended implication of this requirement on desktop VR. For example, if you've got a page with two iframes on it of different origins and one of them has an active immersive session (i.e. user activation happened and the iframe had the right feature policy). If you've got someone in the headset viewing it and someone sitting next to them at the computer with the mouse... what happens if the person with the mouse clicks on the iframe with the different origin?

I'm guessing this requirement was originally about inline sessions, so we might just need to tweak the language a bit to accommodate this.

Also this may be related to #696 and #747

Copy link
Contributor

Choose a reason for hiding this comment

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

A primary consideration of this requirement is input sniffing. I could imagine a scenario where a user in an immersive session was presented with an OS-level virtual keyboard or notification input (possibly in response to an event triggered by a different origin).

Copy link
Member

Choose a reason for hiding this comment

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

@johnpallett I'm not sure we're talking about the same thing? In that case, wouldn't the previous bullet be the one that cause the algorithm to return false?

index.bs Outdated

To determine if an <dfn>immersive session request is allowed</dfn> the user agent MUST run the following steps:

1. If the request was not [=triggered by user activation=], return <code>false</code>
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to expand the definition of user activation to include things like launching an installed web app?

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, thank you for the reminder. Done.

@johnpallett, do you want to sanity check this new chunk of text for unintended consequences?

index.bs Outdated
1. If |document| is not of the [=same origin-domain=] as the [=active document=], return <code>false</code>
1. If |document| is not a [=responsible=] document, return <code>false</code>
1. If |document| is not [=active and focused=], return <code>false</code>
1. If |document|'s origin is not allowed to use the WebXR [[#feature-policy|feature policy]] or the [=underlying sensors=]' feature policies, return <code>false</code>
Copy link
Member

Choose a reason for hiding this comment

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

@johnpallett Is there already an issue tracking the need to make a decision on this topic?

toji and others added 5 commits July 19, 2019 11:35
Describes the mitigations and checks that must be used when returning
pose data from the API. Also includes discussion of data adjustments
and the privacy considerations for returning immersive sessions.
Co-Authored-By: johnpallett <johnpallett@google.com>
Co-Authored-By: johnpallett <johnpallett@google.com>
@toji
Copy link
Member Author

toji commented Jul 19, 2019

Updated the PR to address any feedback that isn't pending a separate decision.

@toji
Copy link
Member Author

toji commented Jul 22, 2019

Okay, updated the PR to take into account the underlying sensor discussion and inline feature discussion, both of which I've dived into in greater detail above. I have not explicitly addressed the concern about iframes on desktop devices because I'm still not sure from the conversation if that's a scenario that we aren't concerned about. I do think it's something that we could loosen restrictions on in the future with no ill effects, though.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

LGTM!

1. If |document| is not of a [=secure context=], return <code>false</code>
1. If |document|'s origin is blocked by [[#feature-policy|feature policy]], return <code>false</code>
1. If the [=currently focused area=] does not belong to |document|, return <code>false</code>
1. If |document| is not of the [=same origin-domain=] as the [=active document=], return <code>false</code>
Copy link
Member

Choose a reason for hiding this comment

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

@johnpallett I'm not sure we're talking about the same thing? In that case, wouldn't the previous bullet be the one that cause the algorithm to return false?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants