integer overflow in nsEscape, still

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dveditz, Assigned: dveditz)

Tracking

({fixed1.9.1, verified1.8.1.19, verified1.9.0.5})

unspecified
mozilla1.9.1
fixed1.9.1, verified1.8.1.19, verified1.9.0.5
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.5 +
wanted1.9.0.x +
blocking1.8.1.19 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x ?
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 attachment)

918 bytes, patch
bsmedberg
: review+
bsmedberg
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.8.1.19+
Samuel Sidler (old account; do not CC)
: approval1.9.0.5+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #455987 +++

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

bug 455987 tried to fix this, but I added when I should have subtracted the size of the terminator in nsEscapeHTML2.
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19+
Flags: blocking1.9.1?
Whiteboard: [sg:moderate]
Created attachment 348268 [details] [diff] [review]
Get the sign right
Attachment #348268 - Flags: superreview?(benjamin)
Attachment #348268 - Flags: review?(benjamin)
Attachment #348268 - Flags: approval1.9.1b2?
Attachment #348268 - Flags: approval1.9.0.5?
Attachment #348268 - Flags: approval1.8.1.19?
Attachment #348268 - Flags: approval1.8.0.15?

Comment 2

9 years ago
The patch that wrote up looks right (based on the simple tests cases we ran it through), but it feels like a lot of work to ensure something bad doesn't happen a developer does something bad with the APIs.  

I think we should just redefine these APIs to fail of you pass more than x-Mb of data or some other huge but possibly reasonable number.  If you pass 4gb of data to nsEscapeHTML2, you are asking for trouble.  :-/

Comment 3

9 years ago
s/that/that Dan
Seems like it would be good to have xpcom/tests/TestEscape.cpp to test for these problems, no?  Then you know what's actually been tried and what hasn't been tried in the tests, and perhaps this error might have been caught.

Comment 5

9 years ago
+1 great idea.
> If you pass 4gb of data to nsEscapeHTML2, you are asking for trouble.  :-/

this is correct though known.
check:
Bug 367721 limiting virtual memory (mainly 64bit)
Bug 445295 misuse of int32 and int64 in API on 64 bit platforms
offtopic:

does someone know how the ibm bug |ptr[negativeidx] = stuff;| was discovered - manual human inspection, static analyzer or else?

if it was static analyzer (which they may wish to promote for cash reasons) they may have more like this and probably *hydra checks for [negativeidx] may be considered.
Whiteboard: [sg:moderate] → [sg:moderate][needs r/sr bsmedberg]
Attachment #348268 - Flags: superreview?(benjamin)
Attachment #348268 - Flags: superreview+
Attachment #348268 - Flags: review?(benjamin)
Attachment #348268 - Flags: review+
Whiteboard: [sg:moderate][needs r/sr bsmedberg] → [sg:moderate]
Comment on attachment 348268 [details] [diff] [review]
Get the sign right

Approved for 1.9.0.5 and 1.8.1.19. a=ss for release-drivers
Attachment #348268 - Flags: approval1.9.0.5?
Attachment #348268 - Flags: approval1.9.0.5+
Attachment #348268 - Flags: approval1.8.1.19?
Attachment #348268 - Flags: approval1.8.1.19+
Attachment #348268 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 348268 [details] [diff] [review]
Get the sign right

a191b2=beltzner
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Fix checked into 1.8 and 1.9.0 branches
Keywords: fixed1.8.1.19, fixed1.9.0.5
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/79948e2ddec5
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
no tests?  qq
Flags: in-testsuite?
>no tests?  qq

there is a testcase in Bug 445117 for basically the same, though it is a perl script generating folder. testcases for this bug will use significant amount of VM.
Should I trust your ability to change the sign, Dan? :-)
You can also check Bonsai.

1.9.0: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-11-18+12%3A45&maxdate=2008-11-18+12%3A45&cvsroot=%2Fcvsroot

1.8.1: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-11-18+12%3A45&maxdate=2008-11-18+12%3A45&cvsroot=%2Fcvsroot
You take the fun out of teasing Mr. Veditz but, yes, it is clearly fixed.
Keywords: fixed1.8.1.19, fixed1.9.0.5 → verified1.8.1.19, verified1.9.0.5
Keywords: fixed1.9.1

Comment 17

9 years ago
Comment on attachment 348268 [details] [diff] [review]
Get the sign right

a=asac for 1.8.0
Attachment #348268 - Flags: approval1.8.0.next? → approval1.8.0.next+

Updated

9 years ago
Flags: blocking1.8.0.next+
Group: core-security
Hey Al, can you verify this bug across trunk and 1.9.1? I don't have access to bug 445117 due to security clearances :(
You need to log in before you can comment on or make changes to this bug.