If a mailto: link includes a body= parameter, the contents of that parameter are treated as HTML rather than plain text. This bug makes it easier to exploit the "wiretap" security hole (bug 66938) from the web. I don't think it would be a great loss for the internet if message templates on the web couldn't include HTML code.

Now that it looks like 66938 has been fixed, is this still a problem?

yes, we are still intepreting the body as HTML. However, I don't know if this is still a security hole!

Mitch or Jesse, is this still an issue? Minusing because it doesn't seem to be.

This isn't a major problem. Here's what it lets an attacker do: 1. An attacker could up a mailto: link containing wiretap code. If the attacker can convince the user to send a message to the address in the link, it's likely that the attacker will end up seeing a copy of the message. 2. An attacker could hide part or all of the body of the default message from the sender. Thus, the sender might think he's composing starting with a blank message, when in fact there is additional text the recipient will see. The text could be hidden by enclosing it in <script>document.write()</script> (invisible in composer, visible to HTML+JS recipients), or by enclosing it in <span style="display:none"> (invisible except to non-HTML recipients). Note that RFC 2368 (mailto: URLs) says in its Security Considerations that "a mail client should never send a message based on a mailto URL without first showing the user the full message that will be sent". Thus this is a standards- compliance issue in addition to a minor security issue.

nominating nsbeta again for reconsideration as it seems we have a security issue here...

Hmmmm. It seems I need to fix this in order to fix bug 61983. So I will fix it as part of that. yaaaay!

IIRC, we use mailto: with HTML as body internally, e.g. for Send Link... Watch out for that, if you fix this bug. I am not sure that this should be fixed as formulated. Sure, I see the problems mentioned, and they should be prevented (current and forthcoming ones). However, forbidding *all* HTML in mailto: seems like suggesting to use lynx, because JS has security problems. How about that: you run the HTML in the body through the HTML Sanitizer implemented in bug 108153. Maybe you could even do this for *all* text programmatically inserted into the composer, e.g. also for quotes. All other fields are not allowed to contain HTML. Note that the address fierld can have the form "Real Name <user@example.com>", though. BTW: Do we allow the From field to be sat? I don't think we should.

That was bug 61893.

Not fixed with bug 61893, BTW. I found another way to do it--I sent the force-plain-text parameter in the mailto link (force-plain-text=Y).

Let's make mailto url uses plain text by default (it's currently html) and in case we use html, let's run the body through the html sanitizer...

mailto url are now always interpreted as plain text unless the parameter "html-body" is specified. In that case, the message body is sanitized to avoid any potential security problem. The old parameter "force-plain-text" is now obsolete.

Comment on attachment 93226 [details] [diff] [review] Proposed fix, v1 r=varada; looks good to me.

same patch, just fix spelling error: aHTMLBoby --> aHTMLBody

Comment on attachment 94234 [details] [diff] [review] proposed fix, v2 carrying over r=varada

In your patch, we alwasy copy the rawBody, even if it isn't HTML. + + nsString rawBody = NS_ConvertUTF8toUCS2(aBodyPart); + nsString sanitizedBody; + + MSG_ComposeFormat format = nsIMsgCompFormat::PlainText; + if (aHTMLBody) I think you could assign to raw body, and do the copy, inside the if (aHTMLBody) block. But you'll have to fix the logic of this code: + pMsgCompFields->SetBody(format == nsIMsgCompFormat::HTML ? sanitizedBody.get() : rawBody.get()); maybe: if (!aHTMLBody) pMsgCompFields->SetBody(NS_ConvertUTF8toUCS2(aBodyPart).get()); else pMsgCompFields->SetBody(format == nsIMsgCompFormat::HTML ? sanitizedBody.get() : rawBody.get()); That should avoid the body copy in the plain text case, right? 2) + char* allowedTags = 0; + nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID); + if (prefs) + prefs->CopyCharPref("mailnews.display.html_sanitizer.allowed_tags", &allowedTags); You're leaking allowedTags. Use an nsXPIDLCString.

1) The rawBody is used for plain text as well. In both case, we need to convert the utf8 body received to Unicode. Then when we set the message body in the compose fields, it get converted back using the compose field charset (could be UTF8 or something else). The original code was doing that already. Therefore, I cannot avoid the copy/convertion in plain text or in html mode 2) I'll fix that...

Fix memory leak with allowTags

Comment on attachment 96049 [details] [diff] [review] Proposed fix, v3 carry over R=varada

Comment on attachment 96049 [details] [diff] [review] Proposed fix, v3 JF tells me that this code only gets executed for mailto urls. So, making the code more optimized (avoiding a copy) but making the code more complicated doesn't make a lot of sense. but don't use nsIPref, it is deprecated. see http://lxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPref.idl#48 once you fix that, sr=sspitzer

I have replaced nsIPref by nsIPrefService and nsIPrefBranch

Comment on attachment 96191 [details] [diff] [review] Proposed fix, v4 R=varada, SR=sspitzer

Fixed in the trunk

I think this fix caused a regression: bug 167815, clicking a mailto: link always gives a plain text message compose window. The text given in body= should be interpreted as plain text, but the user should be able to type HTML into the message compose window that appears.

This has caused bug #190120, and a clear regression. For anybody who has something to add to this bug, please consider that this one's resolution has caused a lot of trouble... so don't think to reopen it. The fix also introduced other problems, as stated in bug 190120. C'ya.

msettenvini@tin.it: fixing bug 190120 will probably not regress the security hole in this bug. If it does, I will file a new bug instead of reopening this one.