1343279 - "View Image" doesn't work anymore on canvas

Status: VERIFIED FIXED

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

Description

[Loic]

STR:
1) Open canvas http://html5demos.com/canvas
2) Right click on it and select "View Image"

Result: no action.
Expected: display canvas as blob ressource (blob:null/78a41db8-75d3-4571-8061-09d02cda7308) in location bar

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9307d60e21ba70f46fb47c9eb43b6b5246b7e64c&tochange=d31df518cb6ec920cf9ab7accb67682298f8a9f8

Regressed by bug 1182569.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Thanks Loic - I'll have a look ASAP.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

So, it seems that the contentSecurityManager is blocking the load, here are the args and a stacktrace underneath:

doContentSecurityCheck {
  channelURI: blob:null/ea613288-72fc-4d9e-8b37-1e4a5f24b199
  loadingPrincipal: nullptr
  triggeringPrincipal: http://html5demos.com/canvas
  principalToInherit: NullPrincipal
  contentPolicyType: 6
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 
  initalSecurityChecksDone: no
  enforceSecurity: no
}


0x00007fffe6babbbf in nsContentSecurityManager::CheckChannel (aChannel=0x7fffc11539d0) at /home/ckerschb/source/mc/dom/security/nsContentSecurityManager.cpp:738
738       MOZ_ASSERT(false, "uahaha");
(gdb) bt
#0  0x00007fffe6babbbf in nsContentSecurityManager::CheckChannel (aChannel=0x7fffc11539d0) at /home/ckerschb/source/mc/dom/security/nsContentSecurityManager.cpp:738
#1  0x00007fffe6baadb6 in nsContentSecurityManager::doContentSecurityCheck (aChannel=0x7fffc11539d0, aInAndOutListener=[(nsDocumentOpenInfo *) 0x7fffcad5d140])
    at /home/ckerschb/source/mc/dom/security/nsContentSecurityManager.cpp:577
#2  0x00007fffe3982a44 in nsBaseChannel::AsyncOpen2 (this=0x7fffc1153980, aListener=0x7fffcad5d140) at /home/ckerschb/source/mc/netwerk/base/nsBaseChannel.cpp:703
#3  0x00007fffe49bd651 in nsURILoader::OpenURI (this=0x7fffcb162840, channel=0x7fffc11539d0, aFlags=0, aWindowContext=0x7fffc6485030)
    at /home/ckerschb/source/mc/uriloader/base/nsURILoader.cpp:839
#4  0x00007fffe8e487dd in nsDocShell::DoChannelLoad (this=0x7fffc6485000, aChannel=0x7fffc11539d0, aURILoader=0x7fffcb162840, aBypassClassifier=false)
    at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:11509
#5  0x00007fffe8e47e99 in nsDocShell::DoURILoad (this=0x7fffc6485000, aURI=0x7fffc524f380, aOriginalURI=0x0, aLoadReplace=false, aReferrerURI=0x7fffc614c9b0, 
    aSendReferrer=true, aReferrerPolicy=0, aTriggeringPrincipal=0x7fffc8a18a80, aPrincipalToInherit=0x7fffc79fa120, aTypeHint=0x0, aFileName=..., aPostData=0x0, 
    aHeadersData=0x0, aFirstParty=true, aDocShell=0x0, aRequest=0x7fffffff8170, aIsNewWindowTarget=false, aBypassClassifier=false, aForceAllowCookies=false, 
    aSrcdoc=..., aBaseURI=0x0, aContentPolicyType=6) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:11323
#6  0x00007fffe8e456ef in nsDocShell::InternalLoad (this=0x7fffc6485000, aURI=0x7fffc524f380, aOriginalURI=0x0, aLoadReplace=false, aReferrer=0x7fffc614c9b0, 
    aReferrerPolicy=0, aTriggeringPrincipal=0x7fffc8a18a80, aPrincipalToInherit=0x7fffc79fa120, aFlags=0, aWindowTarget=..., aTypeHint=0x0, aFileName=..., 
    aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=true, aSrcdoc=..., aSourceDocShell=0x0, aBaseURI=0x0, aCheckForPrerender=false, 
    aDocShell=0x0, aRequest=0x0) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:10786
