This is the SeaMonkey version of bug 1151366 "File disclosure via covertly imposed attachments in HTML emails " Longs story short, the compose window sets the app type to APP_TYPE_EDITOR https://hg.mozilla.org/comm-central/rev/14bd3b70a7fda2bfb80b0905adb05ceef296e2d4#l7.14 - this let's the compose window reference file://, mailbox:// and imap:// resources that will get attached at send time. The attempt was earlier to try and tag resources with moz-do-not-send so that say things in a quoted reply doesn't get included. As far as we know that part mostly works (though we found some edge cases it wouldn’t: like Edit as New). It is however easy to trick someone to copy-paste attack html into the compose window, with hidden "images" that can steal files from your computer or a lot of your imap Inbox. http:// intranet resources are also at danger. Copy-pastes were supposed to be sanitized, but as it turns out APP_TYPE_EDITOR don't sanitize pastes... and even if sanitized, the (earlier) default to include wouldn't have helped. Instead of playing the wack-a-mole game, Thunderbird moved away from being an APP_TYPE_EDITOR. Embedded images now use data: URLs. I tried to not break SeaMonkey, but I think this ifdef should have included mailbox and imap: https://hg.mozilla.org/comm-central/rev/14bd3b70a7fda2bfb80b0905adb05ceef296e2d4#l8.225 So what needs to be ported/checked/changed is at least: - bug 1315480 - bug 1316570 - bug 1151366 - https://hg.mozilla.org/comm-central/rev/14bd3b70a7fda2bfb80b0905adb05ceef296e2d4

I can't access bug 1151366. Can someone CC me? Thanks

Because of our broken tests I will need to do some more testing before asking for review. Would be glad if someone could also compile and check this out. I basically took the mail function unmodified and might have missed something Magnus and Jorg. Big thanks. I own you a beer or two. Could have never have done it in just a few hours otherwise. I basically ported all 3 patches. I needed to add the notification box to the SeaMonkey compose window for unblocking only. I didn't style it yet but it works in classic. Our copyImage in utlityOverlay was changed in Bug 752505 "Copy Image broken on Nightly" and so I should think it is ok for all cases and I didn't take this part from Bug 1315480. If it works we need to figure out how to get the additional strings into 2.49.1

SeaMonkey 2.49 ESR52 version. Takes the strings for the blocking notification out of Thunderbirds composeMsgs.properties. Local build with compare-locales and repack for de worked so it will probably only burn when doing the actual relese build on the builder. Hi ewong :) Quick test was ok but need to check if any syntax changes not supported in 52 crept in.

(In reply to Frank-Rainer Grahl (:frg) from comment #3) > Created attachment 8881278 [details] [diff] [review] > 1322172-dataurl.patch > > Because of our broken tests I will need to do some more testing before > asking for review. Would be glad if someone could also compile and check > this out. I basically took the mail function unmodified and might have > missed something I did some testing, concentrating on the things you asked me to test and everything worked as expected. I ended up setting up my Gmail to work with SeaMonkey so will be using this for my normal email access for a couple of days. I will let you know if I run into any issues.

Comment on attachment 8881278 [details] [diff] [review] 1322172-dataurl.patch Since Bill has done some successful testing already, I've only looked visually at the code and tried to reconcile those with the patches that have landed for TB in the other bugs already. From what I can tell, looks good in general. >+++ b/suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties >+ >+blockedContentPrefLabel=Options >+blockedContentPrefAccesskey=O >+ >+blockedContentPrefLabelUnix=Preferences >+blockedContentPrefAccesskeyUnix=P >+ You don't need to distinguish between "Options" and "Preferences" here. For some reason, Firefox and Thunderbird call it "Options" on Windows and moved it from the Edit into the Tools menu. Not so SeaMonkey, it's "Preferences" on all platforms, thus you only need +blockedContentPrefLabelUnix=Preferences and +blockedContentPrefAccesskeyUnix=P (and maybe drop the trailing Unix, though you'll still need it for the comm-esr52 version of this patch). >+++ b/suite/mailnews/compose/EdColorPropsOverlay.xul >+ File.createFromNsIFile(nsFile).then(file => { >+ reader.readAsDataURL(file); >+ }); This is different from bug 1316570 attachment 8810227 [details] [diff] [review], or did I miss another patch ported from mail/? >\ No newline at end of file should there be? >+++ b/suite/mailnews/compose/messengercompose.xul >+ <key keycode="VK_ESCAPE" oncommand="handleEsc();"/> Where did the additional ESC-key handling come from? I don't see it in either of the ported TB bugs... >+<menupopup id="blockedContentOptions" value="" >+ onpopupshowing="onBlockedContentOptionsShowing(event);"> >+</menupopup> Oh, so that's for dismissing the blocked-content bar?

