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

Allow use from within secure context and top-level browsing context only #10

Closed
anssiko opened this issue Mar 16, 2017 · 15 comments · Fixed by #13
Closed

Allow use from within secure context and top-level browsing context only #10

anssiko opened this issue Mar 16, 2017 · 15 comments · Fixed by #13

Comments

@anssiko
Copy link
Member

anssiko commented Mar 16, 2017

(This issue is branched from a proposal made in #5 (comment) since there seemed to be adequate support for making such a spec update.)

Problem:

Malicious content such as framed tracker scripts using the API to fingerprint users.

Proposed solution:

Make the API available only within a secure context that is also a top-level browsing context. This disallows the use of the API within framed content, as well as from any content that is not a secure context.

See top-level documents and framed documents for illustrations.

Summary of changes:

There exists a hook in the spec to implement this change with no API surface changes in a backwards compatible manner:

  • if the API is invoked from within a browsing context that is not a secure context and not a top-level browsing context, then
  • leave the promise returned by getBattery() in a pending state

This means we won't break existing web content using the API.

@riju volunteered to look into updating the Chromium/Blink implementation accordingly after the spec changes have landed.

@riju
Copy link

riju commented Mar 16, 2017

Presently in getBattery(), there is a note that
The user agent must not reject the battery promise.

We can fix this by rejecting the promise with "SecurityError" DOMException, if the incumbent settings object is not a secure context or a top-level browsing context.

@anssiko
Copy link
Member Author

anssiko commented Mar 16, 2017

Rejecting with a SecurityError seems to be a common pattern, so we should consider it.

Since existing web content using the API may not handle rejected promise (because it was not expected), it'd be a backwards incompatible change.

@anssiko
Copy link
Member Author

anssiko commented May 10, 2017

Submitted a spec patch 53360f1 per the proposal outlined above, i.e. reject the battery promise with a SecurityError if not a secure context and top-level browsing context.

I heard no concerns to the proposal, so I'll merge this immediately to get the Editor's Draft updated.

@Honry will update the test suite accordingly to match this spec change.

anssiko added a commit that referenced this issue May 10, 2017
Fix #10: Restrict to secure context and top-level browsing context
Honry added a commit to Honry/web-platform-tests that referenced this issue May 10, 2017
…text

 - Fix w3c/battery#10
 - Append https postfix
 - Add two tests for checking SecurityError with insecure context
   and non top-level browsing context
 - Remove tests for checking different Navigator object in iframe
 - Add missing author and spec link
Honry added a commit to Honry/web-platform-tests that referenced this issue May 10, 2017
…text

 - Tests update for w3c/battery#10
 - Append https postfix
 - Add two tests for checking SecurityError with insecure context
   and non top-level browsing context
 - Remove tests for checking different Navigator object in iframe
 - Add missing author and spec link
Honry added a commit to Honry/web-platform-tests that referenced this issue May 10, 2017
…text

 - Tests update for w3c/battery#10
 - Append https postfix
 - Add two tests for checking SecurityError with insecure context
   and non top-level browsing context
 - Remove tests for checking different Navigator object in iframe
 - Add missing author and spec link
@anssiko
Copy link
Member Author

anssiko commented Jun 1, 2017

This patch has landed in Chromium: https://codereview.chromium.org/2880763002/

scheib pushed a commit to scheib/chromium that referenced this issue Jun 1, 2017
…t only.

Make the Battery Status API available only within a secure context that is
also a top-level browsing context. This disallows the use of the API within
framed content, as well as from any content that is not a secure context.

Details: w3c/battery#10

WPT updated in web-platform-tests/wpt#5871

BUG=661792

Review-Url: https://codereview.chromium.org/2880763002
Cr-Commit-Position: refs/heads/master@{#476263}
@mounirlamouri
Copy link
Member

Firefox's implementation was providing too much entropy, it doesn't mean that other implementations have to do tho same. I'm not sure how restricting the feature to top level browsing context is the right solution. The Battery API can be used by third party content for valid reasons. We can fix the privacy issues with the Battery API by reducing the entropy of the value shared with the web page instead of blocking the capability entirely. I believe the specification is already recommending implementations to be careful with this. Why do we need to block the API even more, making it pretty much useless in a lot of situations?

MXEBot pushed a commit to mirror/chromium that referenced this issue Jun 14, 2017
…ing context only. (patchset #6 id:160001 of https://codereview.chromium.org/2880763002/ )

Reason for revert:
This CL should have gotten a API OWNER lgtm with an Intent to Ship. If the change is accepted by the API OWNERS, we should re-land this.

Original issue's description:
> [Battery] Allow usage from SecureContext or top-level browsing context only.
>
> Make the Battery Status API available only within a secure context that is
> also a top-level browsing context. This disallows the use of the API within
> framed content, as well as from any content that is not a secure context.
>
> Details: w3c/battery#10
>
> WPT updated in web-platform-tests/wpt#5871
>
> BUG=661792
>
> Review-Url: https://codereview.chromium.org/2880763002
> Cr-Commit-Position: refs/heads/master@{#476263}
> Committed: https://chromium.googlesource.com/chromium/src/+/3543d97ca7a33fb8ad48261ad252428555747896

TBR=timvolodine@chromium.org,foolip@chromium.org,rijubrata.bhaumik@intel.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=661792

Review-Url: https://codereview.chromium.org/2937553005
Cr-Commit-Position: refs/heads/master@{#479025}
@anssiko
Copy link
Member Author

anssiko commented Jun 14, 2017

The Battery API can be used by third party content for valid reasons.

Earlier, people were asking for use cases for the API in general, so I provided some in https://lists.w3.org/Archives/Public/public-device-apis/2016Jul/0011.html

None of these seem to be perfect fit for iframed content, although all of them could be implemented in an iframe as well.

What use cases we have for iframed content beyond tracking scripts that some folks are concerned about? Perhaps maps.google.com running in an iframe (see the third use case in my list above)?

We can fix the privacy issues with the Battery API by reducing the entropy of the value shared with the web page instead of blocking the capability entirely.

The thinking was reducing entropy is a complementary mitigation strategy.

I believe the specification is already recommending implementations to be careful with this.

Correct, https://w3c.github.io/battery/#security-and-privacy-considerations says:

The user agent should not expose high precision readouts of battery status information as that can introduce a new fingerprinting vector.

@mounirlamouri
Copy link
Member

I know that some websites use the Battery API to have an idea of the battery consumption changes when running experiments. For websites like YouTube, I think doing something like this on iframes in addition to main frame would make sense. I do not know if they do though. It would be easy to check if the API is used in a YT iframe.

@FluorescentHallucinogen

Does it affect Shadow DOM (e.g. Polymer web components for Battery Status API)?

@anssiko
Copy link
Member Author

anssiko commented Jun 15, 2017

I'll reopen this issue to revisit.

@anssiko
Copy link
Member Author

anssiko commented Jun 15, 2017

@mounirlamouri, we discussed your feedback on the WG call today, and I reopened this issue as a result of that to see if we should revert the spec change 53360f1 too.

We concluded it'd be great if we'd get your concrete use cases documented that require Battery Status API access from within an iframe in more detail.

Your use case to "have an idea of the battery consumption changes when running experiments" was a good one already, would like to understand a bit more how the API is used in the wild, and especially what type of content uses the API from within an iframe. Can we use telemetry to figure that out?

Another related suggestion from @dontcallmedom was to allow iframed content access the API as long as it is same-origin with the context that created it. This probably does not help with your use case of embedded YT player (or maps), for example.

Also we noted adding a policy directive for battery is possible future work, but that won't help with the existing web content that cannot be changed (i.e. cannot edit HTML, or add a new HTTP header).

@RByers
Copy link

RByers commented Jun 16, 2017

If we want to change the default, then I think there definitely should be a policy opt-in. This is pretty trivial to add these days - @clelland can help. I don't know how easy it is for cases like Maps and YouTube to get container pages to update their <iframe> elements, but it's at least something (and consistent with the other "interventions" we've done like blocking vibrate in iframes by default).

Also +1 to there being legitimate power-monitoring use cases here. I don't know details but I've heard that major mobile apps have been able to detect power regressions in the wild based ONLY on rough telemetry of how often they see a 2-digit battery meter tick down by 1%. If true, that's pretty powerful for empowering sites to improve their battery overheads. That case could, of course, be addressed with lower fingerprint risk by some sort of relative API instead of an absolute one.

@anssiko
Copy link
Member Author

anssiko commented Jun 19, 2017

If we want to change the default, then I think there definitely should be a policy opt-in. This is pretty trivial to add these days - @clelland can help. I don't know how easy it is for cases like Maps and YouTube to get container pages to update their <iframe> elements, but it's at least something (and consistent with the other "interventions" we've done like blocking vibrate in iframes by default).

@RByers, did I read you right that you'd be supportive of limiting this API to the top-level browsing context assuming we'd add an appropriate policy directive for battery?

@mounirlamouri, would that work for you too?

Also +1 to there being legitimate power-monitoring use cases here. I don't know details but I've heard that major mobile apps have been able to detect power regressions in the wild based ONLY on rough telemetry of how often they see a 2-digit battery meter tick down by 1%. If true, that's pretty powerful for empowering sites to improve their battery overheads. That case could, of course, be addressed with lower fingerprint risk by some sort of relative API instead of an absolute one.

Thanks! The power-monitoring use cases you expanded further sound very valuable indeed.

@RByers
Copy link

RByers commented Jun 19, 2017

@RByers, did I read you right that you'd be supportive of limiting this API to the top-level browsing context assuming we'd add an appropriate policy directive for battery?

Top-level-only by default with an opt-in to enable frames to be granted access (just like vibrate) - yes that seems like a reasonable design to me. But we'd have to examine the web compat implications to determine if we could make such a breaking change in Chrome (according to our removal process).

@clelland
Copy link

If we add a directive for battery to the feature policy (which is, as @RByers says, pretty simple to do), I think that the sensible default would be to allow use at the top level, and in any same-origin children, and block use in cross origin frames.

That's the stance I'm trying to advocate for new policy-controlled APIs, and we're reframing older ones to work the same way. There's really no point in trying to restrict access from same-origin frames, when they can just reach back up through parent to do anything they want anyway.

@anssiko
Copy link
Member Author

anssiko commented Aug 31, 2017

Submitted a PR #13 per @clelland's suggestion in #10 (comment)

@anssiko anssiko removed the v2 label Sep 7, 2017
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 a pull request may close this issue.

6 participants