#7  0x00007fffe8e20902 in nsDocShell::LoadURI (this=0x7fffc6485000, aURI=0x7fffc524f380, aLoadInfo=0x7fffc4e67c10, aLoadFlags=262144, aFirstParty=true)
    at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:1586
#8  0x00007fffe8e2cf26 in nsDocShell::LoadURIWithOptions (this=0x7fffc6485000, aURI=0x7fffcab3c220 u"blob:null/8766baa3-d41b-4eff-b112-7109a967a8df", aLoadFlags=0, 
    aReferringURI=0x7fffc614c9b0, aReferrerPolicy=0, aPostStream=0x0, aHeaderStream=0x0, aBaseURI=0x0, aTriggeringPrincipal=0x0)
    at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:4860
#9  0x00007fffe38aa03a in NS_InvokeByIndex () at /home/ckerschb/source/mc/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:88
#10 0x00007fffe48992d3 in CallMethodHelper::Invoke (this=0x7fffffff8a30) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:2010
#11 0x00007fffe4896d91 in CallMethodHelper::Call (this=0x7fffffff8a30) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1329
#12 0x00007fffe487c0af in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
    at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1296
#13 0x00007fffe4885541 in XPC_WN_CallMethod (cx=0x7fffdbcff000, argc=8, vp=0x7fffd7c8b5e8) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:983
#14 0x00007fffe97100c8 in js::CallJSNative (cx=0x7fffdbcff000, native=0x7fffe48851ed <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:282
#15 0x00007fffe96eb963 in js::InternalCallOrConstruct (cx=0x7fffdbcff000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:448
#16 0x00007fffe96ebcdc in InternalCall (cx=0x7fffdbcff000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:493
#17 0x00007fffe96ebd1a in js::CallFromStack (cx=0x7fffdbcff000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:499
#18 0x00007fffe96f9a84 in Interpret (cx=0x7fffdbcff000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:2944
#19 0x00007fffe96eb565 in js::RunScript (cx=0x7fffdbcff000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:394
#20 0x00007fffe96eba41 in js::InternalCallOrConstruct (cx=0x7fffdbcff000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:466
#21 0x00007fffe96ebcdc in InternalCall (cx=0x7fffdbcff000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:493
#22 0x00007fffe96ebd96 in js::Call (cx=0x7fffdbcff000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:512
#23 0x00007fffe9cc6a6d in js::fun_call (cx=0x7fffdbcff000, argc=2, vp=0x7fffffff9c58) at /home/ckerschb/source/mc/js/src/jsfun.cpp:1177
#24 0x00007fffe97100c8 in js::CallJSNative (cx=0x7fffdbcff000, native=0x7fffe9cc686e <js::fun_call(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:282
#25 0x00007fffe96eb963 in js::InternalCallOrConstruct (cx=0x7fffdbcff000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:448
#26 0x00007fffe96ebcdc in InternalCall (cx=0x7fffdbcff000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:493
#27 0x00007fffe96ebd96 in js::Call (cx=0x7fffdbcff000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:512
#28 0x00007fffe9de35eb in js::Wrapper::call (this=0x7fffed6950f0 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffdbcff000, proxy=..., args=...)
---Type <return> to continue, or q <return> to quit---
    at /home/ckerschb/source/mc/js/src/proxy/Wrapper.cpp:165
#29 0x00007fffe9dbb7b9 in js::CrossCompartmentWrapper::call (this=0x7fffed6950f0 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffdbcff000, wrapper=..., args=...)
    at /home/ckerschb/source/mc/js/src/proxy/CrossCompartmentWrapper.cpp:351
