1433707 - Activity Stream content page can navigate to file: or other protocols

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Ed Lee :Mardak]

I was looking into bug 1401932 as it's a parity feature of the old Tiles about:newtab page, which had chrome privileges and could navigate directly to file: pages, but the current page could not do so with a plain <a href> link. Turns out the page can already send a message to chrome to navigate to any URI.

I would guess the main concern is from remotely loaded scripts or maybe 3rd party scripts? Bug 1408349?

I'm not sure if old about:home had a similar concern as this bug if that version had no way to navigate to arbitrary pages?

One broad fix would be to have chrome only allow navigation messages to http/s urls and wontfix bug 1401932. Having a dynamic whitelist to include pinned links wouldn't really do anything as the content page can also send a message to chrome to pin any link.

https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/PlacesFeed.jsm#285-291

Comment 1

[Ed Lee :Mardak]

freddyb, should we only handle OPEN_LINK (and OPEN_NEW_WINDOW and OPEN_PRIVATE_WINDOW) actions that have http/s urls? And wontfix bug 1401932?

Or ideas of how we can allow users to add/pin non-http/s top sites?

Comment 2

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #1)
> freddyb, should we only handle OPEN_LINK (and OPEN_NEW_WINDOW and
> OPEN_PRIVATE_WINDOW) actions that have http/s urls? And wontfix bug 1401932?
> 
> Or ideas of how we can allow users to add/pin non-http/s top sites?

At least off-hand, we should pass a triggering principal for openLinkIn (see also bug 1374741 and friends) and let docshell make the right decision. Speaking of which, that bug needs a new owner...

Of course, in a way that just punts the decision to "what's the right triggering principal". But we could potentially pass a dummy file: URI (codebase) principal which would explicitly allow file: but not e.g. chrome:// . I think.

Comment 3

[Frederik Braun [:freddy]]

What Gijs said :)

Comment 4

[Ed Lee :Mardak]

freddyb, I suppose my question is more of "should we even allow access to file:", i.e., is this even a security concern? If we set the triggering principal to allow for any file:, then I guess it would be it's okay? If it's not desirable to allow for any file:, then is there a reasonable whitelist?

Comment 5

[Ed Lee :Mardak]

Well, the team has triaged bug 1401932 as WONTFIX, so there won't be a need to allow for file: or other URIs. We can lock things down a bit to only allow navigating to http/s if that helps?

Comment 6

[Ed Lee :Mardak]

Is this the expected behavior?

Security Error: Content at moz-nullprincipal:{8bef2d01-e375-8945-afdd-66f5b0d166d0} may not load or link to file:///.
Security Error: Content at moz-nullprincipal:{3e3fce0c-0b45-6a4f-9046-4340672a3b66} may not load or link to about:config.

Where the page navigates away from about:newtab and shows a blank page with loading animation in tab forever(?).

```diff
diff --git a/system-addon/lib/PlacesFeed.jsm b/system-addon/lib/PlacesFeed.jsm
index 5cd3b40d..9c6965b0 100644
--- a/system-addon/lib/PlacesFeed.jsm
+++ b/system-addon/lib/PlacesFeed.jsm
@@ -240,21 +240,24 @@ class PlacesFeed {
         type: at.PLACES_LINK_BLOCKED,
         data: {url: value}
       }));
     }
   }
 
   /**
    * Open a link in a desired destination defaulting to action's event.
    */
   openLink(action, where = "", isPrivate = false) {
-    const params = {private: isPrivate};
+    const params = {
+      private: isPrivate,
+      triggeringPrincipal: Services.scriptSecurityManager.createNullPrincipal({})
+    };
 
     // Always include the referrer (even for http links) if we have one
     const {event, referrer} = action.data;
     if (referrer) {
       params.referrerPolicy = Ci.nsIHttpChannel.REFERRER_POLICY_UNSAFE_URL;
       params.referrerURI = Services.io.newURI(referrer);
     }
 
     const win = action._target.browser.ownerGlobal;
     win.openLinkIn(action.data.url, where || win.whereToOpenLink(event), params);
```

Comment 7

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #6)
> Is this the expected behavior?
> 
> Security Error: Content at
> moz-nullprincipal:{8bef2d01-e375-8945-afdd-66f5b0d166d0} may not load or
> link to file:///.
> Security Error: Content at
> moz-nullprincipal:{3e3fce0c-0b45-6a4f-9046-4340672a3b66} may not load or
> link to about:config.
> 
> Where the page navigates away from about:newtab and shows a blank page with
> loading animation in tab forever(?).

Sounds like we get halfway through trying to load things and then realize we shouldn't... some more debugging info might be helpful. Does the same happen when trying to navigate to, say, about:feeds (which isn't chrome privileged but also isn't web linkable so the null principal should generate the same error being the triggering principal).

