[1.0.8] Yet another context menu regression

VERIFIED FIXED

Status

()

--
blocker
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: moz_bug_r_a4, Assigned: Gavin)

Tracking

({verified1.7.13})

1.7 Branch
verified1.7.13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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).
(Reporter)

Comment 1

13 years ago
Posted file testcase 1
A user cannot save an image that is displayed by <object> element.
(Reporter)

Comment 2

13 years ago
Posted file testcase 2
It is possible to circumvent urlSecurityCheck().
(Reporter)

Comment 3

13 years ago
Posted file testcase 3
It is possible to circumvent "Don't try to save broken images" patch.
ugh :(
I will look into this... I don't think this we can ship 1.0.8 with this bug.
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
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.
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.
Severity: normal → blocker
OS: Windows XP → All
Hardware: PC → All
Blocks: 333131
(Reporter)

Comment 8

13 years ago
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 8 and attachment 217800 [details] describe a problem that existed in 1.0.7, spun off into bug 333394.
Attachment #217800 - Attachment description: testcase 4 - arbitrary code execution → testcase 4 - arbitrary code execution (bug 333394)
(Reporter)

Comment 11

13 years ago
<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>.
(Reporter)

Comment 12

13 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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?
Attachment #217755 - Attachment is obsolete: true
Posted patch patch v3 (obsolete) — Splinter Review
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.
Attachment #217813 - Attachment is obsolete: true
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.
Attachment #217817 - Flags: superreview?(dveditz)
Attachment #217817 - Flags: review?(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)
Attachment #217817 - Flags: review?(mconnor) → review+
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.
Attachment #217817 - Flags: superreview?(dveditz)
Attachment #217817 - Flags: superreview+
Attachment #217817 - Flags: approval-aviary1.0.8?
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.
Attachment #217817 - Flags: approval-aviary1.0.8? → approval-aviary1.0.8-
Depends on: 333428
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?

r=me on the additional change to check !this.target.imageBlocked.  Good catch to moz_bug_r_a4
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.
Attachment #217817 - Attachment is obsolete: true
Attachment #217887 - Flags: approval-aviary1.0.8?
Attachment #217888 - Flags: approval1.7.13?
Attachment #217888 - Flags: approval-aviary1.0.8?
> 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 on attachment 217888 [details] [diff] [review]
xpfe (Suite and Thunderbird)

approved for aviary101 and mozilla1.7 branches, a=dveditz for drivers
Attachment #217888 - Flags: approval1.7.13?
Attachment #217888 - Flags: approval1.7.13+
Attachment #217888 - Flags: approval-aviary1.0.8?
Attachment #217888 - Flags: approval-aviary1.0.8+
Comment on attachment 217887 [details] [diff] [review]
fix 333428 too (Firefox)

approved for aviary101 branch, a=dveditz for drivers
Attachment #217887 - Flags: approval-aviary1.0.8? → approval-aviary1.0.8+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0.8, fixed1.7.13
Resolution: --- → FIXED

Comment 29

13 years ago
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.

(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

13 years ago
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

13 years ago
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

13 years ago
Adding verified-aviary1.0.8 keyword based on testing of the latest respins.
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
Verified on Mozilla 1.7.13 candidates from 04-14 on Mac and Windows
Status: RESOLVED → VERIFIED
Keywords: fixed1.7.13 → verified1.7.13
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Group: security

Updated

11 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.