#30 0x00007fffe9dd96b5 in js::Proxy::call (cx=0x7fffdbcff000, proxy=..., args=...) at /home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:464
#31 0x00007fffe9dda777 in js::proxy_Call (cx=0x7fffdbcff000, argc=2, vp=0x7fffffffa278) at /home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:716
#32 0x00007fffe97100c8 in js::CallJSNative (cx=0x7fffdbcff000, native=0x7fffe9dda69a <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:282
#33 0x00007fffe96eb81d in js::InternalCallOrConstruct (cx=0x7fffdbcff000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:436
#34 0x00007fffe96ebcdc in InternalCall (cx=0x7fffdbcff000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:493
#35 0x00007fffe96ebd1a in js::CallFromStack (cx=0x7fffdbcff000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:499
#36 0x00007fffe981ae29 in js::jit::DoCallFallback (cx=0x7fffdbcff000, frame=0x7fffffffa2f8, stub_=0x7fffc8f28628, argc=2, vp=0x7fffffffa278, res=...)
    at /home/ckerschb/source/mc/js/src/jit/BaselineIC.cpp:2340

(gdb) print DumpJSStack()
[Thread 0x7fffbdefb700 (LWP 2120) exited]
0 _loadURIWithFlags(browser = [object XULElement], uri = "blob:null/8766baa3-d41b-4eff-b112-7109a967a8df", params = [object Object]) ["chrome://browser/content/browser.js":991]
    this = [object ChromeWindow]
1 loadURIWithFlags(aURI = "blob:null/8766baa3-d41b-4eff-b112-7109a967a8df", aFlags = [object Object]) ["chrome://browser/content/tabbrowser.xml":7678]
    this = [object XULElement]
2 openLinkIn(url = "blob:null/8766baa3-d41b-4eff-b112-7109a967a8df", where = "current", params = [object Object]) ["chrome://browser/content/utilityOverlay.js":380]
    this = [object ChromeWindow]
3 openUILinkIn(url = "blob:null/8766baa3-d41b-4eff-b112-7109a967a8df", where = "current", aAllowThirdPartyFixup = [object Object]) ["chrome://browser/content/utilityOverlay.js":194]
    this = [object ChromeWindow]
4 openUILink(url = "blob:null/8766baa3-d41b-4eff-b112-7109a967a8df", event = [object XULCommandEvent], aIgnoreButton = undefined) ["chrome://browser/content/utilityOverlay.js":99]
    this = [object ChromeWindow]
5 viewMedia/<(blobURL = "blob:null/8766baa3-d41b-4eff-b112-7109a967a8df") ["chrome://browser/content/nsContextMenu.js":1131]
    this = [object ChromeWindow]
6 process() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":922]
    this = [object Object]
7 walkerLoop() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":806]
8 bound walkerLoop() ["self-hosted":916]
    this = [object Object]
9 bound bound walkerLoop() ["self-hosted":916]
    this = null
10 scheduleWalkerLoop/<(undefined) ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":742]

$1 = void

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Gijs, I think in order to fix this particular problem we should explicitly pass a principal within canvasToBlobURL - in this case a SystemPrincipal. Given that this patch also needs to be uplifted I think that's a reasonable approach. Ultimately we should try to fix Bug 1316275 however. What do you think?

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8842122 [details] [diff] [review]
bug_1343279_view_image_canvas.patch

