1661174 - Verify we are not dereferencing WebShare URLs

Status: RESOLVED FIXED

(Core :: DOM: Navigation, task)

Description

[Marcos Caceres [:marcosc]]

From [1]

...Web Share API allows users to share links from the browser via 3rd party applications (e.g. mail and messaging apps). The problem is that file: scheme is allowed and when a website points to such URL unexpected behavior occurs. In case such a link is passed to the navigator.share function an actual file from the user file system is included in the shared message which leads to local file disclosure when a user is sharing it unknowingly. The problem is not very serious as user interaction is required, however it is quite easy to make the shared file invisible to the user. The closest comparison that comes to mind is clickjacking as we try to convince the unsuspecting user to perform some action.

Below are the steps to reproduce the issue:

1. Visit https://overflow.pl/webshare/poc1.html
2. Click “Share it with friends!”
3. Select the method (e.g. mail, messages)
4. “Send it” or “Share it” (or just inspect what has been attached)
5. Local /etc/passwd has been sent to the recipient

It would be good to have someone verify that we are not affected by this on Android. That is, we are not extracting the file:// contents and sending that.

Noting that we have a windows implementation too (but not shipped, AFAIK)... we might need a different bug for that, but just putting that here for the record.

[1] https://blog.redteam.pl/2020/08/stealing-local-files-using-safari-web.html

Comment 1

[Tiger Oakes]

We should be fine here, we don't have any special code to handle URLs. In fact, they're treated as text and just appended to the message body.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Do we think there is anything actionable here?

Comment 3

[Marcos Caceres [:marcosc]]

Probably nothing specific on Android, but we are discussing if we need to be more restrictive on the DOM side + spec.

Discussion here:
https://github.com/w3c/web-share/issues/173

Comment 4

[Daniel Veditz [:dveditz]]

We should, at the very least, run the URLs through our CheckMayLoad() checks--or whatever has replaced it. If a web site couldn't navigate to or include a URL as a subresource then it shouldn't be allowed to share it. Minimum bar, prevents non-file:/// urls from sharing file:/// and any page from sharing internal nonsense like chrome: or about: urls.

blob: URLs are also problematic -- any external program won't be able to resolve them, and if we ever have "web-share" targets they won't be accessible to targets that aren't same-origin. But if we allow sharing data at some point in the future, it does make sense for a site to be able to share it's own data, which might happen to be in blob: form for convenience but could be converted to a self-contained data: url.

I'm not sure it makes sense to allow a page to be able to share schemes handled by external apps. Maybe the better approach is to create a short allow-list of "web safe" schemes. Preferably in the spec so it's consistent across browsers.

Comment 5

[Marcos Caceres [:marcosc]]

Attachment: Bug 1661174 - restrict schemes supported by Web Share API

Restrict to "loadable" URLs. Also exclude blob URLs.

Comment 6

