nsEscapeHTML* should escape single quote

RESOLVED FIXED

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

14 years ago
Group: security
Status: NEW → ASSIGNED
Summary: nsEscape should escape single quote → nsEscapeHTML2 should escape single quote
(Assignee)

Comment 1

14 years ago
Created attachment 149956 [details] [diff] [review]
Fix, v1

Fix.

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

Updated

14 years ago
Attachment #149956 - Flags: superreview?(jst)
Attachment #149956 - Flags: review?(darin)

Comment 2

14 years ago
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?
(Assignee)

Comment 3

14 years ago
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 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
Attachment #149956 - Flags: superreview?(jst) → superreview+

Updated

14 years ago
Attachment #149956 - Flags: review?(darin) → review+
(Assignee)

Comment 5

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

Comment 6

14 years ago
Created attachment 150101 [details] [diff] [review]
Fix, v2 (checked in)

This is the version I checked in. Identical to last version, with comment
changes requested.
Attachment #149956 - Attachment is obsolete: true
(Assignee)

Comment 7

14 years ago
FIXED.

I'll add the comment/doc mentioned in comment2/3, if/when I have a chance later.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Summary: nsEscapeHTML2 should escape single quote → nsEscapeHTML* should escape single quote

Comment 8

14 years ago
Shouldn't "'" be escaped to "&apos;" instead of "&lsquo;"?
(Assignee)

Comment 9

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

Comment 10

14 years ago
I guess "&apos;" is only defined in XHTML.

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

Comment 11

14 years ago
This broke apostrophe's in bookmark names.
Blocks: 245758
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

14 years ago
Bug 245758.

I said I won't fix fallout. This bug is fixed, bookmarks need to be fixed.
Closing again.
No longer blocks: 245758
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

14 years ago
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?
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?
(Assignee)

Comment 15

14 years ago
> 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

14 years ago
Created attachment 150180 [details] [diff] [review]
Fix, v3 - use #39 instead of lsquo
(Assignee)

Updated

14 years ago
Attachment #150180 - Flags: superreview?(jst)
Attachment #150180 - Flags: review?(darin)
Comment on attachment 150180 [details] [diff] [review]
Fix, v3 - use #39 instead of lsquo

r+sr=jst
Attachment #150180 - Flags: superreview?(jst)
Attachment #150180 - Flags: superreview+
Attachment #150180 - Flags: review?(darin)
Attachment #150180 - Flags: review+
(Assignee)

Comment 18

14 years ago
Fix v3 checked in. Marking FIXED again.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.