Review of attachment 8842122 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1132,5 @@
>      if (this.onCanvas) {
>        this._canvasToBlobURL(this.target).then(function(blobURL) {
>          openUILink(blobURL, e, { disallowInheritPrincipal: true,
> +                                 referrerURI: referrerURI,
> +                                 originPrincipal: systemPrincipal});

No, I don't think this is right. Why can't this use the principal of the content embedding the canvas element? The blob URL should be issued for that content (which is something else to check! That, and when do we revoke it?).

This also looks like it would use the wrong origin attributes (so it breaks out of a container).

Generally, I'm a bit miffed seeing this because when I looked at the patch in bug 1331686 I should have noticed that you were reusing originPrincipal as triggeringPrincipal, which I think might not always be right.

originPrincipal was added in bug 1284395. I've CC'd you. Feel free to ping me on IRC, but re-reading the bugs here, and from my understanding of triggeringPrincipal, maybe openUILink(In) needs its own triggeringPrincipal parameter.

Comment 5

[Marcia Knous [:marcia]]

Tracking 53/54+ for this DOM Security regression.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Gijs, I don't know how to proceed with this bug. I agree that we shouldn't reuse originPrincipal as the triggeringPrincipal, hence I filed [1] to follow up on that (but that seems like a separate issue).

Currently, the triggeringPrincipal is 'http://html5demos.com/canvas', which causes security checks to fail. In particular, http://html5demos.com/canvas is not allowed to load 'blob:null/06cb3f43-d724-495c-87c6-2283e1076c4d'. The checks originate from the contentSecurityManager, which consults CheckLoadURIWithPrincipal [2] which then consults CheckMayLoad [3] which then ultimately fails the security check [4].

In general, I think 'right-click -> view-image' feels like a top level load, hence I am not sure if we shouldn't use the systemPrincipal as the triggeringPrincipal in that case.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1344706
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#110
[3] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#683
[4] https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#446

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> In general, I think 'right-click -> view-image' feels like a top level load,
> hence I am not sure if we shouldn't use the systemPrincipal as the
> triggeringPrincipal in that case.

If there is no other way to make this work, and we're guaranteed this path is only from user interaction, then I guess the system principal option works. That said, we still need to fix the userContextId issue (and maybe private browsing state if we're still passing that separately?) so we pass that correctly.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If there is no other way to make this work

The basic issue is that the way blobs work is that only the principal used for creating a blob: (or a principal that subsumes it) can link to it.

I think fundamentally we have three options here:

1)  Continue creating the blob with the system principal.  Then we need to do the load with the system principal too.

2)  Create the blob with the page principal.  Then we need to fail this entire operation for a tainted canvas, because otherwise the page could get at the blob.  This seems undesirable.

3)  Create the blob with some new principal we synthesize.  For example, we can create a nullprincipal with the same origin attributes as the page, create a sandbox with that principal, and create the blob url in there.  Then use that principal for the load.

One problem with option 3 is that the blob url will get revoked when we kill off the sandbox (I hope!).  On the other hand, at the moment that blob url and its corresponding blob live for the lifetime of whatever global browser/base/content/content.js runs in, which may not be great either.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I guess the point is we could explicitly keep the sandbox alive while the content.js global is alive, and get our existing behavior wrt lifetime.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Gijs, what do you think?

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Gijs, what do you think?

I think if possible I'd like to go with option #3 from comment #8, using comment #9 to ensure the blob stays alive long enough. Given that 53 has just merged to beta and we need this fixed there, if #3 is very difficult to implement in practice (I don't think so, but...) I would also be OK with implementing and uplifting #1 first, and then trying to get #3 done.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Boris, Gijs,

I am happy to try to resolve the problem by trying to implement approach (3) of comment 8. At the moment I am having a hard time following what the proposed plan is. Starting at browser/base [1,2] I don't know where I would create that NullPrincipal. Assuming in [1] and then I pass the new NullPrincipal to [2]. But then how would I create the new blob using that principal? Would that mean hacking up the HTMLCanvasElement [3] and also the ImageEncoder [4], because I suppose it's there where we actually create the blob or am I completely wrong?

