90728 - mailto: link treats body= as HTML

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[scottputterman]

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

Comment 3

[Jean-Francois Ducarroz]

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

Comment 4

[scottputterman]

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

Comment 5

[Jesse Ruderman]

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.

Comment 6

[Jean-Francois Ducarroz]

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

Comment 7

[John Keiser (jkeiser)]

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

Comment 8

[Ben Bucksch (:BenB)]

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 <[e-mail]>", though.

BTW: Do we allow the From field to be sat? I don't think we should.

Comment 9

[John Keiser (jkeiser)]

That was bug 61893.

Comment 10

[John Keiser (jkeiser)]

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).

Comment 11

[Jean-Francois Ducarroz]

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...

Comment 12

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

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 13

[varada]

Comment on attachment 93226 [details] [diff] [review]
Proposed fix, v1

r=varada;
looks good to me.

Comment 14

[Jean-Francois Ducarroz]

Attachment: proposed fix, v2

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

Comment 15

[Jean-Francois Ducarroz]

Comment on attachment 94234 [details] [diff] [review]
proposed fix, v2

carrying over r=varada

Comment 16

[(not reading, please use [e-mail] instead)]

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.

Comment 17

[Jean-Francois Ducarroz]

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...

Comment 18

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v3

Fix memory leak with allowTags

Comment 19

[Jean-Francois Ducarroz]

Comment on attachment 96049 [details] [diff] [review]
Proposed fix, v3

carry over R=varada

Comment 20

[(not reading, please use [e-mail] instead)]

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

Comment 21

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v4

I have replaced nsIPref by nsIPrefService and nsIPrefBranch

Comment 22

[Jean-Francois Ducarroz]

Comment on attachment 96191 [details] [diff] [review]
Proposed fix, v4

R=varada, SR=sspitzer

Comment 23

[Jean-Francois Ducarroz]

Fixed in the trunk

Comment 24

[Jesse Ruderman]

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.

Comment 25

[Matteo Settenvini]

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.

Comment 26

[Jesse Ruderman]

[e-mail]: 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.