Closed Bug 1407891 Opened 2 years ago Closed 2 years ago

View SVG Image for images with data URL fails

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: mt, Assigned: ckerschb)

References

()

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(2 files, 1 obsolete file)

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.
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
Summary: View Image for images with data URL fails → View SVG Image for images with data URL fails
(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?
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
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.
(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
Flags: needinfo?(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....
Flags: needinfo?(bzbarsky)
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Attached patch bug_1407891_allow_img_svg.patch (obsolete) — Splinter Review
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?
Attachment #8925003 - Flags: review?(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...
Flags: needinfo?(ckerschb)
Attachment #8925003 - Flags: review?(bzbarsky)
(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.
Flags: needinfo?(ckerschb)
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?
(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
Right, I think using triggeringPrincipal in general is good.  Just talking about this specific case, where we basically want to override the default behavior.
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?
Attachment #8925003 - Attachment is obsolete: true
Attachment #8926107 - Flags: review?(bzbarsky)
Here is an automated test for it!
Attachment #8926180 - Flags: review?(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.
Attachment #8926107 - Flags: review?(bzbarsky) → review+
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 on attachment 8926180 [details] [diff] [review]
bug_1407891_allow_img_svg_tests.patch

Nice!  r=me
Attachment #8926180 - Flags: review?(bzbarsky) → review+
(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.
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.
Pushed by mozilla@christophkerschbaumer.com:
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
You need to log in before you can comment on or make changes to this bug.