User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 The actual security problem is, that if you press the PRINT / Screenshot button and then right-click and insert, Firefox will create a file called moz-screenshot.jpg in the directory C:\Dokumente und Einstellungen\[USER]\Lokale Einstellungen\Temp (German Windows XP) which contains the screenshot. This file is not deleted if you delete private data with Firefox. Reproducible: Always Steps to Reproduce: 1.Open the rich text editor for editing e.g. a wikipedia article or write a letter in Gmail etc. 2.Press the PRINT / Scrrenshot button 3.Right-Click and click Insert Actual Results: A file called moz-screenshot.jpg will be created in the directory C:\Dokumente und Einstellungen\[USER]\Lokale Einstellungen\Temp (German Windows XP) which contains a screenshot at the time you edited in the rich text editor. Expected Results: Firefox should not create this file moz-screenshot.jpg or at least delete it while deleting Private Data with the built-in Firefox feature. This problem was discussed at Mozilla Support (see URL above) with the conclusion: "But this is a weird feature... Firefox team, please do something about this."

Confirming, and taking over. I'm going to change the editor code to use a data: URI instead of a normal file: URI to a temporary file.

This patch makes the editor use a data: URI instead of writing to a temp file and pointing to it. In addition to addressing the privacy issue with the temp file approach, this could speed up the image paste operation a tiny bit because it removes the temporary file I/O. The patch comes with a unit test as well.

This is great! + if (0 == nsCRT::strcmp(bestFlavor, kJPEGImageMime)) + stuffToPaste.AppendLiteral("jpeg"); + else if (0 == nsCRT::strcmp(bestFlavor, kPNGImageMime)) + stuffToPaste.AppendLiteral("png"); + else + stuffToPaste.AppendLiteral("gif"); Why not just stuffToPaste.AppendUTF8toUTF16(bestFlavor, stuffToPaste); ?

I guess + stuffToPaste.AssignLiteral("<IMG src=\"data:image/"); would become + stuffToPaste.AssignLiteral("<IMG src=\"data:"); of course

Addressed the comment. The only issue that made me not do that from the start was the fact that kJPEGImageMime is #define'd as "image/jpg", whereas the correct mime type for JPG images should be "image/jpeg" IINM.

Comment on attachment 375447 [details] [diff] [review] Patch (v2) Yeah, I thought about that, but since we do handle image/jpg I think this is OK.

Landed as <http://hg.mozilla.org/mozilla-central/rev/12536e9936b2>. I modified the test to only ensure the scheme of the pasted image URI to be a base64-encoded data: URI, because as it turns out the actual encoded PNG is different on each platform, and we don't really care about that here.

What's the expected behaviour for sending an email using Thunderbird 3.next? I believe the screenshot.jpg is currently sent as a related attachment.

Comment on attachment 375447 [details] [diff] [review] Patch (v2) This is a nice privacy improvement for 1.9.1. The patch is rather low risk and comes with a test. Nominating for approval to land on 1.9.1.

