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

Reject non-http URLs for url member #174

Merged
merged 6 commits into from
Sep 21, 2020
Merged

Reject non-http URLs for url member #174

merged 6 commits into from
Sep 21, 2020

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Aug 27, 2020

Closes #173

For normative changes, the following tasks have been completed:

  • Modified Web platform tests - incoming with Gecko patch.

Implementation commitment:

@whsieh
Copy link

whsieh commented Aug 27, 2020

It looks like https://github.com/web-platform-tests/wpt/blob/master/web-share/canShare.tentative.https.html still checks that we're able to share data URLs. Would that test need to be updated as well?

@ewilligers
Copy link
Collaborator

The webkit patch allows data urls.

@marcoscaceres do you also consider them problematic?

@martinthomson
Copy link
Member

I think that this needs more treatment than this alone. It needs to acknowledge the risk that share targets could retrieve content from the URL, and that they might follow redirects (or be subject to DNS tweaking) in order to exploit their position in the network to access information that might otherwise be protected by firewalls and similar. If the application also passes on that information, that leads to an unexpected information leakage.

I don't think that it is appropriate to say that this is sufficient to address #173.

@marcoscaceres
Copy link
Member Author

@ericwilligers, about data: URLs, yes, as if they get dereferenced and previewed, then you can get into a situation like:

data:text/html;<meta http-equiv="refresh" content="1;url=file:///here/we/go/again">

@whsieh, what was the rationale for still allowing data: on the Webkit side? Also, you are right about canShare(), but that's a different matter. It's not supposed to leave the browser... but will address. I'm still hopeful we can fix canShare() by doing what I proposed in #108. Maybe with more urgency now?

@martinthomson agree, definitely need to add a security considerations section for apps ingesting these URLs. As you suggested, we are just one link a weak chain here and OSs and apps able to inject need to be more careful.

@marcoscaceres marcoscaceres marked this pull request as draft August 27, 2020 06:45
@marcoscaceres
Copy link
Member Author

@martinthomson, I added your comment above more or less verbatim as they capture the risk quite nicely from a bunch of different angles. Please feel free to suggest additional changes.

@marcoscaceres
Copy link
Member Author

I file w3ctag/security-questionnaire#96 to capture our cautionary tale of trusting URLs and thinking receiving parties will be careful with what they are receiving.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@whsieh
Copy link

whsieh commented Aug 27, 2020

@whsieh, what was the rationale for still allowing data: on the Webkit side? Also, you are right about canShare(), but that's a different matter. It's not supposed to leave the browser... but will address. I'm still hopeful we can fix canShare() by doing what I proposed in #108. Maybe with more urgency now?

I believe the rationale for still allowing the data protocol in the change was just to keep the existing web platform tests passing.

@ewilligers
Copy link
Collaborator

@whsieh, for the share targets you tested, is sharing a http(s) url that redirects a problem?

If not, is sharing the following simply equivalent to sharing a http(s) url that redirects, and not equivalent to sharing a file:// url?

data:text/html;<meta http-equiv="refresh" content="1;url=file:///here/we/go/again">

@whsieh
Copy link

whsieh commented Aug 27, 2020

@whsieh, for the share targets you tested, is sharing a http(s) url that redirects a problem?

If not, is sharing the following simply equivalent to sharing a http(s) url that redirects, and not equivalent to sharing a file:// url?

data:text/html;<meta http-equiv="refresh" content="1;url=file:///here/we/go/again">

This is not a problem (at least, on Cocoa platforms) because neither the share targets (Messages and Mail in this case) nor the UA attempt to load the data URL as web content and then send the web content to the recipient.

@marcoscaceres marcoscaceres marked this pull request as ready for review September 16, 2020 09:20
@marcoscaceres marcoscaceres merged commit 9322cbe into master Sep 21, 2020
@marcoscaceres marcoscaceres deleted the URL_mitigations branch September 21, 2020 09:28
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
Spec:
https://w3c.github.io/web-share/#share-method

If a share() request is active when share() is called again, the new
share request fails immediately.
w3c/web-share#113

In content shell, the OS-integration for the share service is not
present. Instead of crashing in tests, we report a not implemented
error.

WPT web-share/share-sharePromise-internal-slot.https.html now passes
(It previously crashed.)

Protocols other that http and https are no longer supported by
share() or canShare().
w3c/web-share#174

canShare is being discussed in
w3c/web-share#177

Bug: 1002337, 1002514, 1131755
Change-Id: I4ec9f6eb03373fd5c6db1881df906a8df36ca4ff
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
Spec:
https://w3c.github.io/web-share/#share-method

If a share() request is active when share() is called again, the new
share request fails immediately.
w3c/web-share#113

In content shell, the OS-integration for the share service is not
present. Instead of crashing in tests, we report a not implemented
error.

WPT web-share/share-sharePromise-internal-slot.https.html now passes -
it previously crashed.
w3c/web-share#183 improves consistency between
the test and the spec.

Protocols other that http and https are no longer supported by
share() or canShare().
w3c/web-share#174

canShare is being discussed in
w3c/web-share#177

Bug: 1002337, 1002514, 1131755
Change-Id: I4ec9f6eb03373fd5c6db1881df906a8df36ca4ff
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 26, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 28, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Glen Robertson <glenrobchromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Cr-Commit-Position: refs/heads/master{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755

UltraBlame original commit: 527027b783378db7e3c1fcdd908e718799400989
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 28, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Glen Robertson <glenrobchromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Cr-Commit-Position: refs/heads/master{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755

UltraBlame original commit: 527027b783378db7e3c1fcdd908e718799400989
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 28, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Glen Robertson <glenrobchromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Cr-Commit-Position: refs/heads/master{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755

UltraBlame original commit: 527027b783378db7e3c1fcdd908e718799400989
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this pull request Oct 1, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 26, 2021
data: URLs are no longer supported by Web Share API.
w3c/web-share#174

This is tested by automated tests.

The manual test in obsolete, it is not valid or needed.

closes #202

Co-authored-by: Eric Willigers <[email protected]>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 27, 2021
…https.html, a=testonly

Automatic update from web-platform-tests
Web Share: Retire share-url-data-manual.https.html (#28654)

data: URLs are no longer supported by Web Share API.
w3c/web-share#174

This is tested by automated tests.

The manual test in obsolete, it is not valid or needed.

closes #202

Co-authored-by: Eric Willigers <[email protected]>
--

wpt-commits: 2eb3853c6bb77eed4a3b5de3b6042b72463efd65
wpt-pr: 28654
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
GitOrigin-RevId: 060b7f1b2de01048a934bc4aca41973edaf4d12c
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.

Shared URLs can be auto-fetched by native applications without respecting same-origin policy
4 participants