Comment on attachment 8881469 [details] [diff] [review] 1322172-dataurl-esr52.patch >+++ b/suite/locales/jar.mn >+#if AB_CD == en_US >+ locale/@AB_CD@/messenger/messengercompose/composeMsgsTB.properties (/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties) >+#else >+ locale/@AB_CD@/messenger/messengercompose/composeMsgsTB.properties (%../mail/chrome/messenger/messengercompose/composeMsgs.properties) >+#endif Looks evil, but as long as it does what we want it to... ;-) >+++ b/suite/mailnews/compose/MsgComposeCommands.js >@@ -3215,8 +3430,206 @@ function getPref(aPrefName, aIsComplex) >+ setBlockedContent: function(aBlockedURI) { >+ let brandName = this.brandBundle.getString("brandShortName"); >+ let buttonLabel = this.stringBundle.getString((AppConstants.platform == "win") ? >+ "blockedContentPrefLabel" : "blockedContentPrefLabelUnix"); >+ let buttonAccesskey = this.stringBundle.getString((AppConstants.platform == "win") ? >+ "blockedContentPrefAccesskey" : "blockedContentPrefAccesskeyUnix"); can be simplified to >+ setBlockedContent: function(aBlockedURI) { >+ let brandName = this.brandBundle.getString("brandShortName"); >+ let buttonLabel = "blockedContentPrefLabelUnix"; >+ let buttonAccesskey = "blockedContentPrefAccesskeyUnix"; or use the properties directly where invoked.