Comment 8

[Ed Lee :Mardak]

Just updated m-c, and it no longer has the infinite loading animation. And for about:feeds:

Security Error: Content at moz-nullprincipal:{ffebf706-df3f-2b4d-aa1f-ebb2dbeda75e} may not load or link to about:feeds.

And shows a blank page.

At this point, if someone were able to inject a script that triggered OPEN_LINK, it could disrupt the user by opening a bunch of blank tabs / windows. Perhaps good enough for now?

Comment 9

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #8)
> Just updated m-c, and it no longer has the infinite loading animation. And
> for about:feeds:
> 
> Security Error: Content at
> moz-nullprincipal:{ffebf706-df3f-2b4d-aa1f-ebb2dbeda75e} may not load or
> link to about:feeds.
> 
> And shows a blank page.
> 
> At this point, if someone were able to inject a script that triggered
> OPEN_LINK, it could disrupt the user by opening a bunch of blank tabs /
> windows. Perhaps good enough for now?

I think so. It could do that anyway by just running lots of window.open("about:blank", "_blank") calls, I think, right?

Comment 10

[Ed Lee :Mardak]

window.open triggers the "Nightly prevented this site from opening # pop-up windows." but that can still used based on triggering events I believe.

Comment 11

[Frederik Braun [:freddy]]

Looks good. Can you make sure that 'javascript:' URLs are blocked as well and add tests for all those cases?

Comment 12

[Ed Lee :Mardak]

Looks like it does indeed prevent this alert from appearing.

sendAsyncMessage("ActivityStream:ContentToMain", {type: "OPEN_LINK", data: {url: "javascript:alert(1)"}})

Comment 13

[Ed Lee :Mardak]

Attachment: v1

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
A prerequisite is to getting a script to run in about:newtab, which right now is snippets service, I'm not sure if this bug is sec-other or inherits sec-moderate of bug 1408349.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I suppose "scriptSecurityManager" kinda calls it out.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
Activity stream was introduced/on-by-default in 57 but it was in the code in 56 via bug 1384807.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports would need a different patch but should be similarly simple. (There's more call sites that were refactored to a single one in 60.)

> How likely is this patch to cause regressions; how much testing does it need?
Should be pretty safe / simple change.

In terms of landing, I suppose if we do the review here, I wouldn't need to do a PR on github. We export to mozilla-central in batches, so it'll be combined with other changes anyway.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8947415 [details] [diff] [review]
v1

We rated this as a sec-moderate security issue so this doesn't need sec-approval to land.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Go ahead and land on trunk.

Comment 15

[Kate Hudson :k88hudson]

Comment on attachment 8947415 [details] [diff] [review]
v1

Ok, looks good, works as expected. Thanks!

Comment 16

[Liz Henry (:lizzard)]

Ed, is this something you think we should fix in 59 as well?

Comment 17

[Ed Lee :Mardak]

I don't think it's necessary to fix in 59 given that bug 1433707 won't be fixed for 59. The fix here makes it so if that other bug is taken advantage of, it would slightly limit the impact.

Comment 18

[Ed Lee :Mardak]

This landed as part of bug 1435419
https://hg.mozilla.org/mozilla-central/rev/3ef8041b6eb9

Comment 19

[Ed Lee :Mardak]

Looks like bug 1599368 wants to change this Null principal to openTrustedLinkIn, which uses System principal, to support SameSite cookies. To not regress this bug, I would guess we need to do explicit protocol / url checks instead of relying on the principal?

Comment 20

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #19)

Looks like bug 1599368 wants to change this Null principal to openTrustedLinkIn, which uses System principal, to support SameSite cookies. To not regress this bug, I would guess we need to do explicit protocol / url checks instead of relying on the principal?

Yes. I couldn't find this bug when doing the archaeology for the other bug (in a slack discussion with Scott). We need system principal in order for it to count as a samesite navigation.

In terms of safe protocols - http/https, for sure, but after that I'm not sure. What was the rationale for blocking about: and file: links? I don't actually see much in this bug, perhaps the discussion happened elsewhere?

Comment 21

[Daniel Veditz [:dveditz]]

The reason for being careful is that page has lots of sources of input and we don't know which of them might be compromised or gamed in some future iteration of the code. Some about: pages are privileged, and some take parameters that cause things to happen (hopefully not both). file: URLs had, until recently, an ability to read other unrelated files stored nearby and potentially exfiltrate them (multi-stage attack -- you'd have to get a malicious file downloaded first). It's not a system-privileged page because we don't trust it as much as system privileged pages and it's safest to implement a consistent set of security rules (in this case wrt what pages can link to).

All the Pocket urls will be web links (http/https), and top user sites should be, too. I can't think of any other scheme we're likely to need. Any scheme we don't surface on the newtab page can still be made into a real bookmark and launched from there.