+++ This bug was initially created as a clone of Bug #1133577 +++ STR: 1) Visit a site with some images on it in an e10s window 2) Right-click on an image, and choose "Set As Desktop Background..." This causes some "unsafe CPOW usage" warnings in the Browser Console, some TypeErrors, and if you then hit "Set Desktop Background" in the dialog window, you get a "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE". In browser/base/content/nsContextMenu.js: disableSetDesktopBackground: function() { // Disable the Set as Desktop Background menu item if we're still trying // to load the image or the load failed. if (!(this.target instanceof Ci.nsIImageLoadingContent)) <- Causes CPOW warning return true; if (("complete" in this.target) && !this.target.complete) <- Causes CPOW warning return true; if (this.target.currentURI.schemeIs("javascript")) <- Causes CPOW warning return true; var request = this.target <- Causes CPOW warning .QueryInterface(Ci.nsIImageLoadingContent) .getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST); if (!request) return true; return false; }, setDesktopBackground: function() { // Paranoia: check disableSetDesktopBackground again, in case the // image changed since the context menu was initiated. if (this.disableSetDesktopBackground()) return; var doc = this.target.ownerDocument; <- Causes CPOW warning urlSecurityCheck(this.target.currentURI.spec, this.principal); <- Causes CPOW warning // Confirm since it's annoying if you hit this accidentally. const kDesktopBackgroundURL = "chrome://browser/content/setDesktopBackground.xul"; // On non-Mac platforms, the Set Wallpaper dialog is modal. openDialog(kDesktopBackgroundURL, "", "centerscreen,chrome,dialog,modal,dependent", this.target); }, in browser/components/shell/content/setDesktopBackground.js "TypeError: Argument 1 of CanvasRenderingContext2D.drawImage could not be converted to any of: HTMLImageElement, HTMLCanvasElement, HTMLVideoElement." or in the TILE case "TypeError: Argument 1 of CanvasRenderingContext2D.createPattern could not be converted to any of: HTMLImageElement, HTMLCanvasElement, HTMLVideoElement." updatePosition: function () { var ctx = this._canvas.getContext("2d"); ctx.clearRect(0, 0, this._screenWidth, this._screenHeight); this._position = document.getElementById("menuPosition").value; switch (this._position) { case "TILE": ctx.save(); ctx.fillStyle = ctx.createPattern(this._image, "repeat"); <- Causes TypeError ctx.fillRect(0, 0, this._screenWidth, this._screenHeight); ctx.restore(); break; case "STRETCH": ctx.drawImage(this._image, 0, 0, this._screenWidth, this._screenHeight); <- Causes TypeError break; case "CENTER": var x = (this._screenWidth - this._image.naturalWidth) / 2; <- Causes CPOW warning var y = (this._screenHeight - this._image.naturalHeight) / 2; <- Causes CPOW warning ctx.drawImage(this._image, x, y); <- Causes TypeError break; case "FILL": //Try maxing width first, overflow height var widthRatio = this._screenWidth / this._image.naturalWidth; <- Causes CPOW warning var width = this._image.naturalWidth * widthRatio; <- Causes CPOW warning var height = this._image.naturalHeight * widthRatio; <- Causes CPOW warning if (height < this._screenHeight) { //height less than screen, max height and overflow width var heightRatio = this._screenHeight / this._image.naturalHeight; <- Causes CPOW warning width = this._image.naturalWidth * heightRatio; <- Causes CPOW warning height = this._image.naturalHeight * heightRatio; <- Causes CPOW warning } var x = (this._screenWidth - width) / 2; var y = (this._screenHeight - height) / 2; ctx.drawImage(this._image, x, y, width, height); <- Causes TypeError break; case "FIT": //Try maxing width first, top and bottom borders var widthRatio = this._screenWidth / this._image.naturalWidth; <- Causes CPOW warning var width = this._image.naturalWidth * widthRatio; <- Causes CPOW warning var height = this._image.naturalHeight * widthRatio; <- Causes CPOW warning var x = 0; var y = (this._screenHeight - height) / 2; if (height > this._screenHeight) { //height overflow, maximise height, side borders var heightRatio = this._screenHeight / this._image.naturalHeight; <- Causes CPOW warning width = this._image.naturalWidth * heightRatio; <- Causes CPOW warning height = this._image.naturalHeight * heightRatio; <- Causes CPOW warning x = (this._screenWidth - width) / 2; y = 0; } ctx.drawImage(this._image, x, y, width, height); <- Causes TypeError break; } } Actually hitting "Set Desktop Background" in the preview dialog fails with "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIShellService.setDesktopBackground]" setDesktopBackground: function () { document.persist("menuPosition", "value"); this._shell.desktopBackgroundColor = this._hexStringToLong(this._backgroundColor); this._shell.setDesktopBackground(this._image, <- Causes NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE Ci.nsIShellService["BACKGROUND_" + this._position]); },

