Closed Bug 572637 Opened 14 years ago Closed 14 years ago

Paste image into Compose window broken with Shredder

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0b2
Tracking Flags:
Tracking Status
blocking2.0 --- beta2+
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed

People

(Reporter: u382332, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression, verified1.9.1, verified1.9.2, Whiteboard: [tbtrunkneeded])

Attachments

(1 file)

Patch (v1)
6.29 KB, patch
bzbarsky
: review+
dveditz
: approval1.9.2.9+
dveditz
: approval1.9.1.12+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100616 Minefield/3.7a6pre Firefox/3.6
Build Identifier: 

Unable to paste image into Compose window with Shredder.

Copy/paste generates this error in the console:
Security Error: Content at about:blank may not load or link to file:///C:/DOCUME~1/Owner/LOCALS~1/Temp/moz-screenshot-8.png.

Drag and drop from local storage generates this error in console but works from a web page.
Security Error: Content at about:blank may not load or link to file:///C:/Temp/aaaaaa.gif.

Regression

Works:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100614 Shredder/3.2a1pre

Does not work:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100615 Shredder/3.2a1pre

Reproducible: Always
I also can confirm this for Mac, so I've set the platform to all.

Copy/paste on Mac gives in the error console:
"Security Error: Content at about:blank may not load or link to file:///Users/Nomis101/Library/Caches/TemporaryItems/moz-screenshot.png."

Drag&Drop gives in the error console:
"Security Error: Content at about:blank may not load or link to file:///Users/Nomis101/Desktop/Bildschirmfoto.png."

But no inserted image in both cases, only the broken link image.
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; de; rv:1.9.3a6pre) Gecko/20100616 Thunderbird/3.2a1pre
OS: Windows XP → All
Hardware: x86 → All
set to new/regression
product Thunderbird for better visibility.
Status: UNCONFIRMED → NEW
Component: Security → Message Compose Window
Ever confirmed: true
Product: Core → Thunderbird
QA Contact: toolkit → message-compose
Target Milestone: --- → Thunderbird 3.2a1
Whiteboard: [tb32needs]
Also confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100616 SeaMonkey/2.1a2pre, likely Core code.

The message is produced in /caps/src/nsScriptSecurityManager.cpp:
http://mxr.mozilla.org/mozilla-central/search?string=CheckLoadURIError
The most obvious bug in that range would be bug 572660...

