integer overflow in nsEscape, still

RESOLVED FIXED in mozilla1.9.1

Status

()

--
critical
RESOLVED FIXED
10 years ago
10 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)

(Assignee)

Description

10 years ago
+++ 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+
(Assignee)

Updated

10 years ago
Flags: blocking1.9.1?
Whiteboard: [sg:moderate]
(Assignee)

Comment 1

10 years ago
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?
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.  :-/
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.
+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]

Updated

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

Comment 10

10 years ago
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: 10 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 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

10 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

10 years ago
Flags: blocking1.8.0.next+
(Assignee)

Updated

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