1407891 - View SVG Image for images with data URL fails

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Martin Thomson [:mt:]]

STR:

1. Visit a page that includes an image with src="data:..." (example in the URL field).

2. Right click the image and choose "View Image".

Actual: Nothing happens.

Expected: Navigate to the data: URL.

I have verified that tweaking security.data_uri.block_toplevel_data_uri_navigations makes this work as expected.

This is a chrome action and the deliberate choice of users, so it should work.

Comment 1

[Astley Chen (inactive)]

It's intentional from bug 1396798 for not allowing the SVG image with data URI as toplevel document.

http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#55

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Martin Thomson [:mt:] from comment #0)
> 1. Visit a page that includes an image with src="data:..." (example in the
> URL field).

Martin, as mentioned in comment 1, we intentionally block data:image/svg, but allow other data:image/*. I assume you were testing SVGs, or did you experience some problems with other images too?

Comment 3

[Martin Thomson [:mt:]]

OK, it's just SVG; I didn't realize that they were somehow special.

I'm sure that it's intentional, but it still violates expectations.  The problem is that this is not a navigation triggered by the site, but by the *user*.

FWIW, the workaround is to copy the image URL and paste that in the navigation bar.

Comment 4

[Astley Chen (inactive)]

Yes, from user's perspective, it looks like a bug as it's an explicit action but ends up nothing.
I'm not sure whether there is a way to allow it pass the check.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Martin Thomson [:mt:] from comment #3)
> I'm sure that it's intentional, but it still violates expectations.  The
> problem is that this is not a navigation triggered by the site, but by the
> *user*.

Ah, I missed that part - the navigation is triggered by the user, right.

Boris, how would you feel if we move the triggeringPrincipal == SystemPrincipal check [1] before we even do all the data: URI checks, like in that case, image? I suppose all the data loads that are explicitly triggered by the user (which equals systemprincipal) we can allow. What do you think?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#63

Comment 6

[Boris Zbarsky [:bzbarsky]]

"View Image" does NOT use a system triggeringPrincipal.  If it did, we would in fact allow it, because there are no "return false" in nsContentSecurityManager::AllowTopLevelNavigationToDataURI before the systemprincipal check.  That is, nothing before that check can _deny_ the load, only allow it.

If you look at the viewMedia function in browser/base/content/nsContextMenu.js then in the non-canvas case it does:

    let referrerURI = gContextMenuContentData.documentURIObject;
    openUILink(this.mediaURL, e, { disallowInheritPrincipal: true,
                                   referrerURI });

and openUILink ends up calling openUILinkIn, which calls openLinkIn, which does:

  var aTriggeringPrincipal  = params.triggeringPrincipal;

(in this case undefined, but it becomes null by the time we call nsIWebNavigation::LoadURIWithOptions with that argument).

That said, using "Open Frame in New Tab" on something like:

  data:text/html,<iframe src="data:text/html,hey"></iframe>

does work.  That calls:

    let referrer = gContextMenuContentData.referrer;
    openLinkIn(gContextMenuContentData.docLocation, "tab",
               { charset: gContextMenuContentData.charSet,
                 referrerURI: referrer ? makeURI(referrer) : null });

which is somewhat similar; the difference is the missing "disallowInheritPrincipal: true" and how the referrer is gotten.

So what happens on the docshell level in these two cases?  In the iframe case, we land in nsDocShell::LoadURI, triggeringPrincipal is null, referrer is null (!), so we use a system principal as the triggering principal:


  if (!triggeringPrincipal) {
    if (referrer) {
      nsresult rv = CreatePrincipalFromReferrer(referrer,
                                                getter_AddRefs(triggeringPrincipal));
      NS_ENSURE_SUCCESS(rv, rv);
    }
    else {
      triggeringPrincipal = nsContentUtils::GetSystemPrincipal();
    }
  }

In the SVG image case, we have a non-null referrer URI and use it as the triggering principal....

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1407891_allow_img_svg.patch

Boris, thanks for your evaluation within comment 6. Since we are trying to pass an accurate triggeringPrincipal from the front-end to the back-end within our efforts within Bug 1333030 and dependents I was wondering if you should just pass a SystemPrincipal as the triggeringPrincipal explicitly. How would you feel about that?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8925003 [details] [diff] [review]
bug_1407891_allow_img_svg.patch

This is a tough call, because it basically allows getting around CheckLoadURI checks.  It makes me somewhat uncomfortable.

I assume you've verified that "disallowInheritPrincipal: true" means you don't get javascript: or data: executing with system privileges, at least, right?

What happens if this is done on a file:// link on an http page?  Should that allow the load?

I would really like us to have a coherent security policy around this sort of interaction...

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> This is a tough call, because it basically allows getting around
> CheckLoadURI checks.  It makes me somewhat uncomfortable.

I agree it's a tough call; on the other hand there is a CheckLoadURI check right before we call OpenUILinkIn using the correct principal, and DISALLOW_SCRIPT:

urlSecurityCheck(this.mediaURL,
                 this.browser.contentPrincipal,
                 Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
 
I assume (not entirely sure) viewMedia is only called if the user explicitly performs some right-click action, like e.g. right-click->view-media, right? In that case the image would have allowed to be loaded in some context at the first place.

The load itself is triggered by the user, hence systemPrincipal seems accurate to me. Unfortunately I don't think there is an easy workaround. If we go down that route of using systemPrincipal for the load itself, we should definitely add a comment and making sure that no one removes the urlSecurityCheck() before we do the load.

Comment 10

[Boris Zbarsky [:bzbarsky]]

It really doesn't make me comfortable to have comments about some security check needing to happen somewhere else...

Another option is to leave the triggering principal as the page, but add some sort of annotation to the load that makes us allow data: independent of triggering principal, right?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> Another option is to leave the triggering principal as the page, but add
> some sort of annotation to the load that makes us allow data: independent of
> triggering principal, right?

In general, I think the approach of using the triggeringPrincipal as the distinction factor to allow a data: URI load seems fine. For that particular case, I think we could pass a flag (similar to [1]) which we then set in the loadInfo. E.g. loadinfo.allowDataURILoad which we then check within the contentSecurityManager and allow the data URI to load.

Probably this is the way to go forward.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#224

Comment 12

[Boris Zbarsky [:bzbarsky]]

Right, I think using triggeringPrincipal in general is good.  Just talking about this specific case, where we basically want to override the default behavior.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1407891_allow_img_svg.patch

Boris, within this patch I am propagating a specific flag to allow the data URI navigation, regardless of the principal. If you agree with that approach I am happy to provide an automated test. What do you think?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1407891_allow_img_svg_tests.patch

Here is an automated test for it!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926107 [details] [diff] [review]
bug_1407891_allow_img_svg.patch

In the docshell APIs, the fact that we have "navigation" is implicit, but the key part is that we're forcing the thing being allowed...  So maybe forceAllowDataURI or some such and similar naming in other bits?

Apart from that, looks good, thank you!  r=me.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, one thing, though...  It sure would be good if we also updated the openUILink to take a triggering principal, not just a referrer URI.  Followup for that, please.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926180 [details] [diff] [review]
bug_1407891_allow_img_svg_tests.patch

Nice!  r=me

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> Oh, one thing, though...  It sure would be good if we also updated the
> openUILink to take a triggering principal, not just a referrer URI. 
> Followup for that, please.

I agree - we have Bug 1374741 for that. Kate is on it.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

It seems that lint doesn't really like my change:

ckerschbaumer-44248:mc ckerschbaumer$ ./mach lint browser/base/content/utilityOverlay.js
/Users/ckerschbaumer/Documents/moz/mc/browser/base/content/utilityOverlay.js
  210:1  error  Function 'openLinkIn' has a complexity of 41.  complexity (eslint)

✖ 1 problem (1 error, 0 warnings)


Overall, it seems there is nothing I can do to fix that lint error, the only option I see is to disable complexity.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69e828da2238
Allow view-image to open a data: URI by setting a flag on the loadinfo. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5b81ec0ae6
Test navigation for right-click view-image on data:image/svg. r=bz

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/69e828da2238
https://hg.mozilla.org/mozilla-central/rev/5a5b81ec0ae6