If any of you could provide some pointers I am happy to dig in and see if we can get that done, but I would like to understand how first - thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1121
[2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#831
[3] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#824
[4] https://dxr.mozilla.org/mozilla-central/source/dom/base/ImageEncoder.cpp#298

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I don't know where I would create that NullPrincipal.

Well, in the end you need it both places: in content.js and in nsContextMenu.js, right?  In particular, you need it in the place where you create the blob, and you need it at the place where you do the load.  And it needs to be the same principal both places.  If sending nullprincipal over the message manager is not possible, then we can't really use it here.  In that case, maybe an expanded principal containing the page principal would work?

In any case, you create it in one or the other place, and you ship it to whatever place you didn't create it at.

> But then how would I create the new blob using that principal?

You create a sandbox using that principal; see Components.utils.Sandbox.  Then you call the sandbox's URL.createObjectURL on your blob.  That is, replace this bit in content.js:

  message.objects.target.toBlob((blob) => {
    let blobURL = URL.createObjectURL(blob);

with:

  message.objects.target.toBlob((blob) => {
    let blobURL = Components.utils.evalInSandbox("URL", sandbox).createObjectURL(blob);

or so.  I'm pretty sure createObjectURL over Xrays (as here) will create the blob URL in the global the createObjectURL function came from, not the caller global.

> Would that mean hacking up the HTMLCanvasElement

No.

> and also the ImageEncoder

Absolutely not.

> because I suppose it's there where we actually create the blob

What matters is where we create the blob url.

Comment 14

[:Gijs (out for now; he/him)]

I think bz answered the question. :-)

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Gijs, in theory the attached patch should work and do the trick; but I am getting the following error when trying to call |Cu.evalInSandbox("URL", sandbox).createObjectURL(blob)|

> JavaScript error: chrome://browser/content/content.js, line 837: ReferenceError: URL is not defined

Am I missing something in general or is that the right approach and something is just not working properly?

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> Created attachment 8845380 [details] [diff] [review]
> bug_1343279_view_image_canvas.patch
> 
> Gijs, in theory the attached patch should work and do the trick; but I am
> getting the following error when trying to call |Cu.evalInSandbox("URL",
> sandbox).createObjectURL(blob)|
> 
> > JavaScript error: chrome://browser/content/content.js, line 837: ReferenceError: URL is not defined
> 
> Am I missing something in general or is that the right approach and
> something is just not working properly?

I'm about to head out so I don't have a lot of time to look at the approach this second, but the referenceerror is because the sandbox doesn't have the URL global by default - see the 'wantGlobalProperties' flag in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.Sandbox#options to turn it on. Hopefully this unblocks you checking it works. :-)

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Gijs, initially you suggested using the actual principal as the triggeringPrincipal. I thought about it again, and I think it's actually better to use the actual principal and create the blob with that principal than using a NullPrincipal with the OA from the actual principal, right?

I manually verified that the problem is fixed as well as tested that saveMedia() works as well.

With this patch applied we perform security checks in asyncOpen2() with the following arguments:

doContentSecurityCheck {
  channelURI: blob:http://html5demos.com/a537fecd-f0d0-41f3-a48a-882d6fd3d93b
  loadingPrincipal: nullptr
  triggeringPrincipal: http://html5demos.com/canvas
  principalToInherit: NullPrincipal
  contentPolicyType: 6
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 
  initalSecurityChecksDone: no
  enforceSecurity: no
}

In my opinion having 'http://html5demos.com/canvas' as the triggeringPrincipal is actually better than having a nullprincipal. Agreed?

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8845380 [details] [diff] [review]
bug_1343279_view_image_canvas.patch

Besides my earlier point this one looks OK...

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8845418 [details] [diff] [review]
bug_1343279_view_image_canvas.patch

We can't do this because of what bz said earlier:

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #8)
> 2)  Create the blob with the page principal.  Then we need to fail this
> entire operation for a tainted canvas, because otherwise the page could get
> at the blob.  This seems undesirable.


Note that in the other patch, we still need to work out when to destroy the sandbox and thereby (presumably) the blob URL.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Boris, Gijs, I am making progress, but I am still having some trouble. Please note that using this.principal (which is an http:// principal) works and successfully views the image.

Now, the serialization part of a nullPrincipal works as well (I verified that); but then the security check CheckLoadURIWithPrincipal fails; in particular, ::CheckMayLoad() which ultimately fails here [1]. I have never worked with blobs that much, but given that the URI of the nullPrincipal is some moz-nullprincipal:{356ab095-a1ae-4ba6-a22c-1df0f260f702}, I would imagine that the blob should also look something like
> blob:moz-nullprincipal:{356ab095-a1ae-4ba6-a22c-1df0f260f702}/...
but it actually looks more like
> blob:null/522be56b-e74c-424d-bceb-fbe5e6417828.
is that correct? Or is something wrong with our sandbox which needs to be debugged?

The other question I have is, where should we actually destroy the sandbox?

Thanks for your help!

[1] https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#

Comment 21

[Boris Zbarsky [:bzbarsky]]

> is that correct? 

Yes.  The serialization off a nullprincipal is "null" per spec, and that's what goes in the blob url.

But the important question is what principal the actual blob identified by the uuid has.

> :CheckMayLoad() which ultimately fails here [1]

No line number, so hard to say what's going on.  But anyway, in nsNullPrincipal::MayLoadInternal what is the "principal" on aURI?  If it's not pointer-identical to "this", then that is the problem that needs fixing.  We need the _same_ nullprincipal, not just a nullprincipal with the same URI.  If that part doesn't work over message managers, then we can't use a nullprincipal.  We might be able to use an expanded principal containing only the page principal, though...  I'm not sure that works either, actually, looking at nsExpandedPrincipal::MayLoadInternal, but if it does not we should just fix it to work, imo.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #21)
> No line number, so hard to say what's going on.

Sorry about that, ultimately we return the failure here:
https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#424

> But anyway, in
> nsNullPrincipal::MayLoadInternal what is the "principal" on aURI?  If it's
> not pointer-identical to "this", then that is the problem that needs fixing.
> We need the _same_ nullprincipal, not just a nullprincipal with the same
> URI.  If that part doesn't work over message managers, then we can't use a
> nullprincipal.  We might be able to use an expanded principal containing
> only the page principal, though...  I'm not sure that works either,
> actually, looking at nsExpandedPrincipal::MayLoadInternal, but if it does
> not we should just fix it to work, imo.

But anyway, you pointed right to the problem. We create the blob using the following principal:
> moz-nullprincipal:{70a16eec-2106-4dfe-93ac-def7518d7c4f}
and then within nsNullPrincipal::MayLoadInternal() the URI of the principal is
> moz-nullprincipal:{70a16eec-2106-4dfe-93ac-def7518d7c4f}
but the pointers of the principals are not identical here [2] and hence the check fails.
I'll try to get it working using an expanded principal.

[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsNullPrincipal.cpp#146

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Boris, I am only making baby-steps here. According to [1] one can create an expanded principal just by putting a principal into []. A search through DXR was not very helpful either and so I am wondering if that is even the right way to create an expanded principal in JS-land. It seems to work though given the following info when I print the newly created expandedPrincipal:
> [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0x7fffcaac2ec0 (native @ 0x7fffbe9b43e0)]

Anyway, when I try to serialize that expanded principal I encounter an error (see stacktrace underneath) within nsBinaryStream::WriteCompundObject() [2] which complains that the object has no classinfo or is not serializeable.

Looking at the implementation of nsExpandedPrincipal [3] it seems that the expanded principal provides the required information about classinfo [3] and serialization [4] though.

Am I missing something?


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.Sandbox#Expanded_principal
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsBinaryStream.cpp#295
[3] https://dxr.mozilla.org/mozilla-central/source/caps/nsExpandedPrincipal.cpp#12
[4] https://dxr.mozilla.org/mozilla-central/source/caps/nsExpandedPrincipal.h#27

%--- snip ---

#0  0x00007fffe3847556 in nsBinaryOutputStream::WriteCompoundObject (this=0x7fffbe563fa0, aObject=0x7fffcabe39e0, aIID=..., aIsStrongRef=true)
    at /home/ckerschb/source/mc/xpcom/io/nsBinaryStream.cpp:303
#1  0x00007fffe3a1194c in NS_SerializeToString (obj=0x7fffcabe39e0, str=...) at /home/ckerschb/source/mc/netwerk/base/nsSerializationHelper.cpp:34
#2  0x00007fffe3a11f4d in nsSerializationHelper::SerializeToString (this=0x7fffbfe80fe0, serializable=0x7fffcabe39e0, _retval=...)
    at /home/ckerschb/source/mc/netwerk/base/nsSerializationHelper.cpp:65
#3  0x00007fffe38d23ba in NS_InvokeByIndex () at /home/ckerschb/source/mc/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:88
#4  0x00007fffe48979d5 in CallMethodHelper::Invoke (this=0x7fffffff7d80) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:2010
#5  0x00007fffe4895493 in CallMethodHelper::Call (this=0x7fffffff7d80) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1329
#6  0x00007fffe487a7b9 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
    at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1296
#7  0x00007fffe4883c4b in XPC_WN_CallMethod (cx=0x7fffdbc34000, argc=1, vp=0x7fffffff8078) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:983
#8  0x00002e6461a3924f in ?? ()
#9  0x00007fffffff8060 in ?? ()
#10 0x00007fffffff8050 in ?? ()
#11 0x0000000000000000 in ?? ()
(gdb) print DumpJSStack()
0 serializePrincipal(principal = [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0x7fffcaac2ec0 (native @ 0x7fffbe9b43e0)]) ["resource://gre/modules/sessionstore/Utils.jsm":125]
    this = [object Object]
1 _canvasToBlobURL/<(resolve = [function], [function]) ["chrome://browser/content/nsContextMenu.js":1129]
    this = [object ChromeWindow]
2 Promise(aExecutor = function(resolve) {
      mm.sendAsyncMessage("ContextMenu:Canvas:ToBlobURL", {}, {
        target,
        triggeringPrincipal: Utils.serializePrincipal(triggeringPrincipal)
      });

      let onMessage = (message) => {
        mm.removeMessageListener("ContextMenu:Canvas:ToBlobURL:Result", onMessage);
        resolve(message.data.blobURL);
      };
      mm.addMessageListener("ContextMenu:Canvas:ToBlobURL:Result", onMessage);
    }) ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":390]
    this = [object Object]
3 _canvasToBlobURL(    <Failed to get argument while inspecting stack frame>
triggeringPrincipal = [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0x7fffcaac2ec0 (native @ 0x7fffbe9b43e0)]) ["chrome://browser/content/nsContextMenu.js":1126]
    <failed to get 'this' value>
4 viewMedia(e = [object XULCommandEvent]) ["chrome://browser/content/nsContextMenu.js":1148]
    <failed to get 'this' value>
5 oncommand(event = [object XULCommandEvent]) ["chrome://browser/content/browser.xul":1]
    this = [object XULElement]

$1 = void

Comment 24

[Boris Zbarsky [:bzbarsky]]

> so I am wondering if that is even the right way to create an expanded principal in JS-land.

Well, it's the right way to create a sandbox whose principal is an expanded principal, normally.

I don't see any other JS-exposed API for creating expanded principals offhand.

It's not clear to me whether the principal of a sandbox can be extracted...

> and serialization [4] though

It doesn't actually list nsISerializable in its QI or classinfo.  And its Read() and Write() are not implemented.  Sounds like it's not serializable.

It's worth checking with bholley how he thinks this should work, exactly.  Seems to me that to make this work we need to either use the system principal for everything or have some other non-page principal that we can correctly roundtrip across the message manager, and I'm not seeing such a thing at the moment.

Comment 25

[Bobby Holley (:bholley)]

The normal hack to create an explicit expanded principal is [1]. It's a bit wasteful because it creates a temporary global. If we want to use this in production code that's anywhere close to hot, we should probably add an API to nsIScriptSecurityManager to do this.

Expanded principals are serializable IIUC.

[1] http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/caps/tests/unit/test_origin.js#187

Comment 26

[Boris Zbarsky [:bzbarsky]]

> It's a bit wasteful because it creates a temporary global.

We need that global too, so that's not a problem.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Bobby, besides what we are trying to fix within this bug. Should we 'only' do a pointer comparision to check if NullPrincipals are identical? It seems that when serializing a NullPrincipal we might have that problem elsewhere as well, right? I suppose for SystemPrincipals that is not an issue because we use the same systemPrincipal pointer on both ends.

Within this patch I added a dummy version which performs a string comparison which fixes the problem obviously. I further identified that we would have to update FastEquals() to account for that and obviously we would have to account for origin attributes.

What is the reason we only perform a pointer comparison? Is it performance or is there some other reason I don't know about?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> What is the reason we only perform a pointer comparison?

As originally designed, nullprincipals were a nonce principal.  They were given a URI because we had to, and we kinda tried to make the URI unguessable because we were afraid people would use that URI for stuff, but we didn't want to rely on it being unguessable.  Not least because we were definitely not generating it using a cryptographically secure prng or anything.

Comment 29

[Bobby Holley (:bholley)]

given that we're moving towards being able to serialize principals into strings and back again (with the infallible GetOrigin work), I think we probably want to fix things to make this work with null principals. The performance issue isn't a concern because we now atomize URIs and compare those.

I think we should consult with somebody who knows what they're talking about wrt secure rngs, and get their opinion on how secure ours needs to be here. I don't think we _necessarily_ rely on them being unguessable, but I haven't thought about the problem enough to be sure.

Once we do that, it should be relatively straightforward to compare nullprincipals based on their atomized URI (like we do with codebase principals), which would solve the problem here.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1343279_view_image_canvas.patch

Gijs, given the latest discussions within comment 27, 28, and 29 I suppose we should get this bug fixed by using the systemPrincipal as a temporary (upliftable) measure.

I will file a follow up bug which can include the discussion about secure prngs to generate a NullPrincipal and also the string comparision rather than the pointer comparision when checking for equality of NullPrincipals. I don't think that change to NullPrincipals should be uplifted all the way to beta. Nevertheless we should get that fixed ASAP, but as I previously mentioned, not in this bug.

What do you think? Can we agree on that?

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08ec5181293b
Use SystemPrincipal when loading canvasToBlobURL. r=gijs

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> I will file a follow up bug which can include the discussion about secure
> prngs to generate a NullPrincipal and also the string comparision rather
> than the pointer comparision when checking for equality of NullPrincipals. I
> don't think that change to NullPrincipals should be uplifted all the way to
> beta. Nevertheless we should get that fixed ASAP, but as I previously
> mentioned, not in this bug.

I filed Bug 1346759 as a follow up so we can discuss prngs for NullPrincipal creations as well as not relying on the pointer comparison.

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8846544 [details] [diff] [review]
bug_1343279_view_image_canvas.patch

Approval Request Comment
After performing additional security checks for docshell loads within Bug 1182569 'view-image' loads for canvas became subject to an additional security check which blocked the load. We should get the changeset within this bug uplifted to make that work again on all branches.

[Feature/Bug causing the regression]:
Bug 1182569 - Use asyncOpen2() for docshell loads (nsJSProtocolHandler and nsURILoader)

[User impact if declined]:
Users performing 'view-image' on a canvas experience that the image gets blocked by security checks. 

[Is this code covered by automated tests?]:
No. It's complicated to write an automated test for that. I have manually verified using the STRs from comment 0 that it works correctly again.

[Has the fix been verified in Nightly?]:
No, not yet.

[Needs manual test from QE? If yes, steps to reproduce]:
Yes - I think it would be good if someone could perform the STRs from comment 0 to make sure everything works fine again.

[List of other uplifts needed for the feature/fix]:
---

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
It's not risky because it i) only affects canvas view-image loads. In fact we are making the code more accurate by explicitly providing a triggeringPrincipal where there was none before.

[String changes made/needed]:
No

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this need to land on m-c?

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> Does this need to land on m-c?

It needs to land on m-c. Just pushed to inbound 4 hours ago, see comment 31.

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08ec5181293b

Comment 37

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 38

[ovidiu boca[:Ovidiu]]

I tested this issue on Windows 10, Ubuntu 16.04 and Mac OS X 10.10 with the latest FF Nightly 55.0a1(2017-03-14) and I can confirm the fix, this issue in no longer reproducible.

Comment 39

[Julien Cristau [:jcristau]]

Comment on attachment 8846544 [details] [diff] [review]
bug_1343279_view_image_canvas.patch

fix canvas view-image in 53 and 54

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb622b3766d196df115303553738efdf12e5f3dd

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d47ea9e922d

Comment 42

[Andrei Vaida [:avaida]]

Let's make sure this works as intended on 53 as well.

Comment 43

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 54.0a1 (ID=20170228030203) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 53.0b5, 54.0a2 on Win 7 x64, Win 8.1 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.