/r/7097 - Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image URL and make a new element with it to pass to the setDesktopBackground dialog Pull down this commit: hg pull -r 1b17b7476a88314a3884b2ea36186ea39c926523 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/7095/#review5909 ::: browser/base/content/content.js (Diff revision 1) > +function disableSetDesktopBackground(aTarget) { This duplication of the function from nsContextMenu is unfortunate, but I don't know of a better solution. ::: browser/base/content/content.js (Diff revision 1) > + Cu.reportError(e); Don't know if this is necessary.

Comment on attachment 8593176 [details] MozReview Request: bz://1135933/Kwan Clearing review since after some discussion with jimm on IRC it seems this might not be the best approach security-wise, as I'd feared.

Actually I don't see any easy way to do this right at first glance. The problem is that the original version just passed the content image to the dialog window with the arguments property, and then the dialog window uses it for various things a CPOW could not be ever used for (http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/content/setDesktopBackground.js#167) Now the dialog window if I assume it right runs in the parent process (because of xul) where we don't want to load an image from some random content URI, I'm not sure what we should do here... I think even if dialog window had remote support (have they?) it can be hard to open a dialog window whose child runs in the same process as the content (or do we have that magic?)... If we could that would be a fix for this, but then we still needed to find a way to pass this image argument... Eh... Have we faced any similar issue before? Am I missing an obvious solution here?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4) > Actually I don't see any easy way to do this right at first glance. The > problem is that the original version just passed the content image to the > dialog window with the arguments property, and then the dialog window uses > it for various things a CPOW could not be ever used for > (http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/ > content/setDesktopBackground.js#167) > > Now the dialog window if I assume it right runs in the parent process > (because of xul) where we don't want to load an image from some random > content URI, I'm not sure what we should do here... > > I think even if dialog window had remote support (have they?) it can be hard > to open a dialog window whose child runs in the same process as the content > (or do we have that magic?)... At the browser chrome level, no, we do not yet have that magic. We're definitely going to need that magic once was start supporting dom.ipc.processCount > 1. In the meantime, however, we're OK assuming that the content process is being shared. > If we could that would be a fix for this, but > then we still needed to find a way to pass this image argument... Eh... Have > we faced any similar issue before? Am I missing an obvious solution here? Would it be advisable to convert the image from web content into a data URI, and then pass that to the XUL dialog instead?

jimm advised getting some advice from sstamm about the security aspects.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5) > Would it be advisable to convert the image from web content into a data URI, > and then pass that to the XUL dialog instead? I like the idea. We would still have to copy all the attributes probably and I hope we can just ignore styles...

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #5) > > Would it be advisable to convert the image from web content into a data URI, > > and then pass that to the XUL dialog instead? > > I like the idea. We would still have to copy all the attributes probably and > I hope we > can just ignore styles... I don't think any of the attributes matter, do they? All that's needed is the naturalWidth/Height properties, and they can be used to set the dimensions of the canvas the image is drawn to and then toDataURL()ed from (or does Mozilla have a quicker way of converting an image?). Using that approach building on top of my previous patch I've already got it up and working.

