Users can be deceived into sending involuntary attachments via email, thus disclosing private information / local files. Thunderbird allows to embed images inline in HTML emails, including local resources (specified with a file:// scheme). All embedded objects that are not tagged as moz-do-not-send="true" will automatically be attached to the message before dispatching. So, if this tag is contained in a mail body, the local file will be sent as well: <img src="file:///etc/hosts" moz-do-not-send="false"> For security, cited emails have to pass TagEmbeddedObjects(), which sanitizes all untrusted HTML elements (sets moz-do-not-send to true) before including the quoted text as part of a reply or forwarded message: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#444 Otherwise, an attacker could hide a prepared HTML tag in an email and wait for a victim to reply. The victim would quote the attacker's message including the directive to attach a specific file, making him unwittingly transmit a local resource. The measure leaves some attack vectors open, though. - Emails in the inbox can be re-sent as original messages via "Context Menu > Edit As New Message" (or CTRL+E). - HTML text can be copy-pasted into an email. In both cases, the content would not be sanitized and attachment directives are preserved. If I was to reply to someone with a last name as odd as mine, I would likely copy-paste it out of the mail, rather than risking to misspell the name. Any HTML elements hidden in between the letters would therefore end up in my own email and potentially lead to a local file disclosure. Proof of concept via the HTML attachment: - Open the page and copy the specified string. - Compose a new mail in Thunderbird and paste the string into the body. - Send it to someone. - View the raw source of the transmitted mail (CTRL+U). The /etc/hosts file should have been attached silently.

Confirming. Note: requires the use of HTML mail composition. This is not a mitigation because that's the default, but may trip up devs and testers who are more likely to prefer plain text than the general public. Going with sec-moderate as a first guess because the user interaction required will limit the spread of any attack, but it's a completely plausible attack and results in stealing local data so the impact is similar to a sec-high. It's a muddy line depending on how gullible you think the average Thunderbird user is, but there's no question that this attack can send back sensitive data.

I'd like to add that instead of specific files you could just target emails, which might be a more realistic use case and pretty reliable, although it's brute-force. Per https://tools.ietf.org/html/rfc5092 you can construct something like this: <img src="imap://imap.example.com:993/fetch>UID>/INBOX>23" moz-do-not-send="false"> The folder structure should be easy to guess for most mail providers. If you cascade multiple tags with consecutive IDs, chances are that you get an entire mailbox back.

(In reply to Armin Razmdjou from comment #3) > I'd like to add that instead of specific files you could just target emails, > which might be a more realistic use case and pretty reliable, although it's > brute-force. Per https://tools.ietf.org/html/rfc5092 you can construct > something like this: > <img src="imap://imap.example.com:993/fetch>UID>/INBOX>23" > moz-do-not-send="false"> > > The folder structure should be easy to guess for most mail providers. If you > cascade multiple tags with consecutive IDs, chances are that you get an > entire mailbox back. You'd need the username, SSL or STARTTLS, and the start UID to be included in URLs to guess. SMTP servers also reject mailing messages larger than a few dozen megabytes, which corresponds to only a few hundred messages. I don't know our copy-paste code, but probably the easiest way to block this is to sanitize URLs on copy-paste. If that doesn't work, preventing uploads of non-image data is probably the best bet, and dropping support for <link href="" rel="stylesheet"> (or perhaps filtering that out for valid CSS as well, but that's harder). Can someone who understands editor and/or copy-paste better evaluate the first option for feasibility? I'm only good at the backend side of this.

(In reply to Joshua Cranmer [:jcranmer] from comment #4) > You'd need the username, SSL or STARTTLS, and the start UID to be included > in URLs to guess. SMTP servers also reject mailing messages larger than a > few dozen megabytes, which corresponds to only a few hundred messages. I don't - my Thunderbird implies these details by default. So this is the exact payload I just successfully used on my Gmail account: imap://imap.googlemail.com:993/fetch>UID>/[Gmail]/All%20Mail>7 Also, a few hundred mails might be enough to pick up something interesting.

(In reply to Joshua Cranmer from comment #4) > I don't know our copy-paste code, but probably the easiest way to block this > is to sanitize URLs on copy-paste. > > Can someone who understands editor and/or copy-paste better evaluate the > first option for feasibility? I thought we already sanitised URLs on copy-paste, but Ehsan would know more.

(In reply to neil@parkwaycc.co.uk from comment #6) > (In reply to Joshua Cranmer from comment #4) > > I don't know our copy-paste code, but probably the easiest way to block this > > is to sanitize URLs on copy-paste. > > > > Can someone who understands editor and/or copy-paste better evaluate the > > first option for feasibility? > > I thought we already sanitised URLs on copy-paste, but Ehsan would know more. We do. But I'm not sure I understand what this bug report has to do with copy and paste? Isn't this about replying to an email without pasting anything?

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7) > We do. But I'm not sure I understand what this bug report has to do with > copy and paste? Isn't this about replying to an email without pasting > anything? (In reply to Armin Razmdjou from comment #0) > Proof of concept via the HTML attachment: > - Open the page and copy the specified string. > - Compose a new mail in Thunderbird and paste the string into the body. > - Send it to someone. > - View the raw source of the transmitted mail (CTRL+U). The /etc/hosts file > should have been attached silently. ^ That's what I was basing the copy-paste issue off of.

Oh, OK, sorry I missed that. The sanitization upon paste happens inside nsHTMLEditor::ParseFragment. You may want to see why that doesn't work. For example, make sure the caller doesn't set aTrustedInput to true.

Joshua or Neil, would either of you be able to try to make progress with this bug?

Did some investigation - we set nsIDocShell::APP_TYPE_EDITOR for the compose window => nsPlaintextEditor::IsSafeToInsertData will say it's safe -> no sanitation on paste - even with sanitation, a moz-do-not-send="false" is NOT sanitized - moz-do-not-send doesn't really matter (the test case img without it works too) - fwd/reply seem not affected Possible solution - on paste (even for app=mail), set moz-do-not-send=true for file: urls Comments?

Something like this? Don't make the paste trusted just because app=editor. Drop src="file://....." on paste

Comment on attachment 8725410 [details] [diff] [review] bug1151366_sec_file_attach.m-c.patch Review of attachment 8725410 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsTreeSanitizer.cpp @@ +1151,5 @@ > bool aAllowXLink, > bool aAllowStyle, > bool aAllowDangerousSrc) > { > +printf("xxxmagnus nsTreeSanitizer::SanitizeAttributes aAllowDangerousSrc=%d\n", aAllowDangerousSrc); Debugging left-overs? (several more below.) @@ +1446,5 @@ > sAttributesHTML, > (nsIAtom***)kURLAttributesHTML, > false, mAllowStyles, > (nsGkAtoms::img == localName) && > + !mCidEmbedsOnly && !mDropLocalLinks); I don't understand why this only disallows local links. Plus, I don't think that's a good solution in the first place, since it seems like Armin pointed out that the same issue exists with imap links. Furthermore, isn't http://myintranet/secret-file going to have the same issue? ::: editor/libeditor/nsHTMLDataTransfer.cpp @@ +2126,5 @@ > DocumentFragment** aFragment, > bool aTrustedInput) > { > +printf("xxxmagnus nsHTMLEditor::ParseFragment aFragStr=%s, aTrustedInput=%d\n", NS_ConvertUTF16toUTF8(aFragStr).get(), aTrustedInput); > + Looks like debugging left-overs? @@ +2142,5 @@ > if (!aTrustedInput) { > + uint32_t flags = aContextLocalName ? nsIParserUtils::SanitizerAllowStyle : > + nsIParserUtils::SanitizerAllowComments; > + flags |= nsIParserUtils::SanitizerDropLocalLinks; > + nsTreeSanitizer sanitizer(flags); You're changing the behavior for Firefox here, which is not OK. ::: editor/libeditor/nsPlaintextDataTransfer.cpp @@ -440,5 @@ > dsti->GetRootTreeItem(getter_AddRefs(root)); > nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(root); > - uint32_t appType; > - if (docShell && NS_SUCCEEDED(docShell->GetAppType(&appType))) > - isSafe = appType == nsIDocShell::APP_TYPE_EDITOR; This will probably break BlueGriffon and the SeaMonkey composer. I'm not 100% sure how they would rely on this.

Went back to the drawing board a bit with this. 1. don't make app==editor automatically safe --> sanitize will be run 2. no longer add the moz-do-not-send attribute to the list of allowed attributes --> on paste that attribute will be nuked Next up: comm-central part of this.

Reverse the logic for attaching. Require that we have moz-do-not-send="false" to send. The main problem was that we check for the moz-do-not-send attribute but in case there was NO such attribute we'd still accept to send. Also handle for the case of empty src properly. (On trunk you get an error dialog, and after clicking ok you still get to send...) I realize this will probably break someone's current usege, but I think the logic of just attaching without anyone explicitly asked for it is just so dangerous.

Comment on attachment 8730367 [details] [diff] [review] bug1151366_sec_file_attach.m-c.patch Review of attachment 8730367 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsPlaintextDataTransfer.cpp @@ -440,5 @@ > dsti->GetRootTreeItem(getter_AddRefs(root)); > nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(root); > - uint32_t appType; > - if (docShell && NS_SUCCEEDED(docShell->GetAppType(&appType))) > - isSafe = appType == nsIDocShell::APP_TYPE_EDITOR; Can you please explain what this part of the patch is trying to achieve? Isn't isSafe just set to false here for Thunderbird?

No, we set the appType to editor for composition here: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1005 This is so that file:// url images would be able to load during composition (and possibly something else). Obviously we don't want file urls to be able to load in *received* mails.

Comment on attachment 8730367 [details] [diff] [review] bug1151366_sec_file_attach.m-c.patch Review of attachment 8730367 [details] [diff] [review]: ----------------------------------------------------------------- I see, makes sense. I'm till slightly worried that this may affect BlueGriffon, not sure how active Daniel is these days. I'll flag him, but I think you should be good to land this in a few days if he doesn't respond.

(In reply to :Ehsan Akhgari from comment #18) > Comment on attachment 8730367 [details] [diff] [review] > bug1151366_sec_file_attach.m-c.patch > > Review of attachment 8730367 [details] [diff] [review]: > ----------------------------------------------------------------- > > I see, makes sense. I'm till slightly worried that this may affect > BlueGriffon, not sure how active Daniel is these days. I'll flag him, but I > think you should be good to land this in a few days if he doesn't respond. Discovering this only now. Looking.

Comment on attachment 8730367 [details] [diff] [review] bug1151366_sec_file_attach.m-c.patch I am not fundamentally against this patch (I can nuke it in BlueGriffon's tree) but I still find a few things a bit strange, sorry. First, nsIDocShell has a special constant for email clients, why is msgcompose using APP_TYPE_EDITOR and not APP_TYPE_MAIL that would allow a clean switch in nsPlaintextEditor::IsSafeToInsertData instead of that deletion that could have side-effects on pure editors having no restrictions on local files? Second, with the three lines deleted, isSafe is always false before the test |if (!isSafe && aSourceDoc)| so you can clean that up. Purely cosmetic but still. Third, I am not sure I like having pasting in a pure editor like BG based on Subsumes(). This feels wrong. I think I would try to use APP_TYPE_MAIL in msgcompose and fix the limited number of places where APP_TYPE_EDITOR is expected for TB instead. In the case of nsPlaintextDataTransfer.cpp, I would then keep the deleted lines but add a case for APP_TYPE_MAIL.

Or maybe a fourth constant in nsIDocShell meaning an editor inside an email client. We're unable to make the difference between that and a pure editor for the time being.

Ok, so only remove the moz-do-not-send attr from whitelist, so it gets sanitized away on paste.

For thunderbird, don't pretend we're an editor anymore. To be able to view file:// images during composition instead convert them to data uris directly. In addition to the earlier plan (comment 15). Joshua, are you able to review this anytime soon? If not, maybe rkent?

I'd be willing to try to review it if you cannot get anyone else, but I would need to do a develop of background understanding before I could. How about jorgk? He's much closer to the editor and composer than I am these days.

Comment on attachment 8736921 [details] [diff] [review] bug1151366_sec_file_attach.patch (comm-central part) Review of attachment 8736921 [details] [diff] [review]: ----------------------------------------------------------------- I've looked at it since I was threatened to review this. I see a lot of comment changes and minor tweaks and a couple of new functions. Let me ask some questions and then the review will be easier. Kent sometimes reviews his own patches to give further explanation. ::: mailnews/compose/content/mailComposeEditorOverlay.xul @@ +51,2 @@ > > + function OnAcceptOverlay() As far as I can see this is the only "real" change. Can you explain what's happening here. @@ +123,3 @@ > } > > + function GenerateDataURI(url) { Straight forward new function used above. ::: mailnews/compose/src/nsMsgCompose.cpp @@ -989,5 @@ > if (NS_FAILED(rv)) return rv; > > m_baseWindow = do_QueryInterface(treeOwner); > - > - window->GetDocShell()->SetAppType(nsIDocShell::APP_TYPE_EDITOR); Mentioned in comment #11. Editor app type has its consequences. ::: mailnews/compose/src/nsMsgSend.cpp @@ +1457,5 @@ > if (anchor && !forceToBeAttached) > return NS_OK; > } > > + *acceptObject = forceToBeAttached; Here we have the policy change mentioned in comment #15, right? Require that we have moz-do-not-send="false" to send.

(In reply to Jorg K (GMT+2) from comment #25) > ::: mailnews/compose/content/mailComposeEditorOverlay.xul > @@ +51,2 @@ > > > > + function OnAcceptOverlay() > > As far as I can see this is the only "real" change. Can you explain what's > happening here. Sure. When the user clicks ok in the image insert dialog, if a file was selected we convert that to a data: uri instead. Later on send, that data uri is resolved and sent as a cid (like before) if "attach the source of this" is checked. > > + *acceptObject = forceToBeAttached; > > Here we have the policy change mentioned in comment #15, right? Require that > we have moz-do-not-send="false" to send. Yes. I think the policy should be to include only when explicitly asked. Otherwise it's quite a chase to be able to catch all cases e.g. a simple paste where the source is file:// something (and moz-do-not-send is not set at all). And to handle that in a good way if encountered. Some people are likely to use pics in their html signature, they will have to add the moz-do-not-send="false" orstart using a data uri instead.

I think I can't do a qualified review since I don't have enough background knowledge. For example, I don't understand why the nsGkAtoms::mozdonotsend was removed from kAttributesHTML[] and what the sanitise is about. I also don't understand how the image was inserted before, that it, before this bit if (/^file:/i.test(gMsgCompInputElement.value.trim())) { var dataURI = GenerateDataURI(gMsgCompInputElement.value.trim()); gMsgCompInputElement.value = dataURI; gMsgCompAttachSourceElement.checked = true; was added. I did apply the patch to play around with. I noticed two problems: For I brief moment I see the base64 encoded data flash up on the screen in the compose window. That may be due to a slower debug build, but it's noticeable and will confuse the user. The second problem is more severe. I've added a signature that contains an image: <img moz-do-not-send="false" src="file:///D:/Desktop/Bugzilla%20down.png"> That image doesn't show in a new message. It shows when I look at the image and go "OK" on the dialog.

(In reply to Jorg K (GMT+2) from comment #28) > For example, I don't understand why the nsGkAtoms::mozdonotsend was removed > from kAttributesHTML[] and what the sanitise is about. When pasting html don't want someone to be able to hide moz-do-not-send="true" in the source, since that would lead to that file getting sent. Sanitation will now make sure the moz-do-not-send attribute is removed if someone tried to do that. > I also don't understand how the image was inserted before, that it, before > this bit > if (/^file:/i.test(gMsgCompInputElement.value.trim())) { > var dataURI = GenerateDataURI(gMsgCompInputElement.value.trim()); > gMsgCompInputElement.value = dataURI; > gMsgCompAttachSourceElement.checked = true; > was added. It just used to set the img src="file://something" and since editors allow linking to file the image was shown. On send we included anything that *did not* have the moz-do-not-send="false" set. > I did apply the patch to play around with. I noticed two problems: > > For I brief moment I see the base64 encoded data flash up on the screen in > the compose window. That may be due to a slower debug build, but it's > noticeable and will confuse the user. Haven't seen it. > The second problem is more severe. I've added a signature that contains an > image: > <img moz-do-not-send="false" src="file:///D:/Desktop/Bugzilla%20down.png"> > That image doesn't show in a new message. It shows when I look at the image > and go "OK" on the dialog. Hmm, yes (though it will get sent). Our options are not to support it, or have some migration/conversion occur.

Thanks for the clarifications. So the M-C change assures that the moz-do-not-send is stripped upon paste. BTW, you meant to say «hide moz-do-not-send="false"», right, since that's the dangerous case? As for the base64 flashing over the screen, you can believe me, or I can attach a video. Maybe not an issue on a optimised compile. As for the image in the signature, I don't understand your answer. I tried all combinations of <img src="file:///D:/Desktop/Bugzilla%20down.png"> <img moz-do-not-send="true" src="file:///D:/Desktop/Bugzilla%20down.png"> <img moz-do-not-send="false" src="file:///D:/Desktop/Bugzilla%20down.png"> in the HTML box and the picture never shows in the new message. I also tried D:\Desktop\Bugzilla down.png in the file box and that also doesn't work. Both options work on Daily. Surely hell will break lose if suddenly showing pictures in signatures stops working. If I understand the solution correctly, the compose window stops being an editor: - window->GetDocShell()->SetAppType(nsIDocShell::APP_TYPE_EDITOR); so the pictures are not pulled in automatically (quote: editors allow linking to file). They only get attached when added manually now when going "OK" on the properties dialogue. I undid that changed and now the images in the signature show as before. I wonder which other ramifications not being an editor will have. Note that David suggested using APP_TYPE_MAIL in comment #20. I'll clear the r? since I don't think it's ready.

(In reply to Jorg K (GMT+2) from comment #30) > I wonder which other ramifications not being an editor will have. Note that > David suggested using APP_TYPE_MAIL in comment #20. Should be safe, it's only special cased for this purpose. >http://mxr.mozilla.org/comm-central/search?string=APP_TYPE_EDITOR&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central> I didn't want to add another type as whole concept of linking to local files will be troublesome. E.g. likely not possible with any other editor if that ever changes.

Now images are showing in signatures. We replace them too with data uris (until send).

Comment on attachment 8740543 [details] [diff] [review] bug1151366_sec_file_attach.patch Review of attachment 8740543 [details] [diff] [review]: ----------------------------------------------------------------- I'll look at it in more detail tomorrow. Just one question for now. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +4170,5 @@ > > nsAutoCString readStr(readBuf, (int32_t) fileSize); > PR_FREEIF(readBuf); > > + // XXX: ^^^ could really use nsContentUtils::SlurpFileToString instead! What is this comment telling us?

Just that it looks to me we could use that function instead of using our reading the file ourselves, removing 20 or so lines of code. (Not really relevant to this bug, just noted it as it was close to code I was changing.)

Comment on attachment 8740543 [details] [diff] [review] bug1151366_sec_file_attach.patch Sorry, I have to give an r- here. Of the three "image in signature" cases, only one works. I'm also not convinced by the over-all concept. I'll explain that below. There are three ways to include an image in a signature: 1) Use HTML in the signature: My test case: <img moz-do-not-send='false' src="file://D:/Desktop/Bugzilla-down.png"> Not working: Picture is not visible when you first open a new composition. That's a consequence of not being an APP_TYPE_EDITOR any more. Image properties file://D:/Desktop/Bugzilla-down.png as expected. After sending it's fine. 2) Attach signature from file directly pointing to the image file: My test case: D:\Desktop\Bugzilla-down.png Not working: Picture is not visible when you first open a new composition. If you go to image properties you get the data URI: data:image/png;base64,iVBORw0KGgoAA ... ErkJggg== moz-do-not-send='false' Is the moz-do-not-send='false' correct in the data there? When sending this message I only see a broken image in the sent message. The sent message has: src="data:image/png;base64,iVBORw0KGgoAA ... ErkJggg==%20moz-do-not-send=%27false%27" That doesn't look right. 3) Attach signature from file pointing to a HTML file with a reference to the image. My test case: D:\Desktop\Bugzilla-down.html with this content: <img src="file://D:/Desktop/Bugzilla-down.png"> That works although you get the ugly data URI data:image/png;base64,iVBORw etc. in the image properties. The sent message has: <img moz-do-not-send="false" src="cid:part1.EDAB4045.F7CEF57B@jorgk.com"> I must say that I am not thrilled at all about the data URIs in the image properties. That certainly is a confusing change of the behaviour. I'm also not convinced that all this hassle is warranted and that withdrawing APP_TYPE_EDITOR from our e-mail composer is the right thing to do. What problem are we trying to solve here? I had to read comment #0 to get a better understanding. One problem is *pasting* content from an attacker's e-mail. If I understand it correctly, that is now covered by the M-C change, right? The exact effect of removing nsGkAtoms::mozdonotsend from kAttributesHTML[] is that the attribute is dropped, right? And then we have the "policy change", that no value means: Do not send. I'm fine with that. That leaves the problem of "Edit as new message". Now, this is already a mad thing to do. Why would I edit someone else's e-mail as new as my own message? Anyway, for this rare and crazy case, why don't we sanitize the message instead of jumping through all the hoops you're attempting here? Wouldn't it just be a case of calling TagEmbeddedObjects() on the "reused" message as we do for quotes? That should sanitise it the same way quotes are sanitised. What am I missing? Furthermore, I went through the trouble of looking at this: > > I wonder which other ramifications not being an editor will have. Note that > > David suggested using APP_TYPE_MAIL in comment #20. > Should be safe, it's only special cased for this purpose. > >http://mxr.mozilla.org/comm-central/ > search?string=APP_TYPE_EDITOR&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=c > omm-central> Easier here: https://dxr.mozilla.org/comm-central/search?q=APP_TYPE_EDITOR&redirect=false&case=false There are only two relevant cases: https://dxr.mozilla.org/comm-central/source/mozilla/dom/base/nsContentUtils.cpp#3011 That's where it says: // Editor apps get special treatment here, editors can load images from anywhere. So that's the behaviour you want to switch off. OK. But why don't we just allow the loading as long as the image is not sent? So in any part of the new message, that is obtained from the attacker's message via copy/paste or "edit as new", we simply strip or correct the moz-do-not-send attribute. What about the other one: https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsPlaintextDataTransfer.cpp#444 What are the consequences for not being an APP_TYPE_EDITOR any more? You mention some investigation in comment #11. Kent always mentions that he spends hours on reviews, so I'll just state that reviewing this is not easy and quite time consuming.

(In reply to Jorg K (GMT+2) from comment #36) > I must say that I am not thrilled at all about the data URIs in the image > properties. That certainly is a confusing change of the behaviour. Well mails are not really design documents, what matters is that the image is shown. > One problem is *pasting* content from an attacker's e-mail. If I understand > it correctly, that is now covered by the M-C change, right? The exact effect Partly. The checked in m-c change makes sanitation drop that element. But for sanitation to run we need to be APP_TYPE_MAIL. > What are the consequences for not being an APP_TYPE_EDITOR any more? Looks like none that really matter. > that reviewing this is not easy and quite time consuming. Welcome to the club ;)

The last patch had some bugs due to last minute reworks :( This works for all three cases. Also removing mail.compose.dont_attach_source_of_local_network_links since it doesn't make any sense anymore now that files are always data: instead.

I'll repeat again that an important secondary goal for removing the file:// source handling is that it really locks down what we can use for editor. It will only be trouble for any future developments of the composition window.

Comment on attachment 8741489 [details] [diff] [review] bug1151366_sec_file_attach.patch I am really sorry, and I can see all the effort you put into converting file content into data URIs, but I am not convinced by this solution. I am opposed to stop being a APP_TYPE_EDITOR type application and do all the work ourselves. I appreciate that you want to become more independent of what the M-C editor does for us, and you know that I would be the first person to roll out the bug-ridden M-C editor and roll something else into its place. However, presently we do rely on the M-C editor and it makes no sense to do 80% of the work ourselves (jumping through hoops and making the user-interface worse by showing those lovely data URIs) and break the remaining 20%. Create yourself a webpage with this content: <html> <body> <img src="file://someimage.png"> </body> </html> Open that page in Firefox. Copy the image. Paste it into a Magnus-style composition window. It doesn't work. Just like the signatures didn't work. I suggest not to fix these last 20% I am currently aware of by attaching some listener to the document and converting any picture on the fly upon paste. Please tell me what is so wrong about this approach: 1) Continue being a APP_TYPE_EDITOR. 2) Make sure we sanitize quotes and any content from "edit as new" or forward. You said in comment #37: Sanitising only works for APP_TYPE_EDITOR. Why did you change M-C code if you want to stop using the feature (not that the M-C change hurts)? 3) Switch the default attachment action to "do not attach". I'm happy that people have to rework their signatures. We need to be careful in the paste case described above.

(In reply to Jorg K (GMT+2) from comment #40) > Please tell me what is so wrong about this approach: > 1) Continue being a APP_TYPE_EDITOR. Not a big deal in it self. It's semantically wrong though and (as we see in this bug) gives you a big problem because it is special cased to say certain things are safe for editors, when in fact they are really not safe for mail. > 2) Make sure we sanitize quotes and any content from "edit as new" or > forward. > You said in comment #37: Sanitising only works for APP_TYPE_EDITOR. Why > did you change M-C code > if you want to stop using the feature (not that the M-C change hurts)? I don't expect us to stop using it anytime soon if ever. The change was only so people couldn't be tricked to paste <img moz-do-not-send="false" src="file:///" /> which would cause the file to be attached. That is, so that if we find a moz-do-not-send attribute on send, we can trust it. > 3) Switch the default attachment action to "do not attach". Yes that is what I've proposed too. The problem with what you propose above is this: yes, you'd get the image showing in the editor, but uselessly so since it wouldn't get *attached* so the recipient would still NOT see it. I've not tracked down how to fix your paste from file:// based web page (which would be very uncommon!) case but it's certainly possible to paste images as such from other software. I suppose it should just prefer the correct flavour if it finds a file url.

(In reply to Magnus Melin from comment #41) > Not a big deal in it self. It's semantically wrong though and (as we see in > this bug) gives you a big problem because it is special cased to say certain > things are safe for editors, when in fact they are really not safe for mail. Well, as we've seen, there are only two special cases for APP_TYPE_EDITOR in M-C: For loading images: https://dxr.mozilla.org/comm-central/source/mozilla/dom/base/nsContentUtils.cpp#3011 For other stuff, and I still don't understand what the ramifications are of switching this off. https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsPlaintextDataTransfer.cpp#444 nsPlaintextEditor::IsSafeToInsertData() is called a few times in "data transfer". It would be very handy to keep using the automatic image insertion. If the other stuff isn't desired, we'd have to be APP_TYPE_MAIL as previously suggested. > The change was only > so people couldn't be tricked to paste <img moz-do-not-send="false" > src="file:///" /> which would cause the file to be attached. That is, so > that if we find a moz-do-not-send attribute on send, we can trust it. Agreed. But we also need to sanitise "edit as new" and forwarded content. Also you said that the sanitise only works for APP_TYPE_MAIL (quote): "But for sanitation to run we need to be APP_TYPE_MAIL." I'm still confused: The main windows seems to be APP_TYPE_MAIL https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#108 but the compose window was APP_TYPE_EDITOR which your patch removes. > > 3) Switch the default attachment action to "do not attach". > Yes that is what I've proposed too. Of course. I didn't invent any of this ;-) > The problem with what you propose above is this: yes, you'd get the image > showing in the editor, but uselessly so since it wouldn't get *attached* so > the recipient would still NOT see it. Yes, I can see the dilemma, that's why I said above (quote): "We need to be careful in the paste case described above." We'd have to see how we would get image paste with "do not send" set to false, yet other paste actions should paste without that attribute, so it defaults to true. Maybe that's wishful thinking and not distinguishable.

OK, I've thought about it some more, here is my solution: 1) Continue being a APP_TYPE_EDITOR, or switch to APP_TYPE_MAIL, but let the M-C editor do the image handling. 2) Make sure we sanitize pasted content, quotes and any content from "edit as new" or forward. 3) Switch the default attachment action to "do not attach", so any content we see will not be attached unless the moz-do-not-send=false was explicitly set. 4) Signatures: I'm happy that people have to rework their signatures. Cases from comment #36: 1) Images via direct HTML: They need to add moz-do-not-send=false (or you can do it for them). 2) Images attached directly via "Attach signature from file": You add moz-do-not-send=false. 3) Images attached via HTML via "Attach signature from file": They need to add moz-do-not-send=false (or you can do it for them). In fact, you already visited all those spots to convert the image to data URI, so instead you can add the moz-do-not-send=false for file:// based pictures. 5) When pasting: Look at the content being pasted. If the content being pasted only contains one image and nothing else, set moz-do-not-send=false on that image. Advantage of this approach: We don't need to convert images to data URIs you introduced and the user sees the image name in the UI. Less code change. Yes, 5) is a little annoying to do, but in your approach you would have to do something similar to fix my "paste from file:// based web page". (I know it hurts to remove code you've already written, see bug 597369.)

Sorry, I don't like it. There are a lot of upsides in using my proposed approach, and no real upside with the other approach - besides less code change.

With this patch you can paste images from "Copy Image" even if they are file://. I'm just reordering the flavors so that if there's an image that would be preferred over the html transfer flavor.

Comment on attachment 8741489 [details] [diff] [review] bug1151366_sec_file_attach.patch Review of attachment 8741489 [details] [diff] [review]: ----------------------------------------------------------------- Could you have another look at this?

Comment on attachment 8742488 [details] [diff] [review] bug1151366_paste_img.mc.patch I think this won't fly since you will also prefer bitmaps over http:// links. I don't think this will pass review in M-C (since it would change browser behaviour for no good reason). Let's see how your try turns out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60f379a8533b339ce83b285616f3fe34befd2d4 One failure so far: bc7 424 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_clipboard.js | Paste after copy image - "<i>Italic</i> <img src=\"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAABHNCSVQICAgIfAhkiAAAAXZJREFUOE+1 == "<i>Italic</i> <img id=\"img\" tabindex=\"1\" src=\"http://example.org/browser/browser/base/content/test/general/moz.png\">Test - Looks like you managed to do what was intended, grab the data ;-)

Yes it's a slight change in behavior. Can't say if it will be accepted or not for sure, but drag'n'drop from desktop will also "paste" the image data like my patch, not the html with the src url. So it's consistent in that way, and the user did do "Copy Image" so it's not unreasonable imo. Then it's of course possible to just do the transfer flavor reordering for APP_TYPE_MAIL if needed.

(In reply to Magnus Melin from comment #46) > Could you have another look at this? I have looked at this for many hours, so I don't need to take another look. I don't like the approach and I will not approve it. These are my reasons: 1) I strongly dislike the insertion of data URIs since they destroy a useful user interface. Conversion to data URIs should happen as late as possible in the pipeline, usually at send time, not at composition time. 2) You cannot copy images from a file:// based web page. Your M-C change is not acceptable and won't find approval since it changes the behaviour of Firefox for no good reason. In reply to comment #48: You that there are lots of JS "editors" around that do a whole lot of listeting in the contenteditables they live it. We cannot change that behaviour. Hacking it for APP_TYPE_MAIL is, well, a hack. I will work on my approach: 1) Switch to APP_TYPE_MAIL and let the M-C editor do the image handling. This is necessary, as you pointed out, since APP_TYPE_EDITOR receives no sanitation of pasted content, but APP_TYPE_MAIL does. 2) Make sure we sanitize pasted content (done in 1)), quotes and any content from "edit as new" or forward. 3) Switch the default attachment action to "do not attach", so any content we see will not be attached unless the moz-do-not-send=false was explicitly set. 4) Signatures: I'm happy that people have to rework their signatures. Cases from comment #36: 1) Images via direct HTML: They need to add moz-do-not-send=false (or you can do it for them). 2) Images attached directly via "Attach signature from file": You add moz-do-not-send=false. 3) Images attached via HTML via "Attach signature from file": They need to add moz-do-not-send=false (or you can do it for them). In fact, you already visited all those spots to convert the image to data URI, so instead you can add the moz-do-not-send=false for file:// based pictures. 5) When pasting: Look at the content being pasted. If the content being pasted only contains one image and nothing else, set moz-do-not-send=false on that image. I will attach patches for 1) and 5) in the next comments. I believe that part of your patch can be used for parts 3) to 4). Let me know if you're prepared to accept a solution like the one I have proposed.

This also switches to APP_TYPE_MAIL.

Comment on attachment 8742664 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL Review of attachment 8742664 [details] [diff] [review]: ----------------------------------------------------------------- This is a major no-no. You can't allow mail content to access whatever they like on the system.

I have to say I don't think your dislike of data uris (for whatever reason I don't know) is a valid reason not to go ahead with them. Please base it on technical facts. Maybe others can chip in with opinions here?

(In reply to Magnus Melin from comment #52) > This is a major no-no. You can't allow mail content to access whatever they > like on the system. Huh? It allows insertion of images like we did before. Where's the problem? They are sanitised, so they won't be sent. What's wrong with the patch?

It also allows any mail you *receive* to link/access anything on your computer.

(In reply to Magnus Melin from comment #53) > dislike of data uris (for whatever reason I don't know) ... They are ugly, they can be huge for large images and they MUST not show in the user interface if avoidable. The user has <img src="file://...> in his signature or inserts an image from the file system and you show him the encoded binary representation of the file. Please don't! Let me know the technical advantages of your solution apart from being more independent of the M-C editor.

(In reply to Magnus Melin from comment #55) > It also allows any mail you *receive* to link/access anything on your > computer. Since the overall app is APP_TYPE_MAIL? OK, then the patch is wrong. Then we need to stay APP_TYPE_EDITOR and make sure we get sanitation on paste, or create APP_TYPE_MAIL_EDITOR. That's implementation detail. The main question to answer here: Immediate conversion of all images in the composition window to data URIs, yes or no. My answer is a clear "No".

Being *web compatible* is the huge deal. "In the user interface" is relative, because nobody really goes back to change the image source in the properties dialog. You delete the image and paste a new one. On the off chance you do want to change it, it doesn't matter what was there earlier.

Since you asked other people to chip in, let them have an image ;-) This is how the image properties dialogue will look like in Magnus' approach. All user-inserted imaged from a local file (including via a signature) are converted to data URIs. The original file location is lost. What do you think? Maybe Kent has an opinion, too, since he was meant to be a reviewer here.

(In reply to Magnus Melin from comment #58) > Being *web compatible* is the huge deal. And that means, more independence from the M-C editor?

The inclusion of data URIs in the composition would also preclude having any HTML editor associated with the compose window in the future since those potentially large data blocks would just fill the screen. There already is an add-on which provides a HTML tab, so its users wouldn't be pleased.

(In reply to Magnus Melin from comment #53) > I have to say I don't think your dislike of data uris (for whatever reason I > don't know) is a valid reason not to go ahead with them. Please base it on > technical facts. > > Maybe others can chip in with opinions here? First things first, I'm _not_ trying to pile onto the ongoing argument here. This is just my opinion and you and/or Jorg are welcome to disagree. :-) On switching the editable region from APP_TYPE_EDITOR to APP_TYPE_MAIL, I think that's definitely the right thing to do. Note that this flag affects whether Gecko tries to sanitize attributes and elements that can run scripts (think of pasting <img src="invalid" onerror="doBackThings()"> for example.) On the question of whether it's fine to show a data URI in the UI, I personally don't care much either way. If it's just the string in the UI, perhaps upon pasting you could remember the original URI somewhere (preferably outside of the DOM being edited) and use it in the UI when needed? On doing hacks such as attachment 8742664 [details] [diff] [review], absolutely no. I'll r- any such patch which turns off tree sanitization for all APP_TYPE_MAIL editors. On breaking people's existing email signatures, I definitely think we should make sure that images in signatures still work (i.e., the image shows up and will get sent as a data: URI.) On the objection to data: URIs, years ago we switched Thunderbird to convert pasted image files in emails to data: URIs, and the sky didn't fall (and to the best of my knowledge we did not even receive a single complaint.) I hope I'm covering all of the in-flight discussion points. If you have any other questions, please let me know.

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #62) > On doing hacks such as attachment 8742664 [details] [diff] [review], > absolutely no. I'll r- any such patch which turns off tree sanitization for > all APP_TYPE_MAIL editors. Ehsan, I don't understand your comment. My aim is to allow image inclusion from files in TB compose windows. This currently works since the compose window is of APP_TYPE_EDITOR. The downside of being APP_TYPE_EDITOR is that pasted content is *not* sanitised. I tried pasting this form a page <img moz-do-not-send="true" klaus-attr="huhu" src="file://D:/Desktop/Bugzilla-down.png"> and the attributes got pasted. That's why I wanted to switch to APP_TYPE_MAIL to get the sanitation and I did - if (appType != nsIDocShell::APP_TYPE_EDITOR) { + if (appType != nsIDocShell::APP_TYPE_EDITOR && appType != nsIDocShell::APP_TYPE_MAIL) { in attachment 8742664 [details] [diff] [review] to still allow file images being pulled in. This is the wrong thing to do, since the overall TB window is of APP_TYPE_MAIL, so just viewing an e-mail would access the image on the local machine as Magnus pointed out in comment #52. Since APP_TYPE_EDITOR doesn't get sanitation and APP_TYPE_MAIL can't be used, I'm proposing a new type APP_TYPE_MAIL_EDITOR which will get sanitation but will still allow images from file://. You're saying: I'll r- any such patch which turns off tree sanitization for all APP_TYPE_MAIL editors. That was never the plan and that was also not what the patch in attachment 8742664 [details] [diff] [review] was about. Or do you count nsContentUtils::CanLoadImage() as part of the sanitation? I thought, and I'm new to this area, that sanitation has to do with cleaning up content before pasting it. Please explain and also please look at the patch with the f? In an attempt to copy single images as bitmaps, Magnus has proposed attachment 8742488 [details] [diff] [review] which simply changes the order of the flavours. Personally I don't think it's a good idea. Can you please comment on this, too.

Comment on attachment 8742872 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR Review of attachment 8742872 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsIDocShell.idl @@ +320,5 @@ > */ > const unsigned long APP_TYPE_UNKNOWN = 0; > const unsigned long APP_TYPE_MAIL = 1; > const unsigned long APP_TYPE_EDITOR = 2; > + const unsigned long APP_TYPE_MAIL_EDITOR = 2; That should of course be = 3.

Comment on attachment 8742872 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR I don't know exactly what APP_TYPE_MAIL_EDITOR is supposed to be. If you want to create another app type, we need to define exactly how it differs from the existing ones, after auditing the existing places where we do something based on the app type. But I also disagree with your overall approach as my comment suggested. My comment was about treating all APP_TYPE_MAIL as APP_TYPE_EDITOR, I used the wrong attachment name. Sorry about that! Last but not least, I'm very busy and don't have time for reviews these days, as my Bugzilla name clearly suggests. :-) Please find another reviewer once there is a final patch ready. Thanks.

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #65) > I don't know exactly what APP_TYPE_MAIL_EDITOR is supposed to be. If you > want to create another app type, we need to define exactly how it differs > from the existing ones, after auditing the existing places where we do > something based on the app type. But I also disagree with your overall > approach as my comment suggested. I know you're very busy, but you've sadly caused some confusion here. If you want to avoid people having to clarify, please be clear to start with ;-) Which overall approach do you disagree with? You said that switching from from APP_TYPE_EDITOR to APP_TYPE_MAIL is definitely the right thing to do. If we do this, we lose the ability of showing images from file://. Either we need to do Magnus' trick to pull all the images in as data URIs or do my approach of creating a new app type, which, agreed, should be exactly defined. > My comment was about treating all APP_TYPE_MAIL as APP_TYPE_EDITOR, I used > the wrong attachment name. Sorry about that! I'm not aware that any patch suggested that, but then there's a history of obsolete patches, some carry your f- and others carry your r+. Can you please clarify and then I won't ask any more and we won't ask you for review.

(In reply to Jorg K (GMT+2) from comment #66) > (In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment > #65) > > I don't know exactly what APP_TYPE_MAIL_EDITOR is supposed to be. If you > > want to create another app type, we need to define exactly how it differs > > from the existing ones, after auditing the existing places where we do > > something based on the app type. But I also disagree with your overall > > approach as my comment suggested. > I know you're very busy, but you've sadly caused some confusion here. If you > want to avoid people having to clarify, please be clear to start with ;-) > Which overall approach do you disagree with? > You said that switching from from APP_TYPE_EDITOR to APP_TYPE_MAIL is > definitely the right thing to do. Right. > If we do this, we lose the ability of showing images from file://. Either we > need to do Magnus' trick to pull all the images in as data URIs Yes, this is what I suggest we should do.

The functionality of the "Insert HTML" window would also be destroyed by Magnus' (and Ehsan's) solution. For me, it took 13 seconds to bring up the window and TB became unresponsive during this time. Please don't destroy the UI for the sake of fixing a security bug.

Huh? Insert HTML is empty to start with! In any case, you're probably running a very slow debug build. Though the uris can obviously be long, it shouldn't take much time for real. Regarding editing html src that's not a feature available today, and it's totally up to the editor to cap what it wants to display in that view. I know I've seen editors do exactly that for long attribute values until you go to edit the attribute.

(In reply to Magnus Melin from comment #69) > Huh? Insert HTML is empty to start with! In any case, you're probably > running a very slow debug build. Though the uris can obviously be long, it > shouldn't take much time for real. > > Regarding editing html src that's not a feature available today, and it's > totally up to the editor to cap what it wants to display in that view. I > know I've seen editors do exactly that for long attribute values until you > go to edit the attribute. It's available, but not obvious. Select whole text in message and then choose Insert HTML, voila the whole html content is in this window. Thats how I'm changing things in html mails. And this would make it hard with data URIs as it fills the window unneeded.

Insert HTML shows the HTML around the spot where you edit, so I inserted the image first, then tried to add more HTML around it. Also, "Insert HTML" is the poor man's solution to inspect the HTML of the body. I was using the official TB Daily as of yesterday. There are other ways to insert the ugly data URI today without your patch (via drag) but your approach would aggravate the issue greatly since every image from a file would cause it.

<somewhat off topic> I tried a data URI on TinyMCE (https://www.tinymce.com/). They upload the image and show: blob:https://www.tinymce.com/37a04812-b882-46bb-a2f6-bbb3c20371dd as info. I also tried https://textbox.io. They also upload and show: <p><img alt="" src="https://s3.amazonaws.com/tbio.demo.images/1wugguwjiatkm69xmt7w1den1mpvqkxi75mwfmb49lz89o4r/soQYkXSaRI.bmp" /></p> All measures to hide the data like we always have done with <img src="file:// ...> And we're not talking about web editing as such. We're talking about a user preparing a document on his local machine with data from his local machine for sending later. </somewhat off topic>

Investigation into the use of APP_TYPE_MAIL in M-C: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4482 nsGlobalWindow::GetOpenerWindowOuter() - opener not returned for APP_TYPE_MAIL http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#7072 nsContentUtils::PrefetchEnabled() false returned for APP_TYPE_MAIL These wouldn't change with the introduction of APP_TYPE_MAIL_EDITOR. Investigation into the use of APP_TYPE_EDITOR in M-C: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#3011 nsContentUtils::CanLoadImage() - I suggest to add APP_TYPE_MAIL_EDITOR here, attachment 8742872 [details] [diff] [review]. http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsPlaintextDataTransfer.cpp#444 nsPlaintextEditor::IsSafeToInsertData() - true returned for APP_TYPE_EDITOR. Investigation into the use of IsSafeToInsertData() in M-C: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1108 http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1230 IsSafeToInsertData() is called and the result is passed into InsertObject(). Following the call-chain we get here: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#2133 There the sanitiser gets called if it wasn't safe. http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsPlaintextDataTransfer.cpp#190 I didn't look into that further. Summary: APP_TYPE_EDITOR inserts images and does NOT get sanitation. New APP_TYPE_MAIL_EDITOR would insert images and receive sanitation. My approach: 1) New APP_TYPE_MAIL_EDITOR (attachment 8742872 [details] [diff] [review]). 2) Make sure we sanitize pasted content (done in 1)), quotes and any content from forward or "edit as new". Note that quotes and inline forwarded content already pass through TagEmbeddedObjects(), so moz-do-not-send is set to true. Processing missing for "edit as new" content. 3) Switch the default attachment action to "do not attach", so any content we see will not be attached unless the moz-do-not-send=false was explicitly set. 4) Signatures as per comment #49 (not repeating it again). 5) When pasting: Look at the content being pasted. If the content being pasted only contains one image and nothing else, set moz-do-not-send=false on that image, attachment 8742665 [details] [diff] [review].

Please note that bug 1206961 may end up changing some of the code around CanLoadImage(). Please keep an eye on that bug. I don't think that will cause any material change to what has been discussed here but may end up bitrotting the patch...

I put the UI review back which apparently hasn't worked before since the requested parties weren't CC'ed at the time of the request. I just want to hear your opinion about the data UR's in the user interface.

Comment on attachment 8742696 [details] Screenshot showing the data URI I'm opposing In Image Properties dialog it would be no problem. But if you have to edit the message code in Insert HTML dialog then it's a pain with the normally long base64 code filling the window. If the base64 part could be folded then I would see no problem. ui-r- only for the Insert HTML case.

Comment on attachment 8742696 [details] Screenshot showing the data URI I'm opposing It's hard for me to tell all the times that you'd get data: URLs. If I add an inline image while editing, is it a file: URL until I click send? If I reply to a message, is it a cid:* URL? I don't think I can say whether this is ok or not without knowing the exact scenarios where we'd start using a data: URL. * I think it's cid: for the multipart/related messages.

(In reply to Jim Porter (:squib) from comment #77) > It's hard for me to tell all the times that you'd get data: URLs. If I add > an inline image while editing, is it a file: URL until I click send? If I > reply to a message, is it a cid:* URL? > > I don't think I can say whether this is ok or not without knowing the exact > scenarios where we'd start using a data: URL. With Magnus approach, no file URL would ever be used any more: 1) Inline pictures added while editing would be immediately converted to data URIs, (see attachment 8742696 [details]). 2) Pictures from signatures would be converted to URIs when inserting the signature. 3) Pictures pasted from a file:// based web page would also be converted to data URIs, although Magnus has not provided a way to do this yet. In short: The whole HTML would be polluted with data URIs everywhere. That renders the image properties dialogue useless (imagine someone editing the data string), it renders "Insert HTML" useless and it renders all add-ons that offer HTML editing (most notably "Stationary") useless. From a UI perspective this is a catastrophe.

Ok yeah, that's bad. I think the only thing we should be changing is what happens when you paste HTML. While other attack vectors may exist, they're so close to valid use cases that we'd have to be *extremely careful* about changing them.

Sorry to repeat it again: Are are four ways to ship out HTML that would drag local files with it: 1) Reply 2) Forward 3) Edit as New 4) Paste from the attackers e-mail. 1) and 2) are not an issue since they are already treated in TagEmbeddedObjects(). Pasting is taken care of by sanitising, that is, no longer being a APP_TYPE_EDITOR, and switching the default to not send the file unless explicitly asked for. So far Magnus' and my approach are the same. I'd like to create a new app type APP_TYPE_EDITOR_MAIL which would still allow image insertion from a file via M-C code, therefore maintaining the current UI, whereas Magnus doesn't want to rely on M-C code and convert all images to data URIs immediately, therefore damaging the current UI. If we all agreed on the way forward, we could put a safe and working solution together in a day, we have all the pieces required.

Do you know what happens with drafts? When we save a draft, we convert from file: URLs to attachments (possibly with cid: URLs), correct? No valid draft should have a file: URL, right? If that's the case, then it makes sense to sanitize messages and strip file: URLs every time we edit as new. This probably has some ramifications on bug 877159, although I haven't thought them all through. In any case, I agree that data: URLs seem like the wrong way to go here. Unless there's really no other way to resolve this, I think we should avoid them.

Drafts maintain the file:// URL. The conversion to cid: happens when we send. Note that "edit draft" and "edit as new" are two different actions these days. I haven't looked into the "edit as new" of an attacker's message yet. My assumption so far is that I will do the same processing as for forward and reply.

(In reply to Jorg K (GMT+2) from comment #82) > Drafts maintain the file:// URL. The conversion to cid: happens when we send. I double-checked on 45.0. That's not the behavior I see. I inserted an image into an HTML mail via the toolbar button, saved the draft, and viewed the source in the reader. I see a cid: URL. When I edit the draft, I see the URL has changed to an imap: URL instead (or a mailbox: URL for Local Folders). I did notice that, at least on Windows, dragging and dropping an image into the body of an HTML mail already gives us a data: URL...

OK, I tested a different case. I copied a picture from a file:// based web page: <html> <body> <img moz-do-not-send="true" src="file://D:/Desktop/Bugzilla-down.png"> </body> </html> The file:// URL is maintained in the draft. Yes, dragging an image already produces a data URI. But we don't need to make things worse, right?

That behavior definitely seems wrong. Doesn't moz-do-not-send mean that the src attribute will be sent unchanged? I wouldn't expect that behavior as a user.

There seems to be a misunderstanding. The HTML above shows the web page I copied from. I added various attributes to check the new sanitisation upon paste. moz-do-not-send=true means: Do not send this attached. moz-do-not-send=false means: Send this attached. The attack vector was to send invisible content with moz-do-not-send=false, which, when pointing to a known location on the user's machine, would access files and sent them as attachments when copied to another message. In the future it will default to true, so nothing gets sent unless explicitly asked for. Sanitisation will remove any pre-existing values during copy/paste, so the new default value will be used and nothing will get sent. Where the user explicitly inserts an image (also via a signature), we will set moz-do-not-send=false to trigger attaching. What do you mean by: "the src attribute will be sent unchanged?"

(In reply to Jorg K (GMT+2) from comment #86) > What do you mean by: "the src attribute will be sent unchanged?" If you have "<img moz-do-not-send="true" src="file://D:/Desktop/Bugzilla-down.png">" in an email message, I thought the moz-do-not-send="true" attribute means, "don't do anything to the src URL; just send it as-is (i.e. *don't* attach the src and make a cid: URL)." Is that wrong?

Just a few quick notes here: 1) we're not a web page editor 2) the attack vector of this bug is dual - A) not removing moz-do-not-send and also B) by default attaching when that's not present at all There's really not a solid solution around using data uris. You'll just play wack-a-mole, because is we use the editor as an editor it's a TRUSTED editor. We're not (going to), so we must play by the rules of the web, which means no file:// access. If you try to cheat you open an attack vector.

In that case, shouldn't we just create cid: URLs immediately? That's what we want to send anyway.

(In reply to Jim Porter (:squib) from comment #87) > If you have "<img moz-do-not-send="true" > src="file://D:/Desktop/Bugzilla-down.png">" in an email message, I thought > the moz-do-not-send="true" attribute means, "don't do anything to the src > URL; just send it as-is (i.e. *don't* attach the src and make a cid: URL)." > Is that wrong? What you said is correct. (And I still don't understand your comment #85.) moz-do-not-send is normally for non-file URLs, so the sender can decide whether or not they want to attach an image. For example, people who have a 30KB image in their signature don't want to attach it to every message, so they have <img moz-do-not-send="true" src="http://mysite.com/images/sig-pic.png"> in their signature. (In reply to Jim Porter (:squib) from comment #89) > In that case, shouldn't we just create cid: URLs immediately? That's what we > want to send anyway. Sadly that doesn't work since we have no where to get the image from as long as the draft isn't saved.

(In reply to Jorg K (GMT+2) from comment #90) > (In reply to Jim Porter (:squib) from comment #89) > > In that case, shouldn't we just create cid: URLs immediately? That's what we > > want to send anyway. > Sadly that doesn't work since we have no where to get the image from as long > as the draft isn't saved. Can't we store a reference to it in memory? Even if they're not real cid: URLs, we could create some kind of a mapping to an internal list of files that we know the user intended to attach.

(In reply to Magnus Melin from comment #88) > Just a few quick notes here: > 1) we're not a web page editor Some people (and may add-ons) use the compose window as an HTML editor. > 2) the attack vector of this bug is dual - A) not removing moz-do-not-send > and also B) by default attaching when that's not present at all And those will be fixed in both approaches. > There's really not a solid solution around using data uris. You'll just play > wack-a-mole, because is we use the editor as an editor it's a TRUSTED > editor. We're not (going to), so we must play by the rules of the web, which > means no file:// access. If you try to cheat you open an attack vector. I disagree. Of course always using data URIs solves the problem, but I'm not convinced that the problems can't be fixed any other way. Defaulting to "don't send" will just not send unless it's one of the exceptions cases: 1) Signature 2) User inserted image via 2a) direct insertion or 2b) paste of single image. As Jim said in comment #79: We have to be very careful not to affect valid use cases. A "therapia sterilisans magna" is not the right way here. <off topic> There is just that much change users will wear. We've spent man weeks containing the "correspondents column debacle" just because someone (not me!! I implemented the auto-upgrade-opt-out) decided to force a new unbaked feature onto the users. </off topic>

(In reply to Jim Porter (:squib) from comment #91) > Can't we store a reference to it in memory? Even if they're not real cid: > URLs, we could create some kind of a mapping to an internal list of files > that we know the user intended to attach. The M-C editor that rules inside the <div contenteditable> that we're using for HTML editing doesn't support that. However, Ehsan suggested something along those lines (comment #62) (quoting) === On the question of whether it's fine to show a data URI in the UI, I personally don't care much either way. If it's just the string in the UI, perhaps upon pasting you could remember the original URI somewhere (preferably outside of the DOM being edited) and use it in the UI when needed? === However, I still don't see why a solution that carefully manages sending from files can't work.

(In reply to Jorg K (GMT+2) from comment #93) > (In reply to Jim Porter (:squib) from comment #91) > > Can't we store a reference to it in memory? Even if they're not real cid: > > URLs, we could create some kind of a mapping to an internal list of files > > that we know the user intended to attach. > The M-C editor that rules inside the <div contenteditable> that we're using > for HTML editing doesn't support that. It supports other protocols we control, like imap: and mailbox:. Couldn't we create a pendingattachment: protocol that we hook up in the appropriate places?

(In reply to Jim Porter (:squib) from comment #94) > It supports other protocols we control, like imap: and mailbox:. Couldn't we > create a pendingattachment: protocol that we hook up in the appropriate > places? Why so much effort? Can someone clearly state why the solution I set out in comment #73 at the bottom doesn't work or is for other (good) reasons undesired? (I don't count the "wack-a-mole" argument as a good reason.)

In comment 59 Jorgk asked for my opinion. I'm following the discussion, but for now I feel like Magnus, Jorg, Ricahrd, and Jim are still working toward the right solution. If there is an impasse I will try to help resolve it, but I'd rather let these people work it out.

Comment on attachment 8742665 [details] [diff] [review] JK C-C part: Paste listener to attach moz-do-not-sent="false" to single pasted images Review of attachment 8742665 [details] [diff] [review]: ----------------------------------------------------------------- This is also not fixing the problem. It just requires the attacker to put html content with a fake file:/// image on your clipboard instead, which is not that much harder. You can't win here...

(In reply to Jorg K (PTO during summer, NI me) from comment #95) > (In reply to Jim Porter (:squib) from comment #94) > > It supports other protocols we control, like imap: and mailbox:. Couldn't we > > create a pendingattachment: protocol that we hook up in the appropriate > > places? > Why so much effort? I certainly agree there. We already *have* data uris. There's no reason to invent the wheel again. > Can someone clearly state why the solution I set out in comment #73 at the > bottom doesn't work or is for other (good) reasons undesired? (I don't count > the "wack-a-mole" argument as a good reason.) I did so in comment 97. But you really should count the "wack-a-mole" argument as a good reason too.

Changes from earlier: Makes the insert HTML dialog use short data uris (very similar to ff devtools inspector) during editing. With this I believe this is ui-r+ per comment 76.

Are you going to propose a solution for the copying of an image from file:// based web page? As per comment #47 the proposed M-C change came back with a test failure.

Yes, once we can agree on the mailnews part. The test just needs to be adjusted for a different expected result.

I gave the new patch a quick whirl. Here's a screenshot of the "Insert HTML" windows with the, well, shortened (not wrapped into a clickable node) string. The user can of course edit that string and destroy the image. They can also edit the data URI in the image properties and destroy the image. The argument has been that data URIs already happen when an image is dragged into a composition, so perhaps it's about time we disable the editing of those data URIs. Also, I found another hole in your approach: If in "Insert HTML" you insert, for example, my test case, file:///D:/Desktop/Bugzilla-down.png, the image is not displayed. You've have to reach in there as well and replace the image with a data URI. Let me summarise: You have to change processing in - image insertion - signature processing - HTML insertion (still to do) - pasting from a file:// based web page (still to do) (and I'm not sure how willing the M-C people are going to be to wear those changes).

(In reply to Magnus Melin from comment #97) > Comment on attachment 8742665 [details] [diff] [review] > JK C-C part: Paste listener to attach moz-do-not-sent="false" to single > pasted images > > This is also not fixing the problem. It just requires the attacker to put > html content with a fake file:/// image on your clipboard instead, which is > not that much harder. Can you explain to me how the attacker would do that? This code I proposed inspects the text/html flavour of the clipboard at paste time and if it contains a single image, it inserts that with "moz-do-not-send"=false. So the attacker needs to send HTML e-mail that contains <img src="file://file-steal.ext"> in some invisible form. He needs to get the victim to select this one piece of invisible HTML and then paste it. OK, that would have to be a pretty skilled attacker and, more importantly, also a pretty skilled victim to select just that piece of invisible data and paste it. This clipboard inspection was WIP just to show the concept. It can be tightened to ensure that the only visible images would be inserted with "moz-do-not-send"=false. I would argue that if the user actively pastes something and the image pops up in front of him, there is no harm in shipping that out. BTW, I've written this piece of code so you could use it for your solution with the data URIs if M-C won't permit the changes you still haven't specified. You can grab the image and convert it to data URI in the paste handler. (In reply to Magnus Melin from comment #101) > Yes, once we can agree on the mailnews part. The test just needs to be > adjusted for a different expected result. I don't think M-C will agree on behaviour changes in FF or any test change. Therefore, I'd like to see the whole solution before signing off on any of the parts. IMHO it would make more sense to settle the M-C part first or it least start the negotiation.

Comment on attachment 8751444 [details] [diff] [review] bug1151366_sec_file_attach.patch (No offense intended. I just don't like the red indicator in BMO telling me I have to do something. I've looked at the patch and found another hole and some UI issues, see comment #102.)

Comment on attachment 8751502 [details] Insert HTML view It's already better than before. But it's still possible to edit the data content by accident. Also that's not possible to insert a file:// link is not desired.

Yes, of course it's possible to edit data content by accident - you can also edit/insert whatever other invalid html you want, and there's not a thing we can do about that. You'll find out soon enough though when you see what your html looked like. It would not be that hard to convert file:// sources to data if they were present in the Insert HTML dialog. I did at first agree, but thinking about it some more I think we should not: 1) we're not a web page editor 2) that's an unnecessary complication, it's not like something people have grown accustomed to 3) when you're inserting plain html you don't expect what you write to change, you may want to for instance compose a test cased for this bug 4) inserting local pictures by Insert HTML is likely not very used, I don't know why you would do that - it's highly error prone. You're really likely better off linking to http sources 5) the current patch would be welcome behaviour for the editor also. Yes it's possible to add an override but it's all over-engineering the whole feature.

You are correct on two things: - data URI already exist due to drag&drop. - Editing HTLM manually can of course introduce errors, in the HTML and any data URIs contained in it. I don't agree with the rest you're saying or I don't understand it: > 1) we're not a web page editor We have a function to edit HTML, that function works and there is no apparent reason to break it. > 2) that's an unnecessary complication, it's not like something people have grown accustomed to We don't know how many people will be affected. You plan to silently break existing functionality. > 3) when you're inserting plain html you don't expect what you write to change, > you may want to for instance compose a test cased for this bug Well, when you insert a picture from file://mypic.jpg you don't expect to find a data URL in the image properties when you visit it again. But you force this upon the user. Surely, if the user inserts an image they want to see it in their e-mail composition and not some placeholder indicating a broken image. > 4) inserting local pictures by Insert HTML is likely not very used, I don't know > why you would do that - it's highly error prone. You're really likely better off > linking to http sources Here you're repeating point 3). It's an existing function and we shouldn't break it. > 5) the current patch would be welcome behaviour for the editor also. Yes it's possible > to add an override but it's all over-engineering the whole feature. Which editor? The M-C editor? It prefers data URIs? The second part I don't understand. Which "over-engineering"? You're proposing a huge change to the way images are handled, so of course there is some engineering involved. If we go with data URIs, we need to do it fully. That includes: - image insertion. Here I'd really like to see some (image data) place holder and not the base64 string (still to do). - signature processing (done) - Showing data URIs in the HTML window: Done, but I'd prefer not to see any of the base64 stuff, src=(image data) should be enough, same goes for the image properties (see above). - HTML insertion (still to do) - pasting from a file:// based web page, either with an M-C change or the paste listener (still to do) Whilst the TB compose window is not strictly a HTML editor, we know that people use a whole lot of HTML. There are add-ons that do special template processing or formatting of the message body ("Stationery", "SmartTemplate4"). Consequently there are also add-ons which provide HTML editing facilities ("Stationery", "ThunderHTMLedit"). These add-ons will just have to work much harder to cope with the data URIs. TB core at least should also fix all the various corners that break due to the adoption of data URIs. Since this won't land on TB 45.x, we still have months to do it properly for TB 52.

(In reply to Jorg K (PTO during summer, NI me) from comment #107) > - Showing data URIs in the HTML window: Done, but I'd prefer not to see any > of the base64 stuff, > src=(image data) should be enough, same goes for the image properties (see > above). Meanwhile I implemented the src="(image data nn)" in my add-on: https://addons.mozilla.org/en-us/thunderbird/addon/thunderhtmledit/

There hasn't been much progress here lately, so let me make a suggestion. Let's go with the data URIs and finish it off properly as per comment #107: - Image insertion. Would it be possible to show an (image data) place holder and not the base64 string in image properties/image location? (still to do) - Signature processing (done) - Showing data URIs in the HTML window: Done, but I'd prefer not to see any of the base64 stuff, src=(image data) should be enough, same goes for the image properties (see above). - HTML insertion of an image in the HTML windows also needs to convert to data URI (still to do) - Pasting from a file:// based web page, either with an M-C change or the *paste listener*. I suggest to use attachment 8742665 [details] [diff] [review] to detect a single image insertion and turn the image into a data URI (still to do).

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #103) > > This is also not fixing the problem. It just requires the attacker to put > > html content with a fake file:/// image on your clipboard instead, which is > > not that much harder. > > Can you explain to me how the attacker would do that? Try this... it's using an http url just since it's easier to see.

You've been doing too much web work recently. Not sure what you try to prove here. Sure, if I view that web page, select the agadfadfadsf and paste into a TB composition, I get the image pasted. And I see it. And so will the user. After the copy is done, there is an image on the clipboard, and that the user should be able to copy. But your example doesn't work if the source is an e-mail, at least I tried, and I pasted agadfadfadsf. Do we run JS when displaying a message?

Try this. I pastes "agadfadfadsf". Am I missing something? If there is a HTML for a single image on the clipboard, we should insert the image into the composition in order not to affect current usage.

I'm just proving that your suggested approach of looking for a single image on the clipboard does not really work, since we can easily put a file "image" in there. Yes it requires JavaScript, but the whole exploit is socially engineered. Instead of copying a bait straight from the mail, you just put the bait you want the user to send to you on a web page and have them go copy it from there. It's one more step, but hardly far fetched. Or you put it in a news feed, we do run JavaScript there.

And how do you intend to distinguish "baited" clipboard content from user-intended clipboard content? We can't stop image insertion in general. As Jim said: (In reply to Jim Porter (:squib) from comment #79) > Ok yeah, that's bad. I think the only thing we should be changing is what > happens when you paste HTML. While other attack vectors may exist, they're > so close to valid use cases that we'd have to be *extremely careful* about > changing them. There is a fine line between closing a security hole and damaging the product for everyone. If there is HTML for an image on the clipboard, then the image should get pasted IHMO. If the attacker wants to extract compromising images from the victims machine, they know where they are stored and they manage to get them onto the clipboard and they get the victim to paste that into a message and then *SEES* the compromising image and presses send, well, then we can't help this.

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #114) > And how do you intend to distinguish "baited" clipboard content from > user-intended clipboard content? The thing is we don't have to. If we don't use file:// uris while pasting there is no real problem. > We can't stop image insertion in general. Of course not, just make it safe.

(In reply to Magnus Melin from comment #115) > The thing is we don't have to. If we don't use file:// uris while pasting > there is no real problem. The user may paste from a file:// based web page and it would be good not to break that. === The following argument against stop being an APP_TYPE_EDITOR of some sort (I suggested a new type APP_TYPE_EDITOR_MAIL) occurred to me: We will break all add-ons which insert HTML with file:// based images. The first add-on which will break is the popular "Signature Switch" which I use myself. That rips out the signature and places a new one, most likely with some images from files. And the users will be very surprised if the images don't show any more. I'm CC'ing Axel on this bug. Here's the executive summary for Axel: One option to fix the security issues in this bug would for the TB compose window to cease being an APP_TYPE_EDITOR. The immediate consequence is that <img src="file:// ...> would not be displayed any more. Magnus suggestion is to convert those images to <img src="data:image/png;base64, ...>. We would do this when inserting for example images from signatures. However, there are add-ons which change the body of an e-mail and if those add-ons were to place <img src="file:// ...> into the body, the image wouldn't be displayed any more. Axel, what do you day? Your SmartTemplate4 adds HTML, potentially with those images.

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #116) > The user may paste from a file:// based web page and it would be good not to > break that. My suggested approach does not break that. Add-ons may need to adapt, but the changes are not substantial. I don't see the point of discussing this further, I've already proved the alternative is not safe.

OK, you want to stop the discussion and so do I. How about to answer what I have asked (a felt) million times now. Let me summarise (again) the problems introduced by your approach. 1) Manual image insertion. As before already with dragged images, that will now show "data:image/png;base64 ..." more frequently in the UI in the image properties dialogue. Would it be possible to show an (image data) place holder and not the base64 string in image properties/image location? ** Do you plan more action on this item? ** 2) Signature processing. Here you convert any file:// based signature content into a data URI. Maybe it would be good to expose this function for add-ons to call, see below. Or maybe "GenerateDataURI(url)" is that function. ** Do you plan more action on this item? ** 3) Showing data URIs in the HTML window: You proposed to shorten the data URI shown. I'd prefer not to see any of the base64 stuff. src="(image data)" (as I do in ThunderHTMLedit) should be enough, same goes for the image properties (see point 1). ** Do you plan more action on this item? ** 4) HTML insertion of an image in the HTML windows also needs to convert to data URI: If the user manually adds a file:// based image in the HTML window, the image won't show. ** Do you plan more action on this item? ** 5) Pasting from a file:// based web page. You said in comment #117 that your "suggested approach" doesn't break that. Sadly, it is not clear to me, what the suggested approach is. Maybe the M-C change to switch the clipboard flavours around. That doesn't have a complete patch (the one M-C patch presented has test failures). ** What is the "suggested approach"? If it's an M-C change, when will you present this for review with the M-C people? ** 6) Add-ons which insert HTML with file:// based images, as for example "Signature Switch": Similar to point 4), those images won't show and therefore the add-on functionality will be broken. ** How will you communicate this to the add-on authors? IMHO (quote): "Add-ons may need to adapt, but the changes are not substantial." is misleading and a clear understatement. Add-ons *WILL BREAK* and changing processing to extract the file:// based image and convert them to data URIs is not a completely simple task. However, if you set a good example for point 4), I'm sure add-on authors could adapt the TB changes to their needs. You could even provide a function to call. I'd also have to make the change in my ThunderHTMLedit. (Further discussion, see below: (*)). On *all* items I am still unclear whether you will add a better solution or what the solution is in the first place. So let's stop discussing an nail down the solution, implement it, review it and get it landed. === *) A little off topic: The experience with Tb 45.x has shown that there are many negligent add-on authors who don't test their add-ons with alpha/beta versions in order to adapt them. So the breakage and outcry happens on release of TB 52. I haven't counted the endless list of bugs caused by incompatible add-ons in TB 45.

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #116) > (In reply to Magnus Melin from comment #115) > > The thing is we don't have to. If we don't use file:// uris while pasting > > there is no real problem. > The user may paste from a file:// based web page and it would be good not to > break that. > > === > > The following argument against stop being an APP_TYPE_EDITOR of some sort (I > suggested a new type APP_TYPE_EDITOR_MAIL) occurred to me: We will break all > add-ons which insert HTML with file:// based images. The first add-on which > will break is the popular "Signature Switch" which I use myself. That rips > out the signature and places a new one, most likely with some images from > files. And the users will be very surprised if the images don't show any > more. > > I'm CC'ing Axel on this bug. Here's the executive summary for Axel: > > One option to fix the security issues in this bug would for the TB compose > window to cease being an APP_TYPE_EDITOR. The immediate consequence is that > <img src="file:// ...> would not be displayed any more. Magnus suggestion is > to convert those images to <img src="data:image/png;base64, ...>. We would > do this when inserting for example images from signatures. that would work for me - see special cases in my example below. > > However, there are add-ons which change the body of an e-mail and if those > add-ons were to place <img src="file:// ...> into the body, the image > wouldn't be displayed any more. Yes, ST4 does this and Stationery as well. Doesn't it depend on _when_ the conversion is done? If the security issue only applies to quoted emails (replying to an email with an attack vector <img> tag) could the conversion be restricted to <blockquote> only? > > Axel, what do you day? Your SmartTemplate4 adds HTML, potentially with those > images. My questions are: 1) are the img tags not expanded when loading composer or editing the tags in HTML view? - if you are planning on writing a conversion function we could potentially call it as part of our workflow by parsing just the template (we never modify the quoted block anyway). I have several mail templates that use tags like the below in order to inject some icons: <img src="file:///E:/Dev/Mozilla/DEV/QF/_Mozdev/quickfolders/www/ico/QF16x16.png" style="float:left;margin-right:1em;"> These are injected via Stationery - I am okay with automatic conversion to inline pictures as it doesn't present any additional problems in WYSIWYG mode. I think Joerg's ThunderHTMLEdit extension already makes the code more manageable in HTML by collapsing the hex-data according to his suggestion. 2) where conversion fails, could we add a (right-click menu) UI to the broken image icon for manually fixing / browsing to the approximate location of the broken file link? 3) we currently still have a bug that leads to invalid picture data (my guess is that it references a mail:// location which can break when mails are moved to other folders via filter rules) and stops the email from being sent out ("attaching files...") - is there a bug for this one and what can be done to alleviate this? Generally it is very hard to find invalid reference links like this while in WYSIWYG composer and often the only solution is deleting all inline pictures (cut and paste sometimes also works if I am not mistaken). Not sure what to do about mail references, but a tidy up option (remove all broken links) to force sending would be fantastic. 4) Clipboard Behavior: I think when cutting a (displayed) picture that comes from a (mailbox / file) reference, it should automatically be expanded to the raw picture data when pasting it back. Some of these suggestions may involve creating new (or linking existing other) bugs.

Axel, thanks for your input. We know now that we need to keep add-ons in mind which place file:// based images. Also, I think we haven't considered, or at least, I haven't considered, images pasted via mailbox references, like I've just tried pasting one of those and see this using ThunderHTMLedit: <img src="mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Sent.sbd/Test%20attaching?number=4&amp;part=1.2&amp;filename=lijbmghmkilicioj.png" ... We have to ensure that the solution works for those images. As for the "Attaching ...": You may not be aware that we landed bug 453196 which greatly improves the error handling in these cases. But we all agree, converting links to data URI is the way to completely avoid these problems.

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #120) > Axel, thanks for your input. We know now that we need to keep add-ons in > mind which place file:// based images. > > Also, I think we haven't considered, or at least, I haven't considered, > images pasted via mailbox references, like I've just tried pasting one of > those and see this using ThunderHTMLedit: > <img > src="mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Sent. > sbd/Test%20attaching?number=4&amp;part=1.2&amp;filename=lijbmghmkilicioj. > png" ... yes generally it would be a good idea to convert these ones to data as soon as they hit another email, as it is doubtful that they can be tracked and updated in a meaningful way when users draft replies and then move the original thread. Happens to me plenty :)

There hasn't been a lot of attention to this bug lately. Working on bug 882104 this occurred to me: Further to comment #118, here another summary of how HTML with file:// based images can get into the composition: 1) Manual image insertion. 2) Signatures 3) (this was about the ugly data URIs). 4) HTML insertion of an image in the HTML window and add-ons that let you add HTML. 5) Pasting from a file:// based web page. 6) Add-ons which insert HTML with file:// based images. 7) *New*: Including a HTML file with message= compose switch. The list has just become one item longer and I don't know how many more we've missed. I think we're overcomplicating the system and losing editing capability just for the sake of plugging a hole where I'm still not sure that no other solution exists. We could do the solution I outline in comment #49 / comment #73: Create an app type APP_TYPE_MAIL_EDITOR that still allows *viewing* for file:// bases images, do full sanitisation, switch the default to do-not-sent=true, and here's a new idea: When the user pastes and image from a file:// bases source, insert the image and show it and display a message: You've just pasted an image. Thunderbird has inserted the image into the composition, but to protect your privacy, the image will not be sent unless you change the 'Attach this image to the message' option in the image properties. How's that?

Thinking about this a little further, this could be: You've just pasted nn image(s). Thunderbird has inserted the image(s) into the composition, but to protect your privacy, the image(s) will not be sent unless the option 'Attach this image to the message' in the image properties is set (for each pasted image). [Attach image(s)] [Do not attach image(s)] (Note the plural form of the message). This fixes the problem and even notifies the user that something potentially fishy is going on instead of turning the whole system upside down.

I'll be presenting my solution here in the next couple of days to get some movement here. This is after all a security problem that we should fix one day. Here is the M-C patch. New app type APP_TYPE_MAIL_EDITOR, what it does is explained in the IDL.

Oops, there have been some changes in M-C, so the previous patch doesn't work any more. This one does.

Sadly, this bug has stalled. However, this is a real security issue, so we should fix it one day, preferably in TB 52. This my suggestion for a the basic fix. We become a APP_TYPE_MAIL_EDITOR and therefore only receive sanitised paste content, that means, the moz-do-not-send attribute is stripped. We change the default around, to anything without moz-do-not-send=false doesn't get sent. We alert the user when they paste file:// based images. Optional work: - Treat images in signatures as if moz-do-not-send=false were set (so users don't have to manually change their signatures) - On paste, instead of showing the alert, offer the option to set moz-do-not-send=false (see comment #123).

(sorry about the noise, break was at wrong level.)

(In reply to Jorg K (GMT+2) from comment #126) > Optional work: > - Treat images in signatures as if moz-do-not-send=false were set > (so users don't have to manually change their signatures) Actually, signatures with a single image don't work right now since the image does not have moz-do-not-send=false. So that's not optional an needs to be fixed. That would be an easy fix in the signature processing. However, the aim of presenting the patches is to get agreement on the general direction.

Sorry about the noise. I fixed the single image signature case since I had advertised a *working* solution. Now the optional work is this: - Treat images in HTML signatures as if moz-do-not-send=false were set (so users don't have to manually change their HTML signatures) - On paste, instead of showing the alert, offer the option to set moz-do-not-send=false (see comment #123).

Sorry about further noise ;-) This is no longer "minimal" as I "stole" and adapted a hunk from Magnus' patch to add moz-do-not-send=false in signature file:// URLs so users don't even notice the change. All the files referenced in the signature will now be sent. So the optional further work is reduced to: - On paste, instead of showing the alert, offer the option to set moz-do-not-send=false (see comment #123).

More noise ;-) I noticed that the image insertion also needed some tweaks. Most of this was stolen from Magnus' patch ;-) Now if you insert an image from a file, the panel proposes to attach the image and sets moz-do-not-send=false on it. For a pasted image with no attributes, this is also the proposition. After the alert message the user just has to accept when checking the image properties.

(In reply to Jorg K (GMT+2) from comment #131) > For a pasted image with no attributes, this is also the proposition. I could change this, so pasted images show the panel with the box unticked so the user has to explicitly say: Yes, I want to attach this image. (It should just be a matter of checking the URL since it will be empty for new images and populated for paster images.) Opinions?

I've now spent way too much time on this. Let me give an opinion. Although I have no fundamental objection to a plan that requires us to use data uris instead of file uris in messages, I think that we need to give a lot of thought to the migration process for that, which we simply do not have the time or energy for at the moment. So I think it is premature to attempt that in this bug. Mitigation might involve for example an entire release cycle where this is controlled by a hidden option to discover issues as a workaround for awhile for affected users. So I think at the moment the correct mitigation strategy is something along the lines of comment 132 and its predecessors, which involves warning the user when a file is being inserted into a composed message and requiring them to click a non-default checkbox to opt-in to attaching the file. Normally in security, prevailing wisdom says that users will simply ignore warnings, and that is a risk. But I also think the whole security issue in this bug is already a second-order issue since it requires some social engineering and manual actions on the part of the user. A warning that their actions will result in an attached file (which is unexpected), along with an opt-in, should be sufficient. It has the downside that it introduces additional friction into the operation of file insertion, perhaps we can figure out a way to avoid that extra friction when we know the user is opting in.

Comment on attachment 8796983 [details] [diff] [review] JK C-C part: WIP presented for initial feedback (v5). Review of attachment 8796983 [details] [diff] [review]: ----------------------------------------------------------------- I had a couple of small comments, but did not attempt to review the code details. But I think that a warning to the user when files are going to be sent is the right approach. ::: mail/components/compose/content/MsgComposeCommands.js @@ +2353,5 @@ > > + // Add a listener for paste, so we can tag pasted images. > + // False: dispatch to us last, so we get called once the paste has happened. > + // Note: Since we are a APP_TYPE_MAIL_EDITOR, we only receive sanitised content. > + document.addEventListener("paste", pasteListener, false); What happen with drag and drop, which also inserts html content? ::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties @@ +393,5 @@ > replaceButton.accesskey=l > replaceButton.tooltip=Show the Find and Replace dialog > + > +imagePasteFromFileTitle=Paste into compose window > +imagePasteFromFileMessage=You've just pasted an image referencing a file. Thunderbird has inserted the image into the composition, but to protect your privacy, the image will not be sent unless you change the 'Attach this image to the message' option in the image properties. This message is way too long. When the user gets the warning, they need to see 1) the file name, and 2) have the choice immediately of yes or no without having to search for some Thunderbird feature they probably have never used. Maybe just "Include the content of <the file name> when sending this message?"

Comment on attachment 8796645 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR (v3). Review of attachment 8796645 [details] [diff] [review]: ----------------------------------------------------------------- I did not review this code, but I have no objection to having a specialized editor type.

(In reply to Kent James (:rkent) from comment #134) > This message is way too long. When the user gets the warning, they need to > see 1) the file name, and 2) have the choice immediately of yes or no > without having to search for some Thunderbird feature they probably have > never used. > > Maybe just "Include the content of <the file name> when sending this > message?" Actually I this part of the patch is more or less correct. The pasted content may include multiple images, some of them file:// so it's trivial to trick someone to just "click ok to get the job done" if you automate any of the steps. But it all just goes to show, it's a plan B solution if anything.

Thanks for the comments. A bit of a mid-air collision here, but I submit my comments anyway. When the user uses the image insertion dialogue, nothing will change, "Attach image" will be preselected. I would think that it is rather uncommon to *paste* a file-based image. So the extra friction should be acceptable. Besides, we have no way to decide whether a user-initiated paste including HTML with a file-based image is voluntary or the effect of an attack. I don't see how data URIs help us at all in the paste case, which is what this bug is about. In Magnus' approach, pasting HTML with a file-based image is simply not showing the image which is not what the user expects. Magnus hasn't addressed the paste aspect. As far as I know, he was going to try to change the order clipboard flavours are processed in M-C to prefer a bitmap for pasting, but that you only work if a single image were on the clipboard and not a longer HTML fragment including an image. How do we proceed here? Shall I ask for review for the M-C part? Shall I bring the C-C part to review quality? So according to comment #133 Kent agrees with comment #132 that in case of pasted images, the "Attach image" should *not* be preselected unlike for new image insertions? And in comment #134 he opts for a "just do it button". Which one is it? Addressing comment #134: Perhaps we could shorten the message, but I would advise *against* a "just do it button". The user should need to visit his image or images and give the OK. And comment #136: Sorry, but after all these months of looking at it, I still don't know what the "Plan A" solution is and how data URIs can help us with paste.

OK suggestion was: You've just pasted an image referencing a file. Thunderbird has inserted the image into the composition, but to protect your privacy, the image will not be sent unless you change the 'Attach this image to the message' option in the image properties. How about: To attach the pasted image referencing a file to the message, set 'Attach this image to the message' in the image properties. Or: Set 'Attach this image to the message' in the image properties to send the pasted image as part of the message.

Can Magnus and Kent please decide whether they want a "just do it button" or let the user visit the image "manually"? And perhaps Magnus can explain "Plan A" regarding paste and drag and drop, see below. And I forgot to answer: > What happen with drag and drop, which also inserts html content? Drag and drop of a single image inserts a data URI, try it now. Drag and drop of a larger fragment inserts the HTML of the fragment including any contained file-based image. Looks like I need another listener and I need to check whether dragged content is sanitised. Damn, I never tried dragging a larger segment.

"set 'Attach this image to the message' in the image properties." As a typical user, I have no idea what the image properties are. Also in some cases the images are completely hidden, right, so how do you select the image properties? The "just do it" steps I would suggest are a dialog that has two choices "Attach file" and "Don't attach file", plus an Accept. So to accept is two steps, press "Attach File" then "Accept. This is meant for a case where the user was not expecting to be attaching a file, and attaching a file is something they understand.

For me, drag an drop of an image inserts an invalid data uri. But that is probably another bug.

(In reply to Jorg K (GMT+2) from comment #138) > OK suggestion was: > You've just pasted an image referencing a file. Thunderbird has inserted the > image into the composition, but to protect your privacy, the image will not > be sent unless you change the 'Attach this image to the message' option in > the image properties. > > How about: > To attach the pasted image referencing a file to the message, set 'Attach > this image to the message' in the image properties. > > Or: > Set 'Attach this image to the message' in the image properties to send the > pasted image as part of the message. I believe it is critical that information on the file (at least its name) be given in the error message. You have to assume that the user has no idea what "image properties" are when they just wanted to insert an image, so I really dislike that. If you want them do do something, popup the dialog that allows it. I also do not think you can assume this is rare. This might be a standard part of some user's workflow, and the extra friction is going to be annoying.

(In reply to Kent James (:rkent) from comment #140) > As a typical user, I have no idea what the image properties are. Also in > some cases the images are completely hidden, right, so how do you select the > image properties? They could be hidden by CSS, but please remember, this bug is not about images, it is about extracting whatever document as part of an <img src=...>. So if the attacker wants to extract the list of the people you're bribing, this list will not display as an image. > The "just do it" steps I would suggest are a dialog that has two choices > "Attach file" and "Don't attach file", plus an Accept. So to accept is two > steps, press "Attach File" then "Accept. This is meant for a case where the > user was not expecting to be attaching a file, and attaching a file is > something they understand. Typically these dialogues have buttons, "Do it" and "Cancel", not radio buttons and "Accept". But anything is doable. We could certainly list all the files (plural) here: "Include the content of <the file names> when sending this message?" As for drag & drop of an image: Hmm, try dragging my logo here to a TB message. That works fine now and always has.

(In reply to Jorg K (GMT+2) from comment #143) > Typically these dialogues have buttons, "Do it" and "Cancel", not radio > buttons and "Accept". But anything is doable. How about a panel like this: === Include the content the following images when sending this message? [ ] lover-at-beach.jpg [ ] compromising-image.png [Accept] [Cancel] === We should only list images that M-C supports in rendering, so jpg/jpeg, png, gif (see kPNGImageMime, kJPEGImageMime, kJPGImageMime) and not the other documents like doc, pdf, odt, etc. There is also kNativeImageMime, and at least on Windows, FF renders BMP images.

In this patch: - Handle drag and drop as well as paste. The good news is that dropped content also arrives sanitised. - Changed image insertion so that for pasted images the attach checkbox is *not* automatically ticked (see comment #132). Next up: Dig out the image names from the pasted/dropped HTML fragment and present them to the user in a nice panel to tick and accept (see comment #144).

Comment on attachment 8796645 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR (v3). After some deliberation we'd like to introduce a "mail editor" app type as suggested by Daniel Glazman in comment #21. Its properties are described briefly in the IDL. Could you please review the minimal change this entails. Thank you. Given that M-C maintains an option for HTML editors like BlueGriffon and SeaMonkey, there shouldn't be a problem also maintaining an option for Thunderbird which has a wider user base than the other two mentioned.

Richard, what would be the best way to do a panel as described in comment #144? === Include the content the following images when sending this message? [ ] lover-at-beach.jpg [ ] compromising-image.png [OK] [Cancel] === Base it on a XUL <dialog>? Do we have something like this in the system? I saw some in the calendar. I can only find a lot of <prefwindow>s in the system. Where would be a good point to start?

Actually, I had a better look and found some small ones like mailnews/base/search/content/viewLog.xul or mailnews/compose/content/askSendFormat.xul

Never mind, I'm using askSendFormat.xul as a starting point.

In this patch: Dialogue that lists all files is presented to the user. Next up: When the user clicks "OK", set the moz-do-not-send attribute to "false" on the chosen images.

Comment on attachment 8798957 [details] Picture showing the file attach dialogue as proposed by Kent This dialog is directly before sending the message? What happens when I press "Cancel" or the close button? Will the message be sent without the images? If this is the case the "Cancel" button needs an other text or should be hidden and the "OK" button should have something like "Send Message" as text. For me the description text sounds wrong: "Include the content the following...". Should this be "Include *to* the content the following..." or "Include following images..."?

Comment on attachment 8798839 [details] [diff] [review] JK C-C part: WIP (v6). Review of attachment 8798839 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgCompose.cpp @@ +4254,5 @@ > +nsMsgCompose::AddMozDoNotSendFalse(nsString& aData) > +{ > + int32_t fPos; > + int32_t offset = -1; > + while ((fPos = aData.RFind(NS_LITERAL_STRING("file://"), offset)) != kNotFound) { Rolling out your own HTML parser, especially when fixing a security bug, is extremely dangerous. For example, if an attacker provides the following HTML and convinces people to paste it in their signature: <img src="file:///bad.png><span ">I win!</span> This code will turn it into: <img src="file:///bad.png><span " moz-do-not-send=false>I win!</span> and we lose. Note that I'm not bringing this up so that you can fix this one bug with this code and resubmit the patch. The reason why I'm bringing this up is to demonstrate that the basic approach is flawed. Do note that security researchers have looked at the patches that we land for security bugs for ages and depending on the nature of the patch, they may be able to decipher what the security bug was, and find more holes into the patch. Doing things such as parsing HTML yourself trying to add the moz-do-not-send attribute is just not going to work well, unless if you use a real HTML parser, serialize the source HTML into a DOM, perform mutations there and serialize back to an HTML string.

Sorry, mid-air collision, answer to Richard first. (In reply to Richard Marti (:Paenglab) from comment #152) If you apply the M-C patch and the C-C patch, you can try it out ;-) Maybe that would be best. Create yourself a page: <html><body> huhu<br> <img src="file://D:/Desktop/Bugzilla-down.png"><br> <img src="file://D:/Desktop/Dell%20Latitude%203150.png"><br> hihi </body></html> and copy its content from FF. The dialogue comes up immediately after you *pasted* or *dropped* HTML into/onto the composition that contains references for file:// based images. The images are always inserted into the composition, but if you don't select the ones you want and hit "OK", they will have no moz-do-not-send attribute set and thus won't be sent later since we default this to "true" now. If you hit cancel or close the dialogue, the images won't be sent either. (Mind you, the function behind the "OK" button is still missing.) Yes, sorry, the text is wrong, it was taken from Kent's suggestion in comment #134 Include the content of <the file name> when sending this message? and changed to Include the content of the following images when sending this message? but I messed up, "of" is missing. I don't find the message all that great, what is content of an image? We could say: Attach the following images when sending this message? but then people won't understand since they don't consider a (embedded) image in the HTML as an attachment (of course it is attached, but never shows as attachment). Maybe: Send the following images as part of the message? The window title also needs to change, currently "Attach Images to Message", perhaps: "Images referencing files detected". Any good ideas? Kent?

Comment on attachment 8796645 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR (v3). Review of attachment 8796645 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, this is a security bug, and I can't really sign off on this approach. The code changes are probably fine although I didn't spend a ton of time verifying that. I won't try to argue about, but I'm personally not comfortable signing off on this. Perhaps you can find another reviewer who would be open to r+ing this.

(In reply to :Ehsan Akhgari from comment #153) > For example, if an attacker provides the following HTML and convinces people > to paste it in their signature: > <img src="file:///bad.png><span ">I win!</span> Thanks for your interest, but I don't understand the comment. A signature is a file users store on disk, Thunderbird reads the file and adds the content to the compose window. I order that people don't have to change pre-existing signature files, we read the file and add the moz-do-not-send to it before placing the HTML into the compose windows. Where is the attack now? You want the attacker to convince the victim to modify their signature files? I know you're using TB, perhaps are you saying that the attacher is going convince the victim to change their HTML signature in the account settings? (I need to check that I add moz-do-not-send in this case.) I don't get the attack vector here. The security hole is that the attacker can get users to paste HTML referencing files, not only images, into their composition and the files will be sent. The security hole is not that the attacker can convince the victim to re-configure TB.

Comment on attachment 8796645 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR (v3). Christoph, can you please review this. You added https://hg.mozilla.org/mozilla-central/rev/65911fba8069#l1.13 so you might be the right person to review this. BTW, could you adjust the user in your patches: Christoph Kerschbaumer <mozilla@christophkerschbaumer.com> doesn't seem up-to-date.

Comment on attachment 8798957 [details] Picture showing the file attach dialogue as proposed by Kent (In reply to Jorg K (GMT+2) from comment #154) > The dialogue comes up immediately after you *pasted* or *dropped* HTML > into/onto the composition that contains references for file:// based images. Then the dialog is good. > Maybe: > Send the following images as part of the message? > > The window title also needs to change, currently "Attach Images to Message", > perhaps: "Images referencing files detected". > > Any good ideas? Kent? This looks better, but let's decide Kent.

Jörg, please don't steal other people's bugs without asking. Also, the only case the data:-uri approach is stalled is to worry about copying images from file:// based web sites. Still figuring that out, but even if we'd leave that broken it's way better than your approach. We should really just have landed that before summer and worried about the file:// based web sites issue later.

This is my current state of work. I will stop now. I've adjusted the text of the message and removed some unused strings. I checked that in my patch I have *not* covered the case of <img src="file://D:/Desktop/Bugzilla-down.png"> in a HTML signature. (In reply to Magnus Melin from comment #159) > Jörg, please don't steal other people's bugs without asking. Last you commented on the bug before yesterday was on the 10th of June (!!) I have repeatedly asked you to explain your solution (supposedly Plan A) (see for example comment #118) and I stated in comment #122 that HTML can get into the composition in a few ways 1) Image insertion via the image insertion dialogue. 2) Images from a signature. 3) HTML inserted via the HTML insertion or add-ons that insert general HTML (like HTML editors). Also add-ons that explicitly add images, for example "signature switch" and other "template" add-ons. 4) The -compose switch with message=file (or body=) where the message content is taken from a file (or passed on the command line). 5) Pasted/dropped content. of which you have only covered 1) and 2).

(In reply to Kent James (:rkent) from comment #133) > Although I have no fundamental objection to a plan that requires us to use > data uris instead of file uris in messages, I think that we need to give a > lot of thought to the migration process for that I think you're overestimating the impact it would/could have compared to the other approach. The key thing is that the send-by-default exist no more in either approaches (for good reason). Also, security is not typically rolled out behind a pref, you just roll it out.

Carrying forward Richard's ui-r+ I hope the new strings are better.

(In reply to Jorg K (GMT+2) from comment #160) > I have repeatedly asked you to explain your solution (supposedly Plan A) > (see for example comment #118) and I stated in comment #122 that HTML can > get into the composition in a few ways > 1) Image insertion via the image insertion dialogue. > 2) Images from a signature. > 3) HTML inserted via the HTML insertion or add-ons that insert general HTML > (like HTML editors). Also add-ons that explicitly add images, for example > "signature switch" and other "template" add-ons. > 4) The -compose switch with message=file (or body=) where the message > content is taken from a file (or passed on the command line). > 5) Pasted/dropped content. > of which you have only covered 1) and 2). #3 is not an issue as add-ons would obviously have to adapt (in your solution too). It's easy to cargo-cult the JavaScript code needed for this. #4 didn't exist until a few days ago, there's no back-compat to worry about here. We can easily say that in the case they need images in there they simply data: uri them. #5 Pasted content work just fine, with the one exception of images with file:// based web sites. Dropped content works from there too already AFAIR, because they are already using data uris! The A-solution is to finish off what I started. Then no user interaction is needed for something they don't understand anyway. If we can't figure that out, maybe I should steal your pastelistener for that corner case...

Comment on attachment 8796645 [details] [diff] [review] JK M-C part: Handling image insertion for APP_TYPE_MAIL_EDITOR (v3). Review of attachment 8796645 [details] [diff] [review]: ----------------------------------------------------------------- In case it wasn't clear, I'm opposed to this. There's simply no reason to do this approach given there's a good alternative.

(In reply to Magnus Melin from comment #163) > #5 Pasted content work just fine, with the one exception of images with > file:// based web sites. Dropped content works from there too already AFAIR, > because they are already using data uris! No, it doesn't see comment #139.

Dragging of fragments, how do you even do that? Dragging of images (which makes sense) works, like you wrote.

(In reply to Magnus Melin from comment #166) > Dragging of fragments, how do you even do that? See example in comment #154. You drag a little more than just an image. It's the same as paste as you can see from my patch.

Doesn't seem to do anything at all on linux at least.

(In reply to Magnus Melin from comment #168) Works fine in Windows: huhu + image + hihi gets dragged. (In reply to Magnus Melin from comment #159) > even if we'd leave that broken it's way better than your approach. Frankly, after all these months I haven't understood what is "good" or "better" about your approach of the data URIs. The arguments I've heard so far were: Comment #88: There's really not a solid solution around using data uris. You'll just play wack-a-mole, because is we use the editor as an editor it's a TRUSTED editor. We're not (going to), so we must play by the rules of the web, which means no file:// access. If you try to cheat you open an attack vector. E-Mail to TB Council of 27/09/2016 22:27 ... and it's the web thing to do anyway. To me, sending an e-mail is packing a bundle of data and shipping it out and it would be good to only ship what the user intended. Both solutions actually ship the same data, since data URIs are also changed to CIDs referencing an attachment. The idea is that the UI of the e-mail program should be easy to use and presenting *raw image data* is really not part of that. So I'm not convinced at all that your solution is better then mine, since the key point, not sending be default is the same, and of course, your idea. With that changed, it is irrelevant how the data is handled before shipping and IMHO data URIs make things worse rather than better.

(In reply to Magnus Melin from comment #168) > Doesn't seem to do anything at all on linux at least. On my Linux Mint 17.1 with Cinnamon, I can drag huhu+image+hihi to a TB 38.2 compose window with no problem.

(In reply to Jorg K (GMT+2) from comment #169) > E-Mail to TB Council of 27/09/2016 22:27 > ... and it's the web thing to do anyway. If you open a page <html><body> <div style="background-color:#eee;border:1px solid #000;padding:10px; height:200px;" contenteditable=""></div> </body></html> locally, that editor has access to local files. I don't understand why the TB compose window should behave any differently. So I'm asking for the felt 1000th time: Why are the data URIs right, good or even better that just showing the file-based image?

That page would have access to local files only as long as you're viewing it via file://. It works then because it's the same origin. Most of the web security model revolves around the same origin policy, and by using data URIs the data is accessible when needed so you don't have to break that fundamental security model by disabling the security checks. Naturally when you disable the security check you expose yourself to all kind of unexpected trouble, this bug being a prime example of that. Now, a web page editor doesn't do any kind of attaching of the data it references and the data is not sent anywhere, which is why a mail editor is so much different. Hope you got the answer you were looking for. Let me know if it's not clear.

I'm afraid it's not clear. This bug is about tricking users so that data that they didn't mean to send out is in fact sent out. My preference would be addressing this bug. The obvious solution is to not send anything by default but instead send only data that was explicitly requested: 1) Images inserted under user control. 2) Images from signatures inserted under program control (but also explicitly requested by the user since the configured their signatures). 3) Data inserted under user control using HTML or add-ons inserting HTML. For pasted content we will have the policy to "sanitise" pasted content, so that the "send this" directive gets stripped and nothing that is pasted is sent by default. We all agree that not sanitising as we currently do is wrong, and together with shipping by default, that's the attack vector we need to disable. These are the two cornerstones that both solutions have in common. I see no advantage in converting image data to data URIs for viewing purposes, since as we established, according to the "same origin policy" the FF browser and currently the TB compose window will show file-based images. And that's OK, as long as this data isn't shipped out. Or are you saying: I don't trust the Thunderbird software, somehow it will be possible that data is shipped out although it wasn't meant to be? (And that wouldn't happen when converted to data URIs first?) In your solution so far, paste simply won't work. The user doesn't see what they pasted (always referring to file-based images, of course). As far as I can see, we have no way to distinguish a voluntary paste from one that is part of an attack. So the best we can do is display a list of files and send those the user explicitly accepted. But there is no harm done in displaying all pasted content. Since you're planning to change the app type to "none", you can only view data URI images (and of course images with a source via http(s)). So if you were to give the user a view of the images they pasted, you'd have to convert them to data URIs as well, and they would then have the same problem of accidentally being shipped as if they were viewed from a file. You can also not fix the case where the pasted fragment is controlled by CSS with display:none and the user accepts the image when prompted. Repeating: How does converting images from local files to data URIs fix any issue? Which issue? Which fundamental security model am I breaking and which checks am I disabling when Firefox, assumed secure, currently shows me local images in a contenteditable?

(In reply to Magnus Melin from comment #172) > Which fundamental security model am I breaking and which checks am I > disabling when Firefox, assumed secure, currently shows me local images in a > contenteditable? OK, you're saying that the TB compose window doesn't have a file-based origin and therefore shouldn't access local files. The check we're disabling would be to show file-based images anyway. Got that. Please clarify further what advantages upholding the "same origin policy" in a program that serves as a local editor would bring other than avoiding "all kind of unexpected trouble".

(In reply to Jorg K (GMT+2) from comment #173) > I see no advantage in converting image data to data URIs for viewing > purposes, since as we established, according to the "same origin policy" the > FF browser and currently the TB compose window will show file-based images. Let's get things straight: Firefox shows it due to that same origin policy says ok, as long as it's a file:// based web site. Thunderbird compose displays it because it's set as app=EDITOR atm. > And that's OK, as long as this data isn't shipped out. Or are you saying: I > don't trust the Thunderbird software, somehow it will be possible that data > is shipped out although it wasn't meant to be? (And that wouldn't happen > when converted to data URIs first?) If all you need to do is somehow sneak in an attribute on the "image", that is clearly an attack vector. If you only use data: uris all the data is there already, and you couldn't have tricked the data to be there. The attack vector is gone. > Since you're planning to change the app type to "none", you can only view > data URI images (and of course images with a source via http(s)). So if you > were to give the user a view of the images they pasted, you'd have to > convert them to data URIs as well, and they would then have the same problem > of accidentally being shipped as if they were viewed from a file. No, because the data is already on the clipboard. The attacker can't put that there, while they can easily guess the name of a few files on my drive. It's not *me* who is converting to data uris (except for in the back-compat signature case). > Which fundamental security model am I breaking and which checks am I > disabling when Firefox, assumed secure, currently shows me local images in a > contenteditable? None, but that's why it also doesn't work once you put the same page on http:// - exactly because the same origin policy prevents that. It's not a very useful comparison though.

Mid air-collision, but I'll post this anyway since it goes with comment #174. And perhaps you can also give some references for "same origin policy" when applied to local files. I'm not a security expert, but I understand that this policy is there to avoid certain attacks where content is placed on different sites on the WWW. Maybe Christoph, who is still nominated as a reviewer of the patch that is "breaking the rules", can explain this.

(In reply to Magnus Melin from comment #175) > If all you need to do is somehow sneak in an attribute on the "image", that > is clearly an attack vector. Yes, all they need to do is "somehow" install some malware onto my machine, then take over the machine, install an add-on that goes around and sets moz-do-not-send=false on all the images in the composition. But hold on, they already have full access to the machine, so why do it the hard way and not simply ship out *all* data without guessing the file names first. Sure, this is an attack vector: - They guess the name of the file. - They get you to paste. - They somehow sneak in an attribute on the "image". *Very* unlikely. There are many a thousand times more likely attacks. > No, because the data is already on the clipboard. The attacker can't put > that there, while they can easily guess the name of a few files on my drive. > It's not *me* who is converting to data uris (except for in the back-compat > signature case). ... and for image insertion. I'd like to see your solution to the paste problem before I can comment further. As I said, if paste were to do anything useful, you've have to convert file-based images to data URIs.

(In reply to Jorg K (GMT+2) from comment #177) Sorry I forgot: > Sure, this is an attack vector: > - They guess the name of the file. > - They get you to paste. They persuade you not be be alerted by the message that is displayed: DO YOU WANT TO SEND THIS FILE??? > - They somehow sneak in an attribute on the "image". > *Very* unlikely. There are many a thousand times more likely attacks. Even more unlikely. What you're sacrificing is a semi-decent HTML editor just to get this one minuscule additional morsel of pseudo-security. There are a few much more important things to worry about in TB: Messages being sent to the wrong recipient due to faulty autocomplete and we have bugs where the wrong file is attached (fatal).

(In reply to Magnus Melin from comment #175) > If all you need to do is somehow sneak in an attribute on the "image", that > is clearly an attack vector. If you only use data: uris all the data is > there already, and you couldn't have tricked the data to be there. The > attack vector is gone. Is it? Please consider this: The attacker gets you to paste: <img src="file://C://Users/Magnus/Documents/private/important.doc"> (or equivalent for Linux). This *is* pasted into the composition (unless you do some processing on paste) and so far in your solution there isn't even a warning. Also, with app type "none" you don't even see a broken image. Now, in the same inexplicable way, you sneak your attribute onto the "image", and voilà, it is shipped out and no data URI was ever involved. What am I missing? As far as I remember your patch, you don't rip out sending when moz-do-not-sent=false altogether. Again, apart from destroying a semi-decent editor, data URIs ain't solving nothing yet.

Magnus, please answer: (In reply to Jorg K (GMT+2) from comment #173) > How does converting images from local files to data URIs fix any issue? > Which issue? The very unlikely attack vector of 'somehow sneak[ing] in an attribute' remains 100% in place. Upholding the "same origin policy" for a local editor seems purely academic, so therefore data URIs offer zero advantage over file:// access since they're only used for viewing in a voluntarily curtailed access model.

(In reply to Jorg K (GMT+2) from comment #177) > (In reply to Magnus Melin from comment #175) > > If all you need to do is somehow sneak in an attribute on the "image", that > > is clearly an attack vector. > Yes, all they need to do is "somehow" install some malware onto my machine, Of course not, once you can install malware it's game over. > Sure, this is an attack vector: > - They guess the name of the file. > - They get you to paste. > - They somehow sneak in an attribute on the "image". > *Very* unlikely. There are many a thousand times more likely attacks. Unlikely? What are you talking about?? That's the very scenario from this bug! It's very easy to guess the path for certain critical files on linux, e.g. your ssh keys are almost always in the same place. And paste is just one path to get content into the composer. You have drag'n'drop, forward (in various ways), replies, and probably a few more ways. All it takes is one little miss somewhere in those checks and you will have snuck in your phony image with attribute to include a file. (In reply to Jorg K (GMT+2) from comment #178) > What you're sacrificing is a semi-decent HTML editor just to get this one > minuscule additional morsel of pseudo-security. I don't see how this is really degrading anything, and what yo call pseudo-security is the very security cornerstone that the platform is built upon. (In reply to Jorg K (GMT+2) from comment #179) > The attacker gets you to paste: > <img src="file://C://Users/Magnus/Documents/private/important.doc"> (or > equivalent for Linux). > > This *is* pasted into the composition (unless you do some processing on > paste) and so far in your solution there isn't even a warning. Also, with > app type "none" you don't even see a broken image. Yes, but it's also not sending anything. All anybody can trick me to do is to send a broken link, and who cares about that. > Now, in the same inexplicable way, you sneak your attribute onto the > "image", and voilà, it is shipped out and no data URI was ever involved. No, because I made the attribute a no-op for file:// urls so there's no file attached. Only a broken link. > What am I missing? As far as I remember your patch, you don't rip out > sending when moz-do-not-sent=false altogether. Yes it's preserved so you can choose to include images from http inline if you like. But it's ONLY http. > Again, apart from destroying a semi-decent editor, data URIs ain't solving > nothing yet. It changes the editor surprisingly little, and like I've written it solves all the security issues that comes from the bad design of letting an attribute include a random file on send.

(In reply to Magnus Melin from comment #181) > > Sure, this is an attack vector: > > 1) They guess the name of the file. > > 2) They get you to paste. > > 3) They somehow sneak in an attribute on the "image". > > *Very* unlikely. There are many a thousand times more likely attacks. > Unlikely? What are you talking about?? That's the very scenario from this > bug! It's very easy to guess the path for certain critical files on linux, > e.g. your ssh keys are almost always in the same place. And paste is just > one path to get content into the composer. You have drag'n'drop, forward (in > various ways), replies, and probably a few more ways. All it takes is one > little miss somewhere in those checks and you will have snuck in your phony > image with attribute to include a file. The "very scenario" are parts 1) and 2) which meet faulty software. No argument that the software needs fixing. The very unlikely part is number 3) which could only be done via another software fault. So you don't have any faith in the software, that's why you want to do a "clean sweep" approach. BTW, you don't touch replies or forwards as they are already covered by code that you apparently don't trust in https://dxr.mozilla.org/comm-central/rev/75d461c3461613bb44fa1373240f1d4197895023/mailnews/compose/src/nsMsgCompose.cpp#468 > No, because I made the attribute a no-op for file:// urls so there's no file > attached. Only a broken link. I missed that in the patch then, it's been a while, let me see, four months ;-( Sorry, but you can't do that without withdrawing even more functionality: Do this: - Open a compose window - Insert > Link. Chose a file. Notice the "Attach the source of the link to the message" When you do this, a link to the file is inserted, and the file is attached. The recipient can click the link and the file opens. Are you going to rip this out or are you intending to change general files to data URIs too? > ... bad design of letting an attribute include a random file on send. That's in the eye of the beholder. If the attribute is correctly controlled, I see no problem. Software also controls where the message is sent to, and as mentioned, faulty software has led to messages being sent to the wrong recipient with one documented case of a $50.000 financial loss as a consequence. So do we therefore stop sending messages or remove the error-prone autocomplete? You need to see this problem here in proportion. But slowly your line of argument becomes clear. Main argument: bad design of letting an attribute include a random file on send - since if you get this wrong, an undesired file will be shipped. That's driven by distrust in the software in general. Fix: Make moz-do-not-send a no-op for file:// URLs. Consequence: Turn necessary file references into data URIs. May I ask why you want to stop being an EDITOR app then? Users could paste file based references as much as they like, with or without sanitising, since you'd never ship out any file-based stuff. I don't want to think this further, but just as a hint: Showing the paste/drop message I've implemented would give the user the chance to see what they pasted and you can then turn the selected content into data URIs. You still need to trust your software, since if you convert the wrong file, ... well, you know what happens. With making moz-do-not-send a no-op for file:// URLs, why do you need to sanitise and why do you need to revert the default at all? One thing just occurred to me: If you paste HTML with <img src="http://...> into your composition, the current default behaviour is to attach those images. Is this intended to change? I believe it would change in my patch, so I'd have to revisit that.