Moving back to core, I'll try and confirm the exact changeset in a while.
Component: Message Compose Window → Editor
Product: Thunderbird → Core
QA Contact: message-compose → editor
Target Milestone: Thunderbird 3.2a1 → ---
(In reply to comment #5)
> The most obvious bug in that range would be bug 572660...

Did you mean bug 520189?

Are we talking about pasting images, or HTML containing images?  Bug 520189 should not have had any effect over pasting images in the editor...
Hmm, I could reproduce this on mozilla-central using the Midas demo as well.  I'm bisecting on the m-c regression range in comment 4 right now.
(In reply to comment #6)
> Are we talking about pasting images, or HTML containing images?  Bug 520189
> should not have had any effect over pasting images in the editor...

Yes, pasting from the clipboard is broken, which creates a temporary PNG or JPG file, then HTML code with a "file://" URL is inserted. Apparently accessing the temporary file fails now on trunk builds.
(In reply to comment #6)
> (In reply to comment #5)
> > The most obvious bug in that range would be bug 572660...
> 
> Did you mean bug 520189?

Yes, sorry, wrong paste.
 
> Are we talking about pasting images, or HTML containing images?  Bug 520189
> should not have had any effect over pasting images in the editor...

I've just backed out http://hg.mozilla.org/mozilla-central/rev/208d7f999037 locally and it fixed the issue, however, feel if you can confirm I think that would be good.
Without having access to the original bug and its comments, it appears that nsHTMLDataTransfer.cpp may be too restrictive. You've added a section with
> +  // Allow style elements and attributes
which may need to be extended to also allow file access, this would explain
the seen error message if only style-related items are allowed (guessing).
Presumably the issue is the CheckLoadURI checks the paranoid sink does?

For the Midas stuff, this is probably correct, I would think.  Dragging local stuff into that shouldn't really work.

For Shredder, why are we using the paranoid sink to start with?  But if we are, note that CheckLoadURI for images is special-cased in editors normally (but not in the paranoid sink).  See https://hg.mozilla.org/mozilla-central/file/2dcce82f9d66/content/base/src/nsContentUtils.cpp#l2267
(In reply to comment #11)
> For Shredder, why are we using the paranoid sink to start with?  But if we are,
> note that CheckLoadURI for images is special-cased in editors normally (but not
> in the paranoid sink).  See
> https://hg.mozilla.org/mozilla-central/file/2dcce82f9d66/content/base/src/nsContentUtils.cpp#l2267

I'm not sure we are using paranoid sink. If I've debugged correctly then we're actually hitting here:

http://hg.mozilla.org/comm-central/file/cbaa2cb21cf3/mailnews/base/src/nsMsgContentPolicy.cpp#l403

This is an exclusion I added in bug 522019 although I wasn't totally sure that was the right thing to do.

I think this bug may therefore be a result of a change in core triggering this, and I need to figure out what's the right thing to do here.
> If I've debugged correctly then we're actually hitting here:

That wouldn't cause the error console messages from comment 1.
The regression range in comment 4 is bogus (i.e., the first revision in the range has the same bug).  So, this is clearly not a regression from bug 520189.  But we need a regression range in order to be able to track this.
No longer blocks: CVE-2010-2769
blocking2.0: --- → ?
Keywords: regressionwindow-wanted
(In reply to comment #9)
> I've just backed out http://hg.mozilla.org/mozilla-central/rev/208d7f999037
> locally and it fixed the issue, however, feel if you can confirm I think that
> would be good.

That's *really* strange.  The only thing that patch had an effect on was paste operations containing CF_HTML content, which go through the HTML parser and the paranoid content sink.  Anyways, I've also backed that patch out locally and I'm building right now.  But I don't believe that's going to prove anything more than my bisecting experiment in comment 14...
(In reply to comment #15)
> (In reply to comment #9)
> > I've just backed out http://hg.mozilla.org/mozilla-central/rev/208d7f999037
> > locally and it fixed the issue, however, feel if you can confirm I think that
> > would be good.
> 
> That's *really* strange.

It could be that we're really seeing two different things.

I think the changes in bug 520189 could have caused how the content policy works to be different for Thunderbird, and hence the regressions I'm seeing in Thunderbird, but that would then be due to the about:blank bug I mentioned in comment 12.

The midas editor failure could be a different change.

(In reply to comment #13)
> > If I've debugged correctly then we're actually hitting here:
> 
> That wouldn't cause the error console messages from comment 1.

Why not? That code is rejecting the request with originating uri about:blank (via ShouldLoad), that I think would be enough to cause that error message afaict.
> Why not?

Because that error message comes from inside CheckLoadURI.

> that I think would be enough to cause that error message afaict.

You tell wrong, sorry.  ;)
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #9)
> > > I've just backed out http://hg.mozilla.org/mozilla-central/rev/208d7f999037
> > > locally and it fixed the issue, however, feel if you can confirm I think that
> > > would be good.
> > 
> > That's *really* strange.
> 
> It could be that we're really seeing two different things.
> 
> I think the changes in bug 520189 could have caused how the content policy
> works to be different for Thunderbird, and hence the regressions I'm seeing in
> Thunderbird, but that would then be due to the about:blank bug I mentioned in
> comment 12.
> 
> The midas editor failure could be a different change.

So, firstly I'm only talking about the DOM_SECURITY_ERROR thrown here.  That is the core regression, and it may trigger more bugs in Thunderbird as well.

The core bug is not caused by bug 520189.  I just updated to tip and backed out that changeset locally and verified that the resulting build has the same problem.

I'm trying to find a regression range now.
OK, to confirm my sanity, this has never worked!  I tested 3.5.9, and it can't access the file URIs.  I guess we could improve that if Thunderbird decided to handle the patch in bug 490879, but I've never had feedback from Thunderbird folks on that!

I'm moving this back to the Thunderbird component, because it doesn't seem to be a core bug.  If bug 520189 triggers this, please try to apply the three patches in that bug individually to see which one is triggering this behavior, so that we can have a easier time tracking this down.
Component: Editor → Message Compose Window
Keywords: regressionwindow-wanted
Product: Core → Thunderbird
QA Contact: editor → message-compose
Also, bug 520189 is going to be ported to branches as well (tentatively scheduled for the next dot-release, which would be 1.9.2.7 and 1.9.1.12, possibly), so this really needs to be solved by that time so that we make sure that Thunderbird dot releases won't be affected by this bug.

Let me know if I can help here.
status-thunderbird3.0: --- → ?
status-thunderbird3.1: --- → ?
Per comment #3 that would be MailNews Core then, but I'll leave it up to standard8 to make the final assessment.
(In reply to comment #21)
> Per comment #3 that would be MailNews Core then, but I'll leave it up to
> standard8 to make the final assessment.

Yep.  I'm not familar with Thunderbird components, so I just put this bug back where it came from!  ;-)
(In reply to comment #19)
> OK, to confirm my sanity, this has never worked!  I tested 3.5.9, and it can't
> access the file URIs.  I guess we could improve that if Thunderbird decided to
> handle the patch in bug 490879

So, what you are saying is that MailNews Core has to cope with not creating a temporary file for image pasting first since bug 520189 exposed a dependency on that mechanism (which existed for quite a while), and is now expected to make it work also in a hurry on all current release branches?

> but I've never had feedback from Thunderbird folks on that!

Looking at the cc list of bug 490879, it appears the right people are on there. Maybe that patch would work right away, but the first problem I'd see is that the resource (whatever it looks like) must be available at the time of sending or saving as a draft, which might have been the reason for using a temporary file to start with.
Ok, the encoded JPEG or PNG data would be put as base64 into the <IMG> itself, this should avoid the problem of having the resource no longer available at the time of sending, and may also solve issues like bug 404922, which would require to use the same mechanism then as well for drag-and-drop (see description here, the drag-and-drop issue can't be solved by bug 490879, given the reference to the original file dropped from is inserted).

The other question is how to proceed with a "data:"-encoded stream: Sending as is may make other e-mail clients unhappy even if Thunderbird supports it, thus using a similar replacement scheme from "file:" to "cid:" and making the image data an attachment would be necessary if "file:" URLs are no longer allowed.
For purposes of checking the Thunderbird regression range:
Works in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100614
Shredder/3.2a1pre
Fails in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100615
Shredder/3.2a1pre

So this might be a situation where bug 520189 plus a newer trunk checkin has a
cumulative effect.

??? bug 552121
> (comment #19) this has never worked! [FF 3.5.9]

Pasting images into both Thunderbird and SeaMonkey mail/news composition and the SeaMonkey Composer definitely worked until a couple of days ago, thus I'm having problems to understand what Ehsan is referring to. Unambiguously per comment #9 the problem doesn't occur with bug 520189 alone backed out. Thus, regardless of one's perspective, it remains the primary culprit.

The question is what to do about it, either relaxing that patch or keeping up with its consequences and solving the issues exposed by it.
(In reply to comment #23)
> (In reply to comment #19)
> > OK, to confirm my sanity, this has never worked!  I tested 3.5.9, and it can't
> > access the file URIs.  I guess we could improve that if Thunderbird decided to
> > handle the patch in bug 490879
> 
> So, what you are saying is that MailNews Core has to cope with not creating a
> temporary file for image pasting first since bug 520189 exposed a dependency on
> that mechanism (which existed for quite a while), and is now expected to make
> it work also in a hurry on all current release branches?

No.  Bug 520189 has nothing to do with bug 490879 per se.  I was merely pointing out that coping with the patch in bug 490879 is going to put an end to all similar security errors once and for all!  :-)

But yes, I'd really hope that there would be a fix to this issue before the next dot-release in Thunderbird.  Because bug 520189 is a security fix which we do want for branches.  Let's focus our energy in solving this problem before the regression would start being a problem for Thunderbird dot-releases for now!  :-)

> > but I've never had feedback from Thunderbird folks on that!
> 
> Looking at the cc list of bug 490879, it appears the right people are on there.
> Maybe that patch would work right away, but the first problem I'd see is that
> the resource (whatever it looks like) must be available at the time of sending
> or saving as a draft, which might have been the reason for using a temporary
> file to start with.

I actually landed the patch, and backed it out for the sake of Thunderbird (see bug 490879 comment 8 and comments after that), but Thunderbird folks never got back to me on what their plans are for dealing with that change.

I may revisit bug 490879 at some point and land the patch on mozilla-central anyways, but that's a different issue.

(In reply to comment #24)
Let's move this discussion to bug 490879.  In short, Thunderbird only has to change some code to get the data from the data URI instead of from the file, and nothing should change for them.


(In reply to comment #26)
> > (comment #19) this has never worked! [FF 3.5.9]
> 
> Pasting images into both Thunderbird and SeaMonkey mail/news composition and
> the SeaMonkey Composer definitely worked until a couple of days ago, thus I'm
> having problems to understand what Ehsan is referring to. Unambiguously per
> comment #9 the problem doesn't occur with bug 520189 alone backed out. Thus,
> regardless of one's perspective, it remains the primary culprit.

All I was talking about was Firefox.  :-)  And it's not only my perspective; I wrote the patch to bug 520189, so I have a pretty good idea what the patch does, and I can assure you that the code touched by that patch does not directly have anything to do with the code path for pasting images.

It might be something else that MailNews core is doing wrong, which bug 520189 uncovers.  I think for debugging this, one could start with setting a breakpoint in nsClipboardCommand::DoCommand, and see what's happening down from there.

If you need help with the editor side of things, please do let me know.

> The question is what to do about it, either relaxing that patch or keeping up
> with its consequences and solving the issues exposed by it.

I'm afraid that relaxing the checks done in that patch is not an option for security reasons.  But I'm pretty sure that we can solve the actual problem here, so I'm not too much worried about that.
I've moved comment #24 to bug 490879 comment #19 where it belongs. I was under the impression that your patch would disallow "file:"-URLs in pasted contexts completely, but apparently this can be worked around. Thus, rewinding to the discussion in comment #11 and following...
Another side effect of this bug.
I have an HTML sig that inserts a local image file that _was_ converted to a
CID
<img
 style="padding-top: 4px;" alt=""
 src="file:///C:/demogifs/mlogo2.gif"

That is now busted by the "Security violation"
As others noted, Insert > Image still works. Thus, it must be doing something differently than the other mechanism (copy-paste, drag-and-drop, signature).
(In reply to comment #19)
>If bug 520189 triggers this, please try to apply the three
> patches in that bug individually to see which one is triggering this behavior,
> so that we can have a easier time tracking this down.

If no one other have time to do this, I can do it. But because this is a secret bug and I only can access the entire changeset I need to know which files belong to patch 1, 2 and 3 than (if this isn't secret also).
If I only back out the changes for editor/libeditor/html/nsHTMLDataTransfer.cpp from mozilla-central/rev/208d7f999037 than inserting pictures via Copy&Paste and Drag&Drop works again. 
I first backed out only the changes for "// Allow style elements and attributes", but this doesn't help. So I also backed out the changes for "sink = do_CreateInstance" and now pasting images into the compose window works again.
That finding correlates well with comment #11 on using the paranoid sink, it
is my understanding though that this part won't be relaxed (comment #27).
(In reply to comment #20)
> Also, bug 520189 is going to be ported to branches as well (tentatively
> scheduled for the next dot-release, which would be 1.9.2.7 and 1.9.1.12,
> possibly), so this really needs to be solved by that time so that we make sure
> that Thunderbird dot releases won't be affected by this bug.

Based on Ehsan's comment, and given that TB 3.1 is now out, I'm elevating the nominations to block the next dot releases. This would hit for 3.1.2 and 3.0.7 unless resolved, and render a substantial feature unusable on these releases.

This would obviously affect SeaMonkey 2.0.7 equally, but depending on where the issue has to be fixed, the Thunderbird solution may fix it as well or requires a follow-up bug to port the changes to SeaMonkey.
Severity: normal → major
blocking-thunderbird3.0: --- → ?
blocking-thunderbird3.1: --- → ?
status-thunderbird3.0: ? → ---
status-thunderbird3.1: ? → ---
Can anyone try a Thunderbird build including the patch from bug 572642 to see if it changes anything?  (I'm not sure if it should, just checking.)

If that doesn't help, I'd probably need someone from the Thunderbird team to help me debug this.  We can start by someone putting a break point on this line: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLDataTransfer.cpp#2729> and report the values of res and contextfrag when pasting an image.  Ping me on IRC if you need help.  Thanks!
Still the same for me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100625 Shredder/3.2a1pre, which should have that patch.
It would be very interesting if someone could apply the patch in attachment 454209 [details] [diff] [review] and see if it fixes this bug...
(In reply to comment #37)
> It would be very interesting if someone could apply the patch in attachment
> 454209 [details] and see if it fixes this bug...

Good news, this fixed the issue for me (tested on Mac), for D&D and C&P.
Interestingly I see the broken link image  for a very short moment before I see the inserted picture.
Assignee: nobody → ehsan
Blocks: CVE-2010-2769
Status: NEW → ASSIGNED
Component: Message Compose Window → Editor
Product: Thunderbird → Core
QA Contact: message-compose → editor
Attached patch Patch (v1) — — Splinter Review 
Patch + testcase.
Attachment #454309 - Flags: review?(bzbarsky)
Blocks: 490879
Hello in the newest nightly version the error occurs too
Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100630 SeaMonkey/2.1a3pre
Franz
Franz, it can't be fixed in the nightly builds until attachment 454309 [details] [diff] [review]
has been approved and checked in.
Comment on attachment 454309 [details] [diff] [review]
Patch (v1)

r=bzbarsky.  Sorry for the lag.
Attachment #454309 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/a655a678dffb
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
Sorry folks, I need to clear a flag which bugzilla doesn't, temporarily moving and then back again.
Component: Editor → Composition
Product: Core → MailNews Core
Target Milestone: mozilla1.9.3b2 → ---
blocking-thunderbird3.0: ? → ---
blocking-thunderbird3.1: ? → ---
Component: Composition → Editor
Product: MailNews Core → Core
Target Milestone: --- → mozilla1.9.3b2
Verified paste image is fixed.

Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100714 Shredder/3.2a1pre
Status: RESOLVED → VERIFIED
We're going to be taking bug 520189 on the stable Gecko branches for Firefox, so I think we'll need this in there for Thunderbird on those branches.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
Comment on attachment 454309 [details] [diff] [review]
Patch (v1)

This is needed on branches by bug 520189.
Attachment #454309 - Flags: approval1.9.2.9?
Attachment #454309 - Flags: approval1.9.1.12?
Comment on attachment 454309 [details] [diff] [review]
Patch (v1)

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz
Attachment #454309 - Flags: approval1.9.2.9?
Attachment #454309 - Flags: approval1.9.2.9+
Attachment #454309 - Flags: approval1.9.1.12?
Attachment #454309 - Flags: approval1.9.1.12+
Backed out because bug 520189 was backed out (see bug 520189 comment 83).
status1.9.1: .12-fixed → ?
status1.9.2: .9-fixed → ?
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .9+
status1.9.1: ? → wanted
status1.9.2: ? → wanted
Ehsan, any ETA when this can land on the branches again? Since bug 520189 has been checked in yesterday, it leaves the current Thunderbird/Lanikai nightly builds broken with this bug...
Thanks!
Verified for 1.9.1 and 1.9.2 based on passing tests.
Keywords: verified1.9.1, verified1.9.2
Whiteboard: [tb32needs] → [tbtrunkneeded]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: