245399 - nsEscapeHTML* should escape single quote

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Bucksch (:BenB)]

See bug 243040 comment 11 ff.

May be security bug.
May break stuff. I'll attach a patch, but I won't garantee for anything and
don't want to be held liable for any possible fallout.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

Fix.

nsEscapeHTML* really shouldn't hardcode the max entity length in the buffer
allocation. I *almost* caused a (small) buffer overflow here!

Comment 2

[Akkana Peck]

In bug 243040 I asked about the difference between the various routines in
nsEscape, both vs. each other, and when it's appropriate to use nsEscapeHTML and
nsEscapeHTML2 instead of the entity converters that live in the intl libraries,
e.g. NS_ENTITYCONVERTER_CONTRACTID.  This isn't Ben's problem and shouldn't
block bug 243040, but it would be great if someone who understands the issue
could comment on it, or, ideally, add a comment in nsEscape.h explaining the use
of those two routines.

Is the difference that nsEscapeHTML* are meant for cases where we're creating
text that we're immediately intending to parse, but otherwise don't want entity
conversion?

Comment 3

[Ben Bucksch (:BenB)]

I intended to add a comment about nsEscapeHTML* in a later version of the patch.
I don't know what the diff between nsEscape() and NS_EscapeURL is, though.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 149956 [details] [diff] [review]
Fix, v1

-  PRUnichar *resultBuffer = (PRUnichar
*)nsMemory::Alloc(aSourceBufferLen*6*sizeof(PRUnichar) +
sizeof(PRUnichar('\0')));
+  // XXX don't hardcode max entity len
+  PRUnichar *resultBuffer = (PRUnichar
*)nsMemory::Alloc(aSourceBufferLen*7*sizeof(PRUnichar) +
sizeof(PRUnichar('\0')));

If you're going to add that comment here, add it in the other place too, or
just leave it out (my preference).

sr=jst

Comment 5

[Ben Bucksch (:BenB)]

Actually, there are not as many callers as I thought:
<http://lxr.mozilla.org/seamonkey/search?string=nsEscapeHTML>. Mainly bookmarks
and Mailnews in a few places each.

This actually looks bogus to me, because I'd guess that escaping would be needed
 in many places - either they places are not escaping properly or duplicate the
code or it's not needed as often as I think :).

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Fix, v2 (checked in)

This is the version I checked in. Identical to last version, with comment
changes requested.

Comment 7

[Ben Bucksch (:BenB)]

FIXED.

I'll add the comment/doc mentioned in comment2/3, if/when I have a chance later.

Comment 8

[Christian Schmidt]

Shouldn't "'" be escaped to "'" instead of "‘"?

Comment 9

[Ben Bucksch (:BenB)]

I don't know, but apos is not listed on
<http://www.w3.org/TR/REC-html40/sgml/entities.html>

Comment 10

[Christian Schmidt]

I guess "'" is only defined in XHTML.

For HTML, "'" can be used instead. This is what e.g. PHP's
htmlspecialchars() <http://www.php.net/manual/en/function.htmlspecialchars.php>
does.

Comment 11

[Stephen Walker]

This broke apostrophe's in bookmark names.

Comment 12

[Ben Bucksch (:BenB)]

Bug 245758.

I said I won't fix fallout. This bug is fixed, bookmarks need to be fixed.
Closing again.

Comment 13

[Ben Bucksch (:BenB)]

I grepped through the source for "unescape" and the only other relevant instance
I  found is in nsRelatedLinksHandler.cpp (I filed bug 245763 about it), which
seems to be a copy of the relevant bookmarks code. I'll attach a fix for both,
but we probably should add an nsUnescapeHTML2() to nsEscape.cpp?

Comment 14

[Peter Van der Beken [:peterv]]

Well, you slightly changed the contract of nsEscapeHTML, it used to just escape
using entities common to XML and HTML and you've added a HTML-only entity. lsquo
really is not the right entity for an apostrophe, it's apos in XML or #039 in
both XML and HTML. Is there any reason to not use #039?

Comment 15

[Ben Bucksch (:BenB)]

> you slightly changed the contract of nsEscapeHTML

No, only the impl, i.e. what it does, not what it's supposed to do.

> you've added a HTML-only entity

<http://www.w3.org/TR/REC-xml/#sec-predefined-ent> shows that &#039; is probably
the right replacement. REOPENing.

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Fix, v3 - use #39 instead of lsquo

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 150180 [details] [diff] [review]
Fix, v3 - use #39 instead of lsquo

r+sr=jst

Comment 18

[Ben Bucksch (:BenB)]

Fix v3 checked in. Marking FIXED again.