(In reply to Ian Moody [:Kwan] from comment #8) > I don't think any of the attributes matter, do they? All that's needed is > the naturalWidth/Height properties You're right, that's all what we need indeed. > (or does Mozilla have a quicker way of converting an image?). Not that I'm aware of. Also, I wouldn't worry too much about the performance of the conversion here anyway...

/r/7333 - Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog Pull down this commit: hg pull -r e92df83356c301cac7295983881d7491d3b4772e https://reviewboard-hg.mozilla.org/gecko/

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9) > (In reply to Ian Moody [:Kwan] from comment #8) > > (or does Mozilla have a quicker way of converting an image?). > > Not that I'm aware of. Also, I wouldn't worry too much about > the performance of the conversion here anyway... Heh, I phrased it poorly, but I was referring to the code verbosity of creating a canvas and drawing an image to it, as opposed to a toDataURL() directly on the image. Here's the patch anyway. Question for sstamm now is is this safe security wise, or is it still susceptible to imagelib vulnerabilities in the parent process?

(In reply to Ian Moody [:Kwan] from comment #11) > Here's the patch anyway. Question for sstamm now is is this safe security > wise, or is it still susceptible to imagelib vulnerabilities in the parent > process? I'm afraid I don't have enough background (or time) here to be super-helpful without some context. What specifically are you worried about?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12) > (In reply to Ian Moody [:Kwan] from comment #11) > > Here's the patch anyway. Question for sstamm now is is this safe security > > wise, or is it still susceptible to imagelib vulnerabilities in the parent > > process? > > I'm afraid I don't have enough background (or time) here to be super-helpful > without some context. What specifically are you worried about? Ah, sorry for the lack of context. So currently to set the background image the image element itself is passed. That isn't safe in e10s since the <img/> is in a separate process. My first approach for fixing this was to pass up the image URL to the parent process and create a new image element there and set its src. However jimm informed me on IRC that this isn't safe, since if the image triggers a vulnerability in the image lib it'll be in the parent process with greater privileges. The new approach, suggested by mconley, is to draw the image to a <canvas/>, then convert that <canvas/> to a data URL, and pass that up to the parent process and create a new <img> with said data URL as its src. The question is is that safe (since the passed data has been created by Firefox based on a <canvas/>), or is it still susceptible to imglib vulnerabilities? If it's not safe, do you have any other suggestions for safely getting image data based on a web image into the parent process and using it? Trying to set the background image in the content process doesn't seem like it's viable, since I tink that'll be outside its privileges, and there is also the issue of the preview canvas.

Not seeing any reviewers on this stuff - Ian, is this ready for review?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #14) > Not seeing any reviewers on this stuff - Ian, is this ready for review? I probably should have needinfo'd Sid on comment 13, to see if he has an opinion on if this is safe. If it is then it's good to go for review.

I'm not super familiar with how canvas is created and what kinds of protections between imglib and the parent process might exist, or what the differences are in our image decoding. I've sent out a request for more feedback on this bug from a wider group of security folks. But with that disclaimer, let me see if I understand this right: * in the child process we use the "risky" decoding routine in imglib to load and convert the image from URL to some base64-encoded data: URI thing that doesn't require using imglib for decoding or rendering. * we pass the data: URI to the parent * the parent decodes and renders the data: URI (not using imglib). * We trust the code that does any decoding of a data: URI image (not using imglib). Is this correct? If we only use imglib in the child, the parent should be safe from imglib problems. (On the other hand, if we do any calls into imglib in the parent, then we just moved the problems around.)

(In reply to Ian Moody [:Kwan] from comment #13) > The new approach, suggested by mconley, is to draw the image to a <canvas/>, > then convert that <canvas/> to a data URL, and pass that up to the parent > process and create a new <img> with said data URL as its src. The question > is is that safe (since the passed data has been created by Firefox based on > a <canvas/>), or is it still susceptible to imglib vulnerabilities? If the goal is to avoid invoking ImageLib, this doesn't achieve that goal. You're taking a different Necko code path, but the ImageLib code that gets executed is exactly the same. However, since you're re-encoding the image, at least you know that it has been encoded by our own encoder, rather than a potentially malicious encoder. So there is definitely a gain in safety. I also don't know of a better approach that works from JS. =\ (And I generally don't know much about E10S.) (In reply to Sid Stamm [:geekboy or :sstamm] from comment #16) > * in the child process we use the "risky" decoding routine in imglib to load > and convert the image from URL to some base64-encoded data: URI thing that > doesn't require using imglib for decoding or rendering. > * we pass the data: URI to the parent > * the parent decodes and renders the data: URI (not using imglib). > * We trust the code that does any decoding of a data: URI image (not using > imglib). Just to be sure I've been clear: this is incorrect. ImageLib is used to decode the data URI in the parent.

Do we need to decode it again in the parent? If we're setting the desktop background don't we just pass the image itself to the OS and let the OS decode it? (That, too, carries risk.) Either way, if we have re-encoded the image ourselves from a canvas I'm much less worried about this being exploitable.

(In reply to Seth Fowler [:seth] from comment #17)> > However, since you're re-encoding the image, at least you know that it has > been encoded by our own encoder, rather than a potentially malicious > encoder. So there is definitely a gain in safety Yes, this was the goal of this approach. (In reply to Daniel Veditz [:dveditz] from comment #18) > Do we need to decode it again in the parent? If we're setting the desktop > background don't we just pass the image itself to the OS and let the OS > decode it? (That, too, carries risk.) We can't pass the image itself to the shell service when the shell service is invoked in the parent since the native code can't take a CPOW (hence the NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE), and I don't believe setting it from the content process will be allowed due to the reduced privileges it will have, so I think we do need an image element in the parent process.

Comment on attachment 8594879 [details] MozReview Request: bz://1135933/Kwan /r/9365 - Bug 1135933 - [Review only commit] Move nsContextMenu.disableSetDesktopBackground() to content.js /r/7333 - Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog Pull down these commits: hg pull -r 3a69c0ec3a2a8e90ea851632a9cece442107831e https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/7333/#review8317 Looks nice! ::: browser/base/content/content.js:692 (Diff revision 2) > + let disable = false; > + > + // Paranoia: check disableSetDesktopBackground again, in case the > + // image changed since the context menu was initiated. > + if (disableSetDesktopBackground(target)) { > + disable = true; > + } else { > + try { How about: let disabled = disableSetDesktopBackground(target); if (!disabled) { try ... } Also I kind of find disableSetDesktopBackground a missleading name, since it does not seem to disable anything... isSetDesktopBackgroundDisabled would make more sense to me, but that's kind of unrelated to this patch so nevermind. ::: browser/base/content/nsContextMenu.js:1164 (Diff revision 2) > - var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"] > - .getService(Components.interfaces.nsIWindowMediator); > + const wm = Cc["@mozilla.org/appshell/window-mediator;1"]. > + getService(Ci.nsIWindowMediator); I think the . and the indentation is off here.

https://reviewboard.mozilla.org/r/9365/#review8321 Thanks for this part!

https://reviewboard.mozilla.org/r/7333/#review8363 > How about: > let disabled = disableSetDesktopBackground(target); > if (!disabled) { > try ... > } > > Also I kind of find disableSetDesktopBackground a missleading name, since it does not seem to disable anything... isSetDesktopBackgroundDisabled would make more sense to me, but that's kind of unrelated to this patch so nevermind. Ah, that's nice and concise, thanks (though I stuck with "disable" rather than "disabled"). > I think the . and the indentation is off here. I agree it's weird, but it's also the style used consistently (!) for Cc[] throughout the rest of nsContextMenu.js, so I'm going to leave as is.

Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r=gabor Also perform the disableSetDesktopBackground() check in content

Comment on attachment 8612541 [details] MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r=gabor Also perform the disableSetDesktopBackground() check in content

Review-only commit folded and comment addressed, and patch rebased to tip. Green try is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf4046ce42c with two intermittents from bug 1081925 and bug 1167579 It is for the pre-review patch but given the insignificance of the change (https://reviewboard.mozilla.org/r/7331/diff/2-4/) I don't see a need for a new run. I presume the lack of updated hash posting it to do with the switch to separate attachments for each reviewboard commit, so here it is Pull down this commit: hg pull -r 2755ecafebb4d83dced05dccb79d26af8b6b90ce https://reviewboard-hg.mozilla.org/gecko/

Backed out for making browser_addKeywordSearch.js permafail. https://treeherder.mozilla.org/logviewer.html#?job_id=3265846&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/ca5480ad6ffb

Hey Kwan - any chance you'll have time to figure out what's going wrong in browser_addKeywordSearch.js, or would you like to hand it off?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30) > Hey Kwan - any chance you'll have time to figure out what's going wrong in > browser_addKeywordSearch.js, or would you like to hand it off? Yes! Sorry I haven't been more responsive, unfortunately I've been lacking in free time, but I will be able to get to it tomorrow.

So the problem is a rather simple one, and revealed by the "TypeError: aTarget.currentURI is null" in the test logs. When I replaced the menu init call to disableSetDesktopBackground() so I could move the function to content rather than just copy it for the paranoid check, I failed to notice something rather important: It's called inside "if (haveSetDesktopBackground && this.onLoadedImage) {}", which implies a bunch of things (https://hg.mozilla.org/mozilla-central/file/e10e2e8d8bf2/browser/base/content/nsContextMenu.js#l640), namely if (this.target.nodeType == Node.ELEMENT_NODE) { if (this.target instanceof Ci.nsIImageLoadingContent && this.target.currentURI) { So when the function is called in content.js on an <input/> is fails at 'aTarget.currentURI.schemeIs("javascript")' and the menu never opens. There were no test failures on try because it seems no test tries to open the context menu on an <input/>, but when I rewrote browser_addKeywordSearch.js I made it do so, so that's why it now fails. There is already an equivalent if check nearby for the media cache stuff, so moving the call inside that is enough to fix this, I believe.

Comment on attachment 8612541 [details] MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor Also perform the disableSetDesktopBackground() check in content

(In reply to Ian Moody [:Kwan] from comment #35) > There is already an equivalent if check nearby for the media cache stuff, so > moving the call inside that is enough to fix this, I believe. Thanks for looking into this, I guess I should have spotted this one... Anyway, a few things I don't quite get in the fix. Why do you set the initial value to null and not false? (I can't find any difference between how those two values are handled here...) I feel like disableSetDesktopBackground should just handle the case where currentURI is null. But do we really want to enable set bg in that case? I think currentURI is null if there is no current image request and there weren't any images loaded either so I think disableSetDesktopBackground should return true in this case, no?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #37) > (In reply to Ian Moody [:Kwan] from comment #35) > > There is already an equivalent if check nearby for the media cache stuff, so > > moving the call inside that is enough to fix this, I believe. > > Thanks for looking into this, I guess I should have spotted this one... > Anyway, a few things I don't quite get in the fix. Why do you set the > initial value to null and not false? I hummed and hawed a little about which to pick. I went for null because it felt cleaner to have a null value for non-image elements where it doesn't apply (the adjacent cache variables are initialised to null also). And I figured the review could always result in changing it anyway if null was unsatisfactory. > (I can't find any difference between > how those two values are handled here...) I feel like > disableSetDesktopBackground should just handle the case where currentURI is > null. But do we really want to enable set bg in that case? I suppose it could but I was keeping the function as unchanged as possible. Also when currentURI is null the passed-up value is never going to be used anyway, since currentURI has to exist for any of the desktop background stuff to be in the context menu (due to being contingent on nsContextMenu.onLoadedImage which implies currentURI https://hg.mozilla.org/mozilla-central/annotate/cd0d976e5f5c/browser/base/content/nsContextMenu.js#l239)

Comment on attachment 8612541 [details] MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor I think your version makes just as much sense as what I suggested, so I'm fine with the current version, I don't want to hold you back with nuances just wanted to understand it better before I stamp r+ on it :)

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8808996d628 And thank you Ryan for the backout before, if you happen to be the one to see this.