333305 - [1.0.8] Yet another context menu regression

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[moz_bug_r_a4]

This is a regression from Bug 333131.

http://lxr.mozilla.org/aviary101branch/source/browser/base/content/browser.js#3797

<object> element can meet the conditions for image, but doesn't have |src|
property.

Issues:
* A user cannot save an image that is displayed by <object> element.  When
trying to save, the following error occurs.

Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a [nsIIOService.newURI]"  nsresult: "0x804b000a (<unknown>)"  location: "JS frame :: chrome://browser/content/contentAreaUtils.js :: makeURL :: line 694"  data: no]

* It is possible to circumvent security restrictions, which are
urlSecurityCheck() and "Don't try to save broken images" patch (bug 293527, I
cannot see this bug).

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1

A user cannot save an image that is displayed by <object> element.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2

It is possible to circumvent urlSecurityCheck().

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3

It is possible to circumvent "Don't try to save broken images" patch.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

ugh :(
I will look into this... I don't think this we can ship 1.0.8 with this bug.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Note: only the first hunk is really necessary. The second hunk fixes testcase 1 (saving) by making it work, but if it is ommitted the "save" option is just hidden.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

moz_bug, could you test this patch and see if you can find any issues with it? Any other testcases you could provide would also be very useful.

Comment 8

[moz_bug_r_a4]

This bug allows an attacker to run arbitrary code.  An attacker can circumvent
the fix for Bug 292737 by using <object> element.
http://lxr.mozilla.org/aviary101branch/source/browser/base/content/setWallpaper.xul#126

The gavin's patch does *not* block this exploit.

Comment 9

[moz_bug_r_a4]

Attachment: testcase 4 - arbitrary code execution (bug 333394)

Comment 10

[Daniel Veditz [:dveditz]]

Comment 8 and attachment 217800 [details] describe a problem that existed in 1.0.7, spun off into bug 333394.

Comment 11

[moz_bug_r_a4]

<embed> element can meet the conditions for image, and has |src| property.  But
its |src| value may be relative URI.

On 1.0.7, normal context menu appears when right-clicking on an image <embed>.

Comment 12

[moz_bug_r_a4]

Attachment: testcase 5 - image <embed>

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v2

This fixes this bug and bug 333394 by disabling the "Save" and "Set as wallpaper" options from everything but <input type=image> and <img>. I think the loss of functionality for <object type="image"> is minimal compared to the cost of teaching setWallpaper how to safely deal with <object>s, who don't have a "src". This also makes testcase 5 match 1.0.7 behavior.

Thanks for all the help moz_bug. Let me know if you find a problem with this patch?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v3

Without the loss of functionality as I discussed with dveditz. <object> never worked for "set as background" in the aviary series, so I'm not going to worry about making it work and instead just fix the security issue. <input type="images"> also don't work, but didn't before either. I'm kind of wary of just blacklisting javascript, I think maybe a http[s] whitelist would be safer. Explicitly checks for objects with native src attributes. Tested this with attachment 217816 [details], attachment 217594 [details], and the testcases in this bug. Also fixes bug 333394.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I guess theoretically the setWallpaper changes belong in bug 333394, but I tested and wrote the patch as one. I can split them off if that makes things easier.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 217817 [details] [diff] [review]
patch v3

r=me with nits (use the URI from the makeURL call instead of node.src, just to be safe)

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 217817 [details] [diff] [review]
patch v3

sr=dveditz

We need the equivalent nsContextMenu.js change for 1.7.13, but the suite doesn't need the setWallpaper changes.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 217817 [details] [diff] [review]
patch v3

Since we need a new patch with the xpfe version anyway, we should take care of bug 333428 since there's no point in fixing this regression if we're not fixing the original problem.

moz_bug_r_a4 suggests
>+        const nsIILC = Components.interfaces.nsIImageLoadingContent;
>+        if (this.onImage && (this.target instanceof nsIILC) && 
 -->          !this.target.imageBlocked) {
>+          var request = this.target.getRequest(nsIILC.CURRENT_REQUEST);
>+          if (request && (request.imageStatus & request.STATUS_SIZE_AVAILABLE))
>+            this.onLoadedImage = true;
>+        }

which seems to do the trick.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Question: While I understand the change from instanceof nsIImageLoadingContent to HTMLImageElement (to find elements who have a native .src property), why do you remove the check to || this.target.imageBlocked) earlier in the patch? That seems unrelated?