(In reply to comment #8) > What's the expected behaviour for sending an email using Thunderbird 3.next? I > believe the screenshot.jpg is currently sent as a related attachment. With this patch, the image is sent inline (not inline in the MIME sense, but inside the HTML code). Would this cause problems for Thunderbird?

I think it could, if only because other user agents can't deal with data URIs.

(In reply to comment #11) > I think it could, if only because other user agents can't deal with data URIs. Yes, I think this is a valid point. I think Thunderbird code should deal with data: URIs somehow anyway (maybe embed them as MIME parts, attachments, etc.), even without this patch. How is this handled presently?

Comment on attachment 375447 [details] [diff] [review] Patch (v2) Canceling the approval request for 1.9.1 until the Thunderbird related issues can be answered in a good way.

(In reply to comment #12) > Yes, I think this is a valid point. I think Thunderbird code should deal with > data: URIs somehow anyway (maybe embed them as MIME parts, attachments, etc.), > even without this patch. How is this handled presently? Was there a way a message could contain data URI embeddings without this patch? (In reply to comment #13) > Canceling the approval request for 1.9.1 until the Thunderbird related issues > can be answered in a good way. In fact I think this should be backed out given those uncertainties.

(In reply to comment #14) > (In reply to comment #12) > > Yes, I think this is a valid point. I think Thunderbird code should deal with > > data: URIs somehow anyway (maybe embed them as MIME parts, attachments, etc.), > > even without this patch. How is this handled presently? > > Was there a way a message could contain data URI embeddings without this patch? I'm not sure, but I guess arbitrary HTML code can be embedded in the body of emails, right? > (In reply to comment #13) > > Canceling the approval request for 1.9.1 until the Thunderbird related issues > > can be answered in a good way. > > In fact I think this should be backed out given those uncertainties. Before we take such a measure, I'd appreciate if a mailnews guru weighs in (I don't know who's the best person to CC here). I'm sure that mailnews code should have special code to handle <img src="/path/to/tmp/moz-screenshot.png">, so why can't that code be modified to handle data: URIs as well?

(In reply to comment #15) > I'm not sure, but I guess arbitrary HTML code can be embedded in the body of > emails, right? That's likely true, but not necessarily relevant, since data URIs aren't widely used. > Before we take such a measure, I'd appreciate if a mailnews guru weighs in (I > don't know who's the best person to CC here). I'm sure that mailnews code > should have special code to handle <img src="/path/to/tmp/moz-screenshot.png">, > so why can't that code be modified to handle data: URIs as well? I'm not saying it can't, I'm not even saying it doesn't. I simply don't know. If we can get feedback from someone who knows quickly, that would be good. But this issue shouldn't hide and potentially be forgotten on trunk. You could file a new bug and make sure it's blocking 1.9.2. But a backout is pretty cheap these days (and cheaper if you do it now rather than before 1.9.2 ships).

I'm CCing some people from http://www.mozilla.org/mailnews/review.html (which should help us find the right people if they're not the right ones themselves). I've also backed the patch out of m-c for now... <http://hg.mozilla.org/mozilla-central/rev/0cb354f894d1>

xref bug 572637.

(Quoting bug 572637 comment #23) > 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. (Quoting bug 572637 comment #24) > 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. (Quoting bug 572637 comment #27) > 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.

FWIW, I plan to land this patch for 1.9.3, because it's very useful to Firefox. So, we really need to discuss how Thunderbird is going to handle this. Sooner is better than later! ;-)

It's certainly a good idea to get rid of the file workaround, but it has to work properly for all affected applications... Hopefully the discussion here and in the other bug woke up the right people to tackle this issue.

Thanks so much for the heads up, and apologies that none of us have responded sooner. If Mark doesn't get a chance to look at this today (and I suspect he won't, given that he's super busy with 3.1, I'll spend some time working through it tomorrow).

(In reply to comment #22) > Thanks so much for the heads up, and apologies that none of us have responded > sooner. If Mark doesn't get a chance to look at this today (and I suspect he > won't, given that he's super busy with 3.1, I'll spend some time working > through it tomorrow). Great, thanks! As a summary, Thunderbird needs to change the cid attachment generating code to look at the data: URIs in addition to file: URIs. If that's fine with you guys, I hope to land this patch early next week.

Unfortunately, my travel schedule was more hectic than I had hoped, so I haven't yet had a chance to poke at this. I'll try and find time on Monday. Can we coordinate then? I'm pretty much always around IRC as dmose...

(In reply to comment #24) > Unfortunately, my travel schedule was more hectic than I had hoped, so I > haven't yet had a chance to poke at this. I'll try and find time on Monday. > Can we coordinate then? I'm pretty much always around IRC as dmose... Sure, I'll try to ping you on IRC (if I don't forget!). If I do, please feel free to ping me!

The test for this patch fails now because of bug 520189. I guess we should special case the src attribute for <img> elements and allow all the values for them, because the source for images cannot run code in a dangerous way (it will run js in a sandbox even if it's a javascript: URI.) I suspect that this patch might also fix bug 572637...

Comment on attachment 454209 [details] [diff] [review] Part 2: allow all src URIs for <img> tag According to bug 572637 comment 38, this patch also fixes that issue.

Comment on attachment 454209 [details] [diff] [review] Part 2: allow all src URIs for <img> tag I'll post this patch with a test case in bug 572637.

This problem actually causes pasting images on the web to not work, because web pages cannot access file URIs. This is in addition to the privacy issues.

@Dan: Is there a follow-up bug for Thunderbird/MailNews Core to correctly interpret the data:-URI introduced here?

I don't have a working Thunderbird trunk build right now, but tested with a non-static Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100712 SeaMonkey/2.1a3pre {20100712072019} build instead. Pasting image data from the clipboard now resulted in a "data:image/png;base64,..." URI as desired, without generating any /tmp files, hence verifying this bug. Saving as draft or sending interestingly resolved the data:-URI to a "cid:" src already, thus no follow-up appears to be needed. The only polishing I can see is the associated file name, which appears to be a base64 fragment without any file extension. I have filed bug 578104 on this issue.

(In reply to comment #32) > I don't have a working Thunderbird trunk build right now, but tested with a > non-static Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100712 > SeaMonkey/2.1a3pre {20100712072019} build instead. Pasting image data from the > clipboard now resulted in a "data:image/png;base64,..." URI as desired, without > generating any /tmp files, hence verifying this bug. So I assume that you actually meant to mark this as VERIFIED, right? > Saving as draft or sending interestingly resolved the data:-URI to a "cid:" src > already, thus no follow-up appears to be needed. The only polishing I can see > is the associated file name, which appears to be a base64 fragment without any > file extension. I have filed bug 578104 on this issue. That's interesting! Thanks a lot for looking into this!

Yes, I wasn't sure if I'm the right one to mark Editor bugs "verified"... :-)

(In reply to comment #34) > Yes, I wasn't sure if I'm the right one to mark Editor bugs "verified"... :-) If you test a bug and make sure it's present on versions before a patch, but not after it, then you are the right person to mark it as verified! :-) Thanks!

For posterity & searchability, documenting the actual fix for this bug in the summary. TB is still suffering badly from such issues both in general design of how composition handles "things attached" (see attach-paradigm-fail tracker Bug 877159) and specifically for inline images. Somewhere on the way from Firefox (right-click on image, "Copy image") to pasting into Thunderbird composition editor, there are bugs that cause images to NOT be inserted as data: URI as this bug 490879 suggests to have implemented. So if somebody could check if this has regressed, been backed out or what other bugs there are, and in which products, I'd be grateful.

:Ehsan, has this regressed or what/where are the reasons for failure of Bug 989621? Bug 989621 - Images with non-file URLs copied from FF appear broken after (auto/manual) saving of draft; pasting images into TB composition does not use <img src="data: URI"> (inconsistent with drag & drop) (In reply to Thomas D. from comment #36) > TB is still suffering badly from such issues both in general design of how > composition handles "things attached" (see attach-paradigm-fail tracker Bug > 877159) and specifically for inline images. > > Somewhere on the way from Firefox (right-click on image, "Copy image") to > pasting into Thunderbird composition editor, there are bugs that cause > images to NOT be inserted as data: URI as this bug 490879 suggests to have > implemented. So if somebody could check if this has regressed, been backed > out or what other bugs there are, and in which products, I'd be grateful.

I would assume that Firefox puts a text/html part onto the clipboard given that you are copy-pasting an <img> HTML element. On the other hand, there should also be an image/* flavor if you've used "Copy Image" which Thunderbird would need to prefer over the text/html part when pasting, then it should be encoded as data: URI there. Assuming that you are on Windows, the image part may only be available as DIB and thus would need to be recoded as PNG or JPEG depending on clipboard.paste_image_type, hence pasting the reference has the advantage of retaining the original encoding of the web page as is.

Bug 557708 comment 3 suggests that FF needs to add the original image flavor to the clipboard flavors when "Copy image" is done: (In reply to Brian R. Bondy [:bbondy] from comment #3) > In particular we could support when we "Copy Image" we add to the clipboard: > CFSTR_MIME_X_PNG (TEXT("image/x-png")) > > We would then need to post a different [bug] for Thunderbird to use this new > clipboard format instead though. As you commented on Bug 989621 Comment 1, even recoding the image in a flavor different from the original image flavor would be the lesser evil compared to relying on remote URL reference which is totally outside TB's control while we suggest to the user that the image has already successfully been copied into the composition. Keeping external references for things already added to the composition is always unreliable and calling for trouble, as explained in attach-paradigm-fail tracker Bug 877159, to which you have just added more reasons which might break retrieval of external data at later time: > Clearly, the current approach of passing the image by reference and downloading the image from the > source breaks if the context is lost (e.g., secure sites requiring login, image depends on context > like cookies, etc.), thus recoding the image may be the lesser of two evil.

(In reply to Thomas D. from comment #37) > :Ehsan, has this regressed or what/where are the reasons for failure of Bug > 989621? I don't think you correctly understood what this bug did. This is about copying images from local disk or other sources which do not have a URL associated with them, and it has not regressed.