[Marcos Caceres [:marcosc]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b452aa9d9314e02167b1ef60fcd78182872a2fc

Comment 7

[Marcos Caceres [:marcosc]]

If it doesn't stick, someone else might need to take this over from tomorrow 🙏

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out Backed out for causing wpt failures in share-url-invalid.https.html

https://hg.mozilla.org/integration/autoland/rev/55d3f8ee3a9ca2d1e0dedeb6489acf1581231901

after it had landed as https://hg.mozilla.org/integration/autoland/rev/a2f86092c968318ed33f79a591bae9fc58fbdd85

Push which ran the failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=2056a83136fc3752c5ec217e039b28a1068b6eeb&selectedTaskRun=asLJCqPsTomR37IRoNtVWA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=314475885&repo=autoland

TEST-UNEXPECTED-FAIL | /web-share/share-url-invalid.https.html | share() rejects file:// URLs - promise_test: Unhandled rejection with value: object "TypeError: navigator.share is not a function"
etc.

Good luck for the future, Marcos.

Martin, can you find someone to take of the failures and reland it?

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This triggered also these geckoview failures: https://treeherder.mozilla.org/logviewer.html#?job_id=314476347&repo=autoland

[task 2020-08-31T14:08:45.542Z] 14:08:45 INFO - org.mozilla.geckoview.test | Error in saveAndRestoreState(org.mozilla.geckoview.test.ProgressDelegateTest):
[task 2020-08-31T14:08:45.542Z] 14:08:45 INFO - org.mozilla.geckoview.test | java.lang.AssertionError: Scroll position should match
[task 2020-08-31T14:08:45.542Z] 14:08:45 INFO - org.mozilla.geckoview.test | Expected: a numeric value within <0.5> of <100.0>
[task 2020-08-31T14:08:45.542Z] 14:08:45 INFO - org.mozilla.geckoview.test | but: <0.0> differed by <99.5>

Comment 10

[Martin Thomson [:mt:]]

I think that this is down to our windows runners not supporting the necessary hooks for this API (and the new tests not being properly listed). I've a patch up on try for which I'll need to find someone to review.

Comment 11

[Jens Stutte [:jstutte]]

Hi Edgar, can you support Martin here once the patch is up for review?

Comment 12

[Edgar Chen [:edgar]]

Sure thing, feel free to send the review request to me.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This landed:

https://hg.mozilla.org/integration/autoland/rev/697a0ad573c6d605c3fad05d2c819e78012ac10b

And got backed out for PromptDelegateTest failures:

https://hg.mozilla.org/integration/autoland/rev/78015058138ad21efa05bb3e8d86eca200400247

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=geckoview-junit&revision=697a0ad573c6d605c3fad05d2c819e78012ac10b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=314937868&repo=autoland

TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.PromptDelegateTest.dismissedShareReturnsAbortError | java.lang.AssertionError: Error should be correct
TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.PromptDelegateTest.abortedShareReturnsAbortError | java.lang.AssertionError: Error should be correct
TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.PromptDelegateTest.failedShareReturnsDataError | java.lang.AssertionError: Error should be correct
TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.PromptDelegateTest.shareUrlSucceeds | java.lang.AssertionError: Share must succeed.TypeError: Navigator.share: Share URL https://example.com/ must be either http:// or https://.

Comment 14

[Martin Thomson [:mt:]]

edgar, I know that I'm now asking more of you for this than you signed on for, but do you have any idea why these tests might be failing? It took me about a week to reach the point where I could run these tests at all but I'm not able to reproduce the test failure that occurs on CI.

I'm running in an emulator on Linux. I tried on Windows, but failed.

Comment 15

[Edgar Chen [:edgar]]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #13)

FAIL org.mozilla.geckoview.test.PromptDelegateTest.shareUrlSucceeds - java.lang.AssertionError: Share must succeed.TypeError: Navigator.share: Share URL https://example.com/ must be either http:// or https://

I think it is failed because the share URL (https://example.com/) and test page are not same-origin, so CheckMayLoad() returns false.
It depends on what is behavior we expect: only share same-origin URL v.s. just check if the share URL is http(s).
Per https://github.com/w3c/web-share/pull/174, It seems the latter one is what we expect? (sorry, I didn't not go through all sepc discussion). If so, then CheckMayLoad is too strict, we could replace it with net::SchemeIsHTTP(URL) || net::SchemeIsHTTPS(URL).
If the former one is what we expect, then we need to modify the test, and perhaps add more.

Comment 16

[Martin Thomson [:mt:]]

Thanks Edgar, that helps point in the right direction. I think that Dan might have mentioned that we have a process for determining whether we can navigate from A to B. For instance, we prevent some origins from navigating to file:///. I thought - incorrectly - that this was what Marcos was invoking here, but that is clearly not the case. Dan, I don't know where to start looking, do you know the function that you are talking about?

Comment 17

[Daniel Veditz [:dveditz]]

Argh -- I confused two similar names (imho CheckMayLoad is misnamed since it's really about access (JS/DOM) not loading (network)).

I meant we should use nsIScriptSecurityManager::CheckLoadURIWithPrincipal() (note SSM, not the principal)
https://searchfox.org/mozilla-central/source/caps/nsIScriptSecurityManager.idl#116

Comment 18

[Martin Thomson [:mt:]]

Thanks Dan, I've managed to implement that and the tests pass (so that seems like a win).

Christoph, I'm not familiar enough with the flags on CheckLoadURIWithPrincipal to be confident in the change here. Can you give me a quick sanity check? In particular, should I need to include a javascript:window.alert('hi') share target to test the DISALLOW_SCRIPT flag?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Martin Thomson [:mt:] from comment #18)

Christoph, I'm not familiar enough with the flags on CheckLoadURIWithPrincipal to be confident in the change here. Can you give me a quick sanity check? In particular, should I need to include a javascript:window.alert('hi') share target to test the DISALLOW_SCRIPT flag?

I left comments in phab, probably we can switch to an allow list based approach entirely. Happy to look it over again.

Comment 20

[Martin Thomson [:mt:]]

Try build here.

I've opened an issue on the spec that I hope resolves in the right direction. It's easy to change the code to a simple allowlist, but I'd rather not for the reasons stated there.

Assuming that is OK, I'd like one last check please Edgar and Christoph.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Martin Thomson [:mt:] from comment #20)

Assuming that is OK, I'd like one last check please Edgar and Christoph.

Thanks, I r+ the patch in phab assuming you update to the revision you pushed to TRY which looks fine to me. Thanks!

Comment 22

[Edgar Chen [:edgar]]

(In reply to Martin Thomson [:mt:] from comment #20)

Try build here.

Assuming that is OK, I'd like one last check please Edgar and Christoph.

The patch on try looks good to me. Thanks.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/0d9167012994d0eedf86fcc2b8ee8690e2109696
https://hg.mozilla.org/mozilla-central/rev/0d9167012994

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Is this worth uplifting to Beta?

Comment 25

[Martin Thomson [:mt:]]

The pref defaults off, so no real point.

Comment 26

[Andrei Bodea[:andrei]]

I tried to check this issue following the steps from the description and I saw the following:
When I try to share with Chrome, the message is: check out this cute kitten! https://somerandommimagewabesite.com/cat.jpg
4:30
But when I share from Fenix I have: file:///etc/passwd.
Device: Google Pixel 4 XL (Android 11), Latest Nightly 9/29 with GV 83.0.a1.
After the changes do this bug require QA? Are there any steps or something to help me test this issue?
@Daniel can you please help me with this one?

Comment 27

[Kevin Brosnan [Ex-Mozilla]]

This fix is in DOM so not surprised that Fenix does not know about the Gecko flag.

Comment 28

[Martin Thomson [:mt:]]

I can confirm that Android 20200922094538 responds to the "share" button on the POC. With or without dom.webshare.enabled set, so it appears as though Fenix has a parallel implementation of this API. It doesn't seem to load the file though - at least in the apps that I tried. We might need another bug to track that.

Comment 29

[Martin Thomson [:mt:]]

(In reply to Martin Thomson [:mt:] from comment #28)

I can confirm that Android 20200922094538 responds to the "share" button on the POC. With or without dom.webshare.enabled set [...]

My mistake. I didn't reload the page after flipping the pref. And the build ID would seem to indicate that it is older than the fix.

Comment 30

[Daniel Veditz [:dveditz]]

The spec has been updated and now says

If url's scheme is not "http" or "https", return a promise rejected with TypeError.

Correct behavior is now to reject the share() promise. In today's Fenix Nightly I'm still seeing it share the URL text like comment 26, which isn't as terrible as sending the file content but is still wrong. Does Fenix Nightly update Gecko/GeckoView nightly? The API is in Gecko and the promise rejection is based on browser information (like the window principal) -- it shouldn't require Fenix sending it any special information.

Sending the URL text, rather than the file data, is not terrible though.

Comment 31

[Martin Thomson [:mt:]]

Dan, we should be blocking the PoC in Fenix, but I'm still on 20200922094538 despite having received an update (I just downloaded 70+M, so I've no idea what that did). I think that it before this fix landed, so I'm not worried (yet).

Comment 32

[Andrei Bodea[:andrei]]

Re-checked in the latest Nightly from 19/10 Nightly 201019 05:02 (Build #2015770537)AC: 63.0.20201016221440, acd9d4c70GV: 83.0a1-20201016094031AS: 63.0.0.
Note that when I tap on the share it with friends! nothing happens. I had open in links turned ON. On other browsers, I was able to open it in Gmail.
Also, note that no error or message displayed when trying to open that share.
Is this the intended behavior?