mailto: link treats body= as HTML

RESOLVED FIXED in mozilla1.2alpha

Status

MailNews Core
Composition
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: Jean-Francois Ducarroz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.2alpha
x86
Windows NT
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: nsbeta, have fix. security, URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 42230 [details]
testcase
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7

Updated

17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.9

Comment 2

17 years ago
Now that it looks like 66938 has been fixed, is this still a problem?
Keywords: nsbeta1
Target Milestone: mozilla0.9.9 → ---
(Assignee)

Comment 3

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

Comment 4

17 years ago
Mitch or Jesse, is this still an issue?  Minusing because it doesn't seem to be.
Keywords: nsbeta1 → nsbeta1-

Updated

17 years ago
QA Contact: sheelar → esther
(Reporter)

Comment 5

17 years ago
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.
(Assignee)

Comment 6

17 years ago
nominating nsbeta again for reconsideration as it seems we have a security issue
here...
Whiteboard: nsbeta

Comment 7

17 years ago
Hmmmm.  It seems I need to fix this in order to fix bug 61983.  So I will fix it
as part of that.  yaaaay!
Depends on: 61983

Comment 8

17 years ago
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.

Comment 9

17 years ago
That was bug 61893.
Depends on: 61893
No longer depends on: 61983
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).
(Assignee)

Comment 11

16 years ago
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...
(Assignee)

Comment 12

16 years ago
Created attachment 93226 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Whiteboard: nsbeta → nsbeta, have fix

Comment 13

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

r=varada;
looks good to me.
Attachment #93226 - Flags: review+
(Assignee)

Comment 14

16 years ago
Created attachment 94234 [details] [diff] [review]
proposed fix, v2

same patch, just fix spelling error: aHTMLBoby --> aHTMLBody
Attachment #93226 - Attachment is obsolete: true
(Assignee)

Comment 15

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

carrying over r=varada
Attachment #94234 - Flags: review+
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.
(Assignee)

Comment 17

16 years ago
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...
(Assignee)

Comment 18

16 years ago
Created attachment 96049 [details] [diff] [review]
Proposed fix, v3

Fix memory leak with allowTags
Attachment #94234 - Attachment is obsolete: true
(Assignee)

Comment 19

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

carry over R=varada
Attachment #96049 - Flags: review+
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
(Assignee)

Comment 21

16 years ago
Created attachment 96191 [details] [diff] [review]
Proposed fix, v4

I have replaced nsIPref by nsIPrefService and nsIPrefBranch
Attachment #96049 - Attachment is obsolete: true
(Assignee)

Comment 22

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

R=varada, SR=sspitzer
Attachment #96191 - Flags: superreview+
Attachment #96191 - Flags: review+
(Assignee)

Comment 23

16 years ago
Fixed in the trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.2alpha
(Reporter)

Comment 24

16 years ago
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

15 years ago
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.
(Reporter)

Comment 26

15 years ago
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.
(Reporter)

Updated

15 years ago
Whiteboard: nsbeta, have fix → nsbeta, have fix. security
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.