Comment 20

[Mike Connor [:mconnor]]

r=me on the additional change to check !this.target.imageBlocked.  Good catch to moz_bug_r_a4

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Oh, I see dan already pointed that out. It would be good if there were more documentation in this code that explained what these properties mean or are used for, but I digress.

I'm not the most familiar with the context menu code, but assuming this tests well, the change from ILC->HTMLImageElement should be reasonable enough to take based on my understanding of what's going on in the surrounding code.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Attachment: fix 333428 too (Firefox)

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Attachment: xpfe (Suite and Thunderbird)

Comment 24

[Daniel Veditz [:dveditz]]

> the change from ILC->HTMLImageElement should be reasonable enough to take

Note that HTMLImageElement is what this code used prior to our attempt to fix bug 293527. This patch partially backs out that fix (leaving the new onLoadedImage checks) and re-adds the check for loaded images down a bit from the original where it's not caught up in the object-detection code.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 217888 [details] [diff] [review]
xpfe (Suite and Thunderbird)

approved for aviary101 and mozilla1.7 branches, a=dveditz for drivers

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 217887 [details] [diff] [review]
fix 333428 too (Firefox)

approved for aviary101 branch, a=dveditz for drivers

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Landed attachment 217887 [details] [diff] [review] (minus setWallpaper.xul change) and attachment 217888 [details] [diff] [review] on the AVIARY_1_0_1 branch:
mozilla/xpfe/communicator/resources/content/nsContextMenu.js 	1.88.16.6
mozilla/browser/base/content/browser.js 	1.296.2.3.2.134.2.19

Landed attachment 217888 [details] [diff] [review] on the 1.7 branch:
mozilla/xpfe/communicator/resources/content/nsContextMenu.js 	1.88.2.8

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Here's a bonsai link for the checkins from this bug:
http://bonsai.mozilla.org/cvsquery.cgi?branch=&branchtype=match&who=gavin%25gavinsharp.com&whotype=match&date=explicit&mindate=2006-04-10+11%3A34&maxdate=2006-04-10+12%3A32

Comment 29

[Mike Schroepfer]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060410 Firefox/1.0.8

Testcases 1-4 look fine to me.  On testcase #5 right clicking on the image gives me the normal page menu.  On 1.5.0.2 right-clicking gets me the image options menu.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #29)
> Testcases 1-4 look fine to me.  On testcase #5 right clicking on the image
> gives me the normal page menu.  On 1.5.0.2 right-clicking gets me the image
> options menu.

The normal menu for testcase #5 is a known issue with all 1.0.x builds, and should not have changed between current builds and 1.0.7. In other words, my patch didn't fix it, but it shouldn't have caused it, either.

Comment 31

[Jay Patel [:jay]]

Testing Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060410 Firefox/1.0.8, I see the same results as schrep: testcases 1-4 work as expected and testcase 5 does not show the correct image context menu (known bug as Gavin mentioned).

So based on Gavin's previous comment and our testing so far (on Mac and Windows), it looks like the patch is good. :-)

Comment 32

[Jay Patel [:jay]]

Linux looks good too: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.13) Gecko/20060410 Firefox/1.0.8, testcases 1-3 look good, testcase 4 isn't applicable on Linux (don't see the "Set As Wallpaper" item), and testcase 5 is broken in the same way we saw on Windows and Mac.

Comment 33

[Jay Patel [:jay]]

Adding verified-aviary1.0.8 keyword based on testing of the latest respins.

Comment 34

[Tracy Walker [:tracy]]

Verified on Mozilla 1.7.13 candidates from 04-14 on Mac and Windows