integer overflow in nsEscape

RESOLVED FIXED

Status

()

P1
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dveditz, Assigned: dveditz)

Tracking

({fixed1.8.1.18, fixed1.9.0.4, fixed1.9.1})

unspecified
fixed1.8.1.18, fixed1.9.0.4, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.4 +
blocking1.8.1.18 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
+++ This bug was initially created as a clone of Bug #445117 +++

>  /* XXX Hardcoded max entity len. The +1 is for the trailing null. */
>  char *rv = (char *) nsMemory::Alloc(strlen(string) * 6 + 1);
>
> |* 6| overflows at about 683M on 32 bit platform.

The issue Georgi noted in bug 445117 also occurs in xpcom/io/nsEscape.cpp -- the same pattern happens in nsEscapeHTML() and nsEscapeHTML2()

I'm not sure if you can practically stuff 683M into the things that use nsEscapeHTML, maybe if the transport is gzipped.
http://mxr.mozilla.org/mozilla/search?string=nsEscapeHTML
Flags: wanted1.8.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
Flags: blocking1.8.1.18+
(Assignee)

Updated

11 years ago
Whiteboard: [sg:moderate]
dveditz you mean something different from Bug 367736 ?
aaa, i see:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsEscape.cpp#292

292   PRUnichar *resultBuffer = (PRUnichar *)nsMemory::Alloc(aSourceBufferLen *
293                             6 * sizeof(PRUnichar) + sizeof(PRUnichar('\0')));
>I'm not sure if you can practically stuff 683M

i suppose receiving it is possible.

more of problems for exploiting this probably are:

1. 683 MB on the wire consumes much more VM so OOM may be hit
2. one must avoid crash. though if one does overlapping copy (the returned ptr is in low address) it may not crash.
in theory 683MB need not be on the wire - they may be javascript generated probably
(Assignee)

Comment 5

11 years ago
Yeah, a document title in places, say, or something fed through the feed sanitizer.
(Assignee)

Updated

11 years ago
Flags: blocking1.9.0.4? → blocking1.9.0.4+
(Assignee)

Updated

11 years ago
Assignee: nobody → dveditz
(Assignee)

Comment 6

11 years ago
This code got copied to nsPlacesImportExportService.cpp, apparently because the use of string apis in the nsEscape copy caused problems?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/places/src/nsPlacesImportExportService.cpp&rev=1.65#239

Dietrich: what's the story there? Is the situation different now so this code can call the standard escape routine? Simple enough to fix in both places, but if it's no longer necessary getting rid of duplicate code would be nice.
(Assignee)

Comment 7

11 years ago
Posted patch prevent nsEscapeHTML overflow (obsolete) — Splinter Review
In addition to the patch I cleaned up a bunch of tabs. Will attach a -w patch next to make review easier.
Attachment #344247 - Flags: superreview?(benjamin)
Attachment #344247 - Flags: review?(benjamin)
(Assignee)

Comment 8

11 years ago
Posted patch cvs diff -w version of above (obsolete) — Splinter Review
(Assignee)

Comment 9

11 years ago
Posted patch same for places (obsolete) — Splinter Review
Attachment #344249 - Flags: review?(dietrich)

Comment 10

11 years ago
Comment on attachment 344247 [details] [diff] [review]
prevent nsEscapeHTML overflow

>Index: xpcom/io/nsEscape.cpp

>+    char *rv = nsnull;
>+    /* XXX Hardcoded max entity len. The +1 is for the trailing null. */
>+    PRUint32 len = PL_strlen(string);
>+    if (len < (PR_UINT32_MAX / 6)) {
>+        rv = (char *)nsMemory::Alloc( (len * 6) + 1 );
>+    }
>+    char *ptr = rv;
> 
>-	return(rv);
>+    if(rv)

This would be a lot clearer if you took a shortcut path:

  if (len >= PR_UINT32_MAX / 6)
    return nsnull;

  char *rv = (char*) NS_Alloc((6 * len) + 1);
  if (!rv)
    return nsnull;

  ... conversion code

This reduces the overall indent and makes the control flow more apparent.

Also please use NS_Alloc instead of nsMemory::Alloc in all new code.

r=me with those nits addressed
Attachment #344247 - Flags: superreview?(benjamin)
Attachment #344247 - Flags: superreview+
Attachment #344247 - Flags: review?(benjamin)
Attachment #344247 - Flags: review+
(Assignee)

Comment 11

11 years ago
Attachment #344247 - Attachment is obsolete: true
Attachment #344248 - Attachment is obsolete: true
Attachment #344249 - Attachment is obsolete: true
Attachment #344249 - Flags: review?(dietrich)
(Assignee)

Updated

11 years ago
Attachment #344700 - Flags: approval1.9.0.4?
Attachment #344700 - Flags: approval1.8.1.18?
(Assignee)

Comment 13

11 years ago
checked in: http://hg.mozilla.org/mozilla-central/rev/c6d4904201b5
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 344700 [details] [diff] [review]
nits fixed, what was checked in

Approved for 1.8.1.18 and 1.9.0.4. a=ss
Attachment #344700 - Flags: approval1.9.0.4?
Attachment #344700 - Flags: approval1.9.0.4+
Attachment #344700 - Flags: approval1.8.1.18?
Attachment #344700 - Flags: approval1.8.1.18+
(Assignee)

Comment 15

11 years ago
Checked into the 1.8 and 1.9.0 branches
Keywords: fixed1.8.1.18, fixed1.9.0.4
Any word on a mechanism to exercise this code as we discussed the other day, Dan?

Updated

11 years ago
Flags: blocking1.8.0.15+

Comment 17

11 years ago
Comment on attachment 344700 [details] [diff] [review]
nits fixed, what was checked in

a=asac for 1.8.0 branch
Attachment #344700 - Flags: approval1.8.0.15+
(Assignee)

Updated

11 years ago
Group: core-security
(Assignee)

Updated

11 years ago
Depends on: 464998

Comment 18

10 years ago
This landed before 1.9.1 branched
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.