Comment on attachment 8881278 [details] [diff] [review] 1322172-dataurl.patch Review of attachment 8881278 [details] [diff] [review]: ----------------------------------------------------------------- I realize the fb wasn't directed to me but thought I'd drop a few items. ::: suite/mailnews/compose/EdColorPropsOverlay.xul @@ +13,5 @@ > + function onAccept() { > + // If it's a file, convert to a data URL. > + if (gBackgroundImage && /^file:/i.test(gBackgroundImage)) { > + let nsFile = Services.io.newURI(gBackgroundImage, null, null) > + .QueryInterface(Components.interfaces.nsIFileURL).file; alignment nits ::: suite/mailnews/compose/MsgComposeCommands.js @@ +1049,5 @@ > + let nsFile; > + try { > + nsFile = Services.io.getProtocolHandler("file") > + .QueryInterface(Components.interfaces.nsIFileProtocolHandler) > + .getFileFromURLSpec(img.src); alignment nit. @@ +1090,5 @@ > + // unwanted things in the content. > + let ParserUtils = Components.classes["@mozilla.org/parserutils;1"] > + .getService(Components.interfaces.nsIParserUtils); > + let html2 = ParserUtils.sanitize(doc.documentElement.innerHTML, > + ParserUtils.SanitizerAllowStyle); alignment nits for both ParserUtils line and html2 line. @@ +1157,5 @@ > if (!params) { > // This code will go away soon as now arguments are passed to the window using a object of type nsMsgComposeParams instead of a string > > params = Components.classes["@mozilla.org/messengercompose/composeparams;1"].createInstance(Components.interfaces.nsIMsgComposeParams); > params.composeFields = Components.classes["@mozilla.org/messengercompose/composefields;1"].createInstance(Components.interfaces.nsIMsgCompFields); I know this isn't in the code that was changed, but I think these two lines are way too long. @@ +3308,5 @@ > + > + if (gMsgCompose.originalMsgURI) { > + let msgSvc = Components.classes["@mozilla.org/messenger;1"] > + .createInstance(Components.interfaces.nsIMessenger) > + .messageServiceFromURI(gMsgCompose.originalMsgURI); I believe these two lines should be alined to the above . after Components. @@ +3341,5 @@ > + if (background && gMsgCompose.originalMsgURI) { > + // Check that background has the same URL as the message itself. > + let msgSvc = Components.classes["@mozilla.org/messenger;1"] > + .createInstance(Components.interfaces.nsIMessenger) > + .messageServiceFromURI(gMsgCompose.originalMsgURI); ditto here. @@ +3445,5 @@ > + let brandName = this.brandBundle.getString("brandShortName"); > + let buttonLabel = this.stringBundle.getString((AppConstants.platform == "win") ? > + "blockedContentPrefLabel" : "blockedContentPrefLabelUnix"); > + let buttonAccesskey = this.stringBundle.getString((AppConstants.platform == "win") ? > + "blockedContentPrefAccesskey" : "blockedContentPrefAccesskeyUnix"); unsolicited feedback: I'm not comfortable with blockedContentPrefLabelUnix and blockedContentPrefAccesskeyUnix. I would suggest the platform named first. i.e. unixBlockedContentPrefLabel and unixBlockedContentPrefAccessKey That said, assuming the original string is ok with IanN et.al, nit. I would believe it should be blockedContentPrefAccessKey. let buttonLabelStr = "blockedContentPrefLabel"; let buttonAccessKeyStr = "blockedContentPrefAccessKey"; if (AppConstants.platform != "win") { buttonLabelStr += "Unix" buttonAccessKeyStr += "Unix" } let buttonLabel = this.stringBundle.getString(buttonLabelStr); let buttonAccessKey = this.stringBundle.getString(buttonAccessKeyStr); @@ +3473,5 @@ > + null, this.notificationBar.PRIORITY_WARNING_MEDIUM, buttons); > + } > + else { > + this.notificationBar.getNotificationWithValue("blockedContent") > + .setAttribute("label", msg); alignment nit. @@ +3504,5 @@ > + // ... and in with the new. > + for (let url of urls) { > + let menuitem = document.createElement("menuitem"); > + menuitem.setAttribute("label", > + composeBundle.getFormattedString("blockedAllowResource", [url])); parameter alignment nit.. though aligning "composeBundle..." might throw it past the 80 char mark. So maybe substitute composeBundle... with a variable. @@ +3566,5 @@ > + if (filename) { > + try { > + contentType = Components.classes["@mozilla.org/mime;1"] > + .getService(Components.interfaces.nsIMIMEService) > + .getTypeFromURI(uri); alignment nit. @@ +3586,5 @@ > + null, > + Services.scriptSecurityManager.getSystemPrincipal(), > + null, > + Components.interfaces.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, > + Components.interfaces.nsIContentPolicy.TYPE_OTHER); having dealt solely with python over the past year or more, this part of the code scares me. Unfortunately, this isn't python; but javascript, so the only thing I can think of here is to align the parameters to the first one and substitute the last two parameters with something less lengthy since tagging on those with the alignment will go past the 80 char mark. @@ +3589,5 @@ > + Components.interfaces.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, > + Components.interfaces.nsIContentPolicy.TYPE_OTHER); > + let inputStream = channel.open(); > + let stream = Components.classes["@mozilla.org/binaryinputstream;1"] > + .createInstance(Components.interfaces.nsIBinaryInputStream); ditto with the alignment. ::: suite/mailnews/compose/messengercompose.xul @@ +147,5 @@ > <key keycode="VK_F6" oncommand="SwitchElementFocus(event);" modifiers="control,shift"/> > <key keycode="VK_F6" oncommand="SwitchElementFocus(event);" modifiers="shift"/> > <key keycode="VK_F6" oncommand="SwitchElementFocus(event);"/> > + > + <key keycode="VK_ESCAPE" oncommand="handleEsc();"/> I don't believe we need this extra line in 150.

(In reply to Edmund Wong (:ewong) from comment #8) > I'm not comfortable with blockedContentPrefLabelUnix > and blockedContentPrefAccesskeyUnix. > > I would suggest the platform named first. > > i.e. unixBlockedContentPrefLabel > and unixBlockedContentPrefAccessKey That's what was copied from the Thunderbird implementation, thus at least would be needed for 2.49.x. > if (AppConstants.platform != "win") { > buttonLabelStr += "Unix" > buttonAccessKeyStr += "Unix" > } As said, this isn't needed for SeaMonkey - we say "Preferences" independent of the platform.

Thanks for the feedback. So far didn't find an error with the implementation itself and will see that I get it in reviewable state over the weekend.

> with the implementation itself Or better with the patch itself. The implemenatation was ok from the start or it wouldn't have worked in TB :) Additonal feedback always welcome.

(In reply to rsx11m from comment #6) > >+++ b/suite/mailnews/compose/EdColorPropsOverlay.xul > >+ File.createFromNsIFile(nsFile).then(file => { > >+ reader.readAsDataURL(file); > >+ }); > > This is different from bug 1316570 attachment 8810227 [details] [diff] [review] > [review], or did I miss another patch ported from mail/? Yes, bug 1303518. Blame is your friend: https://hg.mozilla.org/comm-central/rev/7b1875cb7a3a#l1.12

I took the code from current comm-central not from the patches to make sure I pick the latest and greatest. That is why I also need to do additional screening for the ESR52 tree.

Right. You need to be careful, since some of those file operations are now async when they were sync in ESR 52. It bit me twice :-( https://treeherder.mozilla.org/#/jobs?repo=comm-esr52&revision=b8034a160796746409a3a2a002243f3531e21198 https://treeherder.mozilla.org/#/jobs?repo=comm-esr52&revision=2f2471564906b5ce66efc7730fe625c3a82fe61f These file operations changed twice. First the |new File()| constructor was removed, and then the thing was made async. ESR 52 needs the former, but not the latter, so backporting the C-C state to ESR 52 is wrong.

I think this version is reviewable. Mostly formatting changes compared to V1. I think I took care of all the NITs and removed the pref variables with Unix in the name. Alignment in messengercompose.xul was really bad and I cleaned it up. Let me know if you prefer a separate patch. As ewong assumed I was unable to align some variables as it should be so I either split the line completely at the assignments or started at column + 2 for assignments at followup lines. Maybe we should start declaring and using Cc. Ci. and Cu. like in mozilla code which would shorten a lot of lines quite good and ease porting

Interdiff V1 to V2 just for reference.

Looking at the interdiff found 2 other alignnemnt and order NITs which I took care of: <commandset id="msgComposeCommandUpdate" and <statusbar id="status-bar"

rsx11m fooled me. Here it needs to be Options only not Preferences like in the remote content blocking :)

Updated patch for esr52. Tested. As Jorg pointed out file access changed. Some other changes too but neglectable. The if for l10n repack needs to stay in. I see no way it can be unified for en-US and other languages in "suite/locales/jar.mn".

(In reply to Frank-Rainer Grahl (:frg) from comment #18) > rsx11m fooled me. Here it needs to be Options only not Preferences like in > the remote content blocking :) ??? - I don't get it, what did I miss? I was comparing that string to the usual notification bars like the mixed/insecure content warnings... But then, you are right if this refers to the "To protect your privacy..." bar, here indeed the button is labelled "Options" (but mentions "Permissions" then in the context menu; thus, one "Option" is to get to the "Preferences" rather than to directly get there as from the SSL bar - slightly confusing).

> ??? - I don't get it, what did I miss? Nothing much. Don't worry. > But then, you are right if this refers to the "To protect your privacy..." Yes. Bingo: > remoteContentPrefLabel=Options > remoteContentPrefAccesskey=O Here we have only unlock for the blockedContentPrefLabel menu. "Preferences" as string looked really awkward. I think "Options" is ok for both.

Comment on attachment 8882827 [details] [diff] [review] 1322172-dataurl-V2.patch >diff --git a/suite/mailnews/compose/EdColorPropsOverlay.xul b/suite/mailnews/compose/EdColorPropsOverlay.xul Potentially could have shared the code with TB, but outside scope of this bug... >+++ b/suite/mailnews/compose/MsgComposeCommands.js >+ // If findbar is visible and the focus is in the message body, >+ // hide it. (Focus on the findbar is handled by findbar itself). >+ let findbar = document.getElementById('FindToolbar'); Nit: mostly " rather than ' in this file. >+ if (!findbar.hidden && activeElement.id == "content-frame") { Most code seems to check findbar exists too. >+var gComposeNotificationBar = { >+ get stringBundle() { >+ delete this.stringBundle; >+ return this.stringBundle = document.getElementById("bundle_composeMsgs"); >+ }, sComposeMsgsBundle already exists, can that not be used? >+ >+ get brandBundle() { >+ delete this.brandBundle; >+ return this.brandBundle = document.getElementById("brandBundle"); >+ }, sBrandBundle already exists, can that not be used? >+function onBlockedContentOptionsShowing(aEvent) { >+ let urls = aEvent.target.value ? aEvent.target.value.split(" ") : []; >+ >+ let composeBundle = document.getElementById("bundle_composeMsgs"); sComposeMsgsBundle already exists, can that not be used? f+ for the moment, I want to see response / next patch.

Comment on attachment 8882827 [details] [diff] [review] 1322172-dataurl-V2.patch Review of attachment 8882827 [details] [diff] [review]: ----------------------------------------------------------------- ::: suite/mailnews/compose/MsgComposeCommands.js @@ +3312,5 @@ > + // We're already loading this, or tried so unsuccesfully. > + return; > + } > + > + if (gMsgCompose.originalMsgURI) { Actually, it turned out that this code is wrong. See bug 1379912. It turns out that a mailnews change I made in bug 1322103 was wrong and I need to revert it. So gMsgCompose.originalMsgURI cannot be used, since for drafts it got overwritten by the original URI of the message being replied to if the draft is a reply. So do the right thing and integrate those couple of lines from bug 1379912 straight away, or this won't work when 1379912 lands.

Thanks Jorg. Patch rebased for bit rot and suite changes for bug 1379912 (not yet checked in). Not setting review. Need to address IanNs comments first.

Patch without formatting changes.

Formatting changes

Maybe you want to r+ bug 1379912 for me since I have no idea when Magnus will show up.

I am done for today and can't promise tomorrow. Looks straight forward just from porting the patch. If it isn't reviewed Tuesday I will see that I manage then.

Comment on attachment 8886881 [details] [diff] [review] 1322172-dataurl-V2a.patch Review of attachment 8886881 [details] [diff] [review]: ----------------------------------------------------------------- Grrr, I've just noticed more. ::: suite/mailnews/compose/MsgComposeCommands.js @@ +1254,5 @@ > var editorElement = GetCurrentEditorElement(); > + > + // Remember the original message URI. When editing a draft it gets > + // overwritten by the source message URI which was replied to or forwarded > + // so we can set the disposition flags. I've changed that comment, you might want to follow. @@ +3360,5 @@ > + }, true); > + > + // Convert mailnews URL back to data: URL. > + let background = editor.document.body.background; > + if (background && gMsgCompose.originalMsgURI) { Damn, this need to change to gOriginalMsgURI as well. @@ +3364,5 @@ > + if (background && gMsgCompose.originalMsgURI) { > + // Check that background has the same URL as the message itself. > + let msgSvc = Components.classes["@mozilla.org/messenger;1"] > + .createInstance(Components.interfaces.nsIMessenger) > + .messageServiceFromURI(gMsgCompose.originalMsgURI); and here. @@ +3366,5 @@ > + let msgSvc = Components.classes["@mozilla.org/messenger;1"] > + .createInstance(Components.interfaces.nsIMessenger) > + .messageServiceFromURI(gMsgCompose.originalMsgURI); > + let originalMsgNeckoURI = {}; > + msgSvc.GetUrlForUri(gMsgCompose.originalMsgURI, originalMsgNeckoURI, null); and here.

Jorg, I updated the patch with yours and IanNs comments. IanN asked about the stringbundles and I am not sure what would be best; sComposeMsgsBundle already exists, can that not be used? Could you tell me why you put a getter for this in the notification box code for mail? mail does not have sBrandBundle but has a getComposeBundle function. FRG

You're referring to the clumsiness in: Line 98: _gComposeBundle = document.getElementById("bundle_composeMsgs"); Line 5565: return this.stringBundle = document.getElementById("bundle_composeMsgs"); Line 5630: let composeBundle = document.getElementById("bundle_composeMsgs"); I didn't write any of this, seems to be a case of "hier weiß die rechte nicht, was die linke tut" ;-(

> You're referring to the clumsiness in: Don't let Richard know what you just said :) That was bug 670639. SeaMonkey never ported it. Personally I like the getters in the notification bar more but using the vars in SeaMonkeys MsgComposeCommands.js would be a little less costly.

Hopefully everything but the stringbundles addressed. I would like to keep them to also keep the file in sync with TB but if you think not I will use the existing vars. Just let me know. Now fully ports bug 1379912 for SM too.

Comment on attachment 8886882 [details] [diff] [review] 1322172-dataurl-part2.patch Just whitespace and formatting changes.

(In reply to Frank-Rainer Grahl (:frg) from comment #32) > Don't let Richard know what you just said :) That was bug 670639. SeaMonkey > never ported it. Well, the getComposeBundle() function is OK, not using it, is not.

See bug 1383569.

Removed the string getters. Final till something else changes again :)

As IanN mentioned quotes and bracket formating was inconsistent and all over the place in MsgComposeCommands.js. question 1 do you want to change it how it is in this patch. I can do the one in mail too. question 2 if yes in a separate bug for both? question 3 if yes for esr52 too to ease backports?

Comment on attachment 8889416 [details] [diff] [review] 1322172-dataurl-part3.patch (In reply to Frank-Rainer Grahl (:frg) from comment #38) > As IanN mentioned quotes and bracket formating was inconsistent and all over > the place in MsgComposeCommands.js. > question 1 do you want to change it how it is in this patch. I can do the > one in mail too. Quotes are inconsistent everywhere, for new strings we use double-quotes. I wouldn't touch it since it will cause too many merge problems.

Minor nitfix. Missed the use of another stringbundle copy.

ESR52 2.49 version unbitrotted and updated.

Comment on attachment 8886882 [details] [diff] [review] 1322172-dataurl-part2.patch r=me for formatting work

Comment on attachment 8889416 [details] [diff] [review] 1322172-dataurl-part3.patch I'd only change it where we touching surrounding code.

Comment on attachment 8891746 [details] [diff] [review] 1322172-dataurl-V3b.patch >+++ b/suite/mailnews/compose/MsgComposeCommands.js >+ let msg = sComposeMsgsBundle.getFormattedString("blockedContentMessage", >+ [brandName, brandName]); Nit: Line up [ with " >+ let fString = sComposeMsgsBundle.getFormattedString("blockedAllowResource", >+ [url]); Nit: Line up [ with " r=me with those fixed.

https://hg.mozilla.org/comm-central/rev/538e19f624671e70877fb3d2dece742398f94329 https://hg.mozilla.org/comm-central/rev/6a2729aebec1a43258690cb8aa199b6a92e2da4a

Comment on attachment 8886882 [details] [diff] [review] 1322172-dataurl-part2.patch [Approval Request Comment] Regression caused by (bug #): -- User impact if declined: just a little harder to backport fixes. Testing completed (on m-c, etc.): c-c c-esr52 Risk to taking this patch (and alternatives if risky): none String changes made by this patch: none

Comment on attachment 8891747 [details] [diff] [review] 1322172-dataurl-esr52-V3.patch r+ for comm-central patch carried forward. Slighly different necause of file and security api changes. Alternate patch for comm-esr52 taking the strings from Thunderbird.

Decided for 2.52 / 55 wontfix. I think string changes are already done because of the August 2 merge date and I don't want to patch it up with the TB strings and comm-central changes.

Comment on attachment 8886882 [details] [diff] [review] 1322172-dataurl-part2.patch a=me

Comment on attachment 8891747 [details] [diff] [review] 1322172-dataurl-esr52-V3.patch a=me

https://hg.mozilla.org/releases/comm-esr52/rev/d99859fe9970070552a2e3e5346e8c17213e5797 https://hg.mozilla.org/releases/comm-esr52/rev/64b45c54e83a3c0784ab413feb9b814b456c38cc