Last Comment Bug 572637 - Paste image into Compose window broken with Shredder
: Paste image into Compose window broken with Shredder
Status: VERIFIED FIXED
[tbtrunkneeded]
: regression, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: mozilla2.0b2
Assigned To: :Ehsan Akhgari
:
Mentors:
: 575371 (view as bug list)
Depends on:
Blocks: 490879 CVE-2010-2769
  Show dependency treegraph
 
Reported: 2010-06-17 01:50 PDT by Bozz
Modified: 2011-02-25 16:07 PST (History)
17 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2+
.9+
.9-fixed
.12+
.12-fixed


Attachments
Patch (v1) (6.29 KB, patch)
2010-06-26 16:46 PDT, :Ehsan Akhgari
bzbarsky: review+
dveditz: approval1.9.2.9+
dveditz: approval1.9.1.12+
Details | Diff | Splinter Review

Description Bozz 2010-06-17 01:50:00 PDT
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
Comment 1 Nomis101 2010-06-17 03:11:11 PDT
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
Comment 2 Joe Sabash [:JoeS1] 2010-06-17 03:51:14 PDT
set to new/regression
product Thunderbird for better visibility.
Comment 3 rsx11m 2010-06-17 05:46:57 PDT
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
Comment 5 Mark Banner (:standard8) (afk until 26th July) 2010-06-17 05:54:51 PDT
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.
Comment 6 :Ehsan Akhgari 2010-06-17 06:36:34 PDT
(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...
Comment 7 :Ehsan Akhgari 2010-06-17 06:41:37 PDT
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.
Comment 8 rsx11m 2010-06-17 06:52:43 PDT
(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.
Comment 9 Mark Banner (:standard8) (afk until 26th July) 2010-06-17 07:11:24 PDT
(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.
Comment 10 rsx11m 2010-06-17 07:17:45 PDT
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).
Comment 11 Boris Zbarsky [:bz] 2010-06-17 07:30:55 PDT
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
Comment 12 Mark Banner (:standard8) (afk until 26th July) 2010-06-17 08:12:21 PDT
(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.
Comment 13 Boris Zbarsky [:bz] 2010-06-17 08:19:44 PDT
> If I've debugged correctly then we're actually hitting here:

That wouldn't cause the error console messages from comment 1.
Comment 14 :Ehsan Akhgari 2010-06-17 08:25:49 PDT
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.
Comment 15 :Ehsan Akhgari 2010-06-17 08:32:05 PDT
(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...
Comment 16 Mark Banner (:standard8) (afk until 26th July) 2010-06-17 08:38:01 PDT
(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.
Comment 17 Boris Zbarsky [:bz] 2010-06-17 08:41:29 PDT
> 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.  ;)
Comment 18 :Ehsan Akhgari 2010-06-17 10:58:06 PDT
(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.
Comment 19 :Ehsan Akhgari 2010-06-17 11:09:08 PDT
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.
Comment 20 :Ehsan Akhgari 2010-06-17 11:10:59 PDT
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.
Comment 21 rsx11m 2010-06-17 11:17:49 PDT
Per comment #3 that would be MailNews Core then, but I'll leave it up to standard8 to make the final assessment.
Comment 22 :Ehsan Akhgari 2010-06-17 11:18:57 PDT
(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!  ;-)
Comment 23 rsx11m 2010-06-17 12:50:45 PDT
(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.
Comment 24 rsx11m 2010-06-17 13:53:28 PDT
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.
Comment 25 Joe Sabash [:JoeS1] 2010-06-17 14:05:02 PDT
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 26 rsx11m 2010-06-17 14:36:41 PDT
> (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.
Comment 27 :Ehsan Akhgari 2010-06-17 14:54:13 PDT
(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.
Comment 28 rsx11m 2010-06-17 16:12:50 PDT
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...
Comment 29 Joe Sabash [:JoeS1] 2010-06-17 16:20:18 PDT
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"
Comment 30 rsx11m 2010-06-17 16:37:39 PDT
As others noted, Insert > Image still works. Thus, it must be doing something differently than the other mechanism (copy-paste, drag-and-drop, signature).
Comment 31 Nomis101 2010-06-17 17:10:55 PDT
(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).
Comment 32 Nomis101 2010-06-18 10:01:08 PDT
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.
Comment 33 rsx11m 2010-06-18 10:15:56 PDT
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).
Comment 34 rsx11m 2010-06-25 08:28:18 PDT
(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.
Comment 35 :Ehsan Akhgari 2010-06-25 09:14:23 PDT
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!
Comment 36 rsx11m 2010-06-25 09:53:37 PDT
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.
Comment 37 :Ehsan Akhgari 2010-06-25 19:25:41 PDT
It would be very interesting if someone could apply the patch in attachment 454209 [details] [diff] [review] and see if it fixes this bug...
Comment 38 Nomis101 2010-06-26 03:54:27 PDT
(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.
Comment 39 :Ehsan Akhgari 2010-06-26 16:46:10 PDT
Created attachment 454309 [details] [diff] [review]
Patch (v1)

Patch + testcase.
Comment 40 Robert Kaiser 2010-06-29 13:11:04 PDT
*** Bug 575371 has been marked as a duplicate of this bug. ***
Comment 41 Franz Wein 2010-06-30 12:20:20 PDT
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
Comment 42 rsx11m 2010-06-30 13:26:31 PDT
Franz, it can't be fixed in the nightly builds until attachment 454309 [details] [diff] [review]
has been approved and checked in.
Comment 43 Boris Zbarsky [:bz] 2010-07-02 16:55:32 PDT
Comment on attachment 454309 [details] [diff] [review]
Patch (v1)

r=bzbarsky.  Sorry for the lag.
Comment 45 Mark Banner (:standard8) (afk until 26th July) 2010-07-13 04:51:12 PDT
Sorry folks, I need to clear a flag which bugzilla doesn't, temporarily moving and then back again.
Comment 46 Bozz 2010-07-14 15:37:29 PDT
Verified paste image is fixed.

Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100714 Shredder/3.2a1pre
Comment 47 Daniel Veditz [:dveditz] 2010-07-23 11:00:09 PDT
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.
Comment 48 :Ehsan Akhgari 2010-07-23 11:54:30 PDT
Comment on attachment 454309 [details] [diff] [review]
Patch (v1)

This is needed on branches by bug 520189.
Comment 49 Daniel Veditz [:dveditz] 2010-07-23 13:05:27 PDT
Comment on attachment 454309 [details] [diff] [review]
Patch (v1)

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz
Comment 51 :Ehsan Akhgari 2010-07-23 14:20:28 PDT
Backed out because bug 520189 was backed out (see bug 520189 comment 83).
Comment 52 rsx11m 2010-07-30 10:43:53 PDT
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...
Comment 54 rsx11m 2010-07-30 13:08:40 PDT
Thanks!
Comment 55 Al Billings [:abillings] 2010-08-11 16:21:07 PDT
Verified for 1.9.1 and 1.9.2 based on passing tests.

Note You need to log in before you can comment on or make changes to this bug.