JS "lambda" replace exposes malloc heap space after end of JS string

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({fixed-aviary1.0.3, fixed1.7.7, js1.5})

Trunk
mozilla1.8beta2
fixed-aviary1.0.3, fixed1.7.7, js1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.7 +
blocking-aviary1.0.3 +
blocking-aviary1.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

13 years ago
Quick and dirty patch next.  There's a better patch that should be done, but the
patch I'll attach next fixes this, AFAICT.

/be
(Assignee)

Comment 1

13 years ago
Created attachment 179311 [details] [diff] [review]
quick and dirty fix

Really, all of cx->regExpStatics should be stacked and unstacked around the
lambda invocations.

/be
Attachment #179311 - Flags: review?(shaver)
(Assignee)

Updated

13 years ago
Severity: normal → critical
Status: NEW → ASSIGNED
Flags: blocking1.7.7?
Flags: blocking-aviary1.0.3?
Priority: -- → P1
Summary: "lambda" replace exposes malloc heap space after end of JS string → JS "lambda" replace exposes malloc heap space after end of JS string
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Comment 2

13 years ago
Created attachment 179313 [details] [diff] [review]
better fix

Lacking block scope and dynamically scoped variables, JS has never tried to
emulate Perl's dynamic scope for $1, $&, etc. with the pre-ECMA extensions
analogous to those vars (RegExp.$1, RegExp.lastMatch, etc).  Here's a case
where it must.

/be
Attachment #179313 - Flags: review?(shaver)
(Assignee)

Updated

13 years ago
Group: security
(Assignee)

Comment 3

13 years ago
*** Bug 288608 has been marked as a duplicate of this bug. ***
*** Bug 288685 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1+
Whiteboard: [sg:fix]
Created attachment 179332 [details]
testcase

preserving testcase for the future in case the cubic.xfo.org.ru blog ever goes
away.
(Assignee)

Updated

13 years ago
Attachment #179313 - Flags: superreview?(dbaron)
(Assignee)

Comment 6

13 years ago
BTW, this bug is like 8+ years old.  Roger Lawrence fixed half of it in 2000:

r=norris,waldemar
Fixes for bugs#23607, 23608, 23610, 23612, 23613. Also, first cut at URI
encode & decode routines.

Unfortunately, AFAICT none of the bugs he cites had anything to do with the two
hunks of that revision:

@@ -1061,16 +1080,22 @@ find_replen(JSContext *cx, ReplaceData *
@@ -1138,16 +1163,17 @@ find_replen(JSContext *cx, ReplaceData *

that half-fixed the original 1997-era bug.

/be
(Assignee)

Updated

13 years ago
Attachment #179311 - Flags: review?(shaver)
(Assignee)

Comment 7

13 years ago
Comment on attachment 179313 [details] [diff] [review]
better fix

dbaron shoots, he scores: cx->regExpStatics.moreParens points to malloc'ed
space that could be realloc'ed by the inner match.

/be
Attachment #179313 - Attachment is obsolete: true
Attachment #179313 - Flags: superreview?(dbaron)
Attachment #179313 - Flags: superreview-
Attachment #179313 - Flags: review?(shaver)
(Assignee)

Comment 8

13 years ago
Created attachment 179344 [details] [diff] [review]
fix, v3
Attachment #179311 - Attachment is obsolete: true
Attachment #179344 - Flags: superreview?(dbaron)
Attachment #179344 - Flags: review?(shaver)
(Assignee)

Comment 9

13 years ago
Comment on attachment 179344 [details] [diff] [review]
fix, v3

I am a crispy critter right now.  This leaks the inner moreParens, if
allocated.  dbaron is two for two.

/be
Attachment #179344 - Attachment is obsolete: true
Attachment #179344 - Flags: superreview?(dbaron)
Attachment #179344 - Flags: review?(shaver)
(Assignee)

Comment 10

13 years ago
Created attachment 179347 [details] [diff] [review]
fix, v4
Attachment #179347 - Flags: superreview?(dbaron)
Attachment #179347 - Flags: review?(shaver)
Attachment #179347 - Flags: superreview?(dbaron) → superreview+
Attachment #179347 - Flags: review?(shaver) → review+
(Assignee)

Comment 11

13 years ago
Fixed on trunk, AVIARY_1_0_1_20050124_BRANCH, and MOZILLA_1_7_BRANCH.

Thanks for the report, I hope that's the last bug from 1997 left ;-).

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.7.7?
Flags: blocking1.7.7+
Flags: blocking-aviary1.0.3?
Flags: blocking-aviary1.0.3+
Resolution: --- → FIXED
Keywords: fixed-aviary1.0.3, fixed1.7.7

Comment 12

13 years ago
js/tests/js1_5/Regress/regress-288688.js checked in.

Updated

13 years ago
Depends on: 288818

Comment 13

13 years ago
This is assigned to SA14821 (including Suite 1.7.6)
http://secunia.com/advisories/14821/

and SA14820 (Firefox 1.0.1 and 1.0.2)
http://secunia.com/advisories/14820/

Link to "Mozilla Products Arbitrary Memory Exposure Test" page:
http://secunia.com/mozilla_products_arbitrary_memory_exposure_test/


Comment 14

13 years ago
*** Bug 289134 has been marked as a duplicate of this bug. ***
Blocks: 256195
*** Bug 289194 has been marked as a duplicate of this bug. ***

Comment 16

13 years ago
*** Bug 290161 has been marked as a duplicate of this bug. ***
*** Bug 290310 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Flags: testcase+

Comment 18

12 years ago
This bug is not entirely solved!!

If you go to the secunia site
http://secunia.com/mozilla_products_arbitrary_memory_exposure_test

and do the test with 1.0.2, it shows some contents of the memory.

Now with 1.0.3 (linux) it only shows some X's, but the entire system crashes
after doing this test a few times!!

Dont know if this is another problem, but here it is, since it's triggered by
the same thing..

Comment 19

12 years ago
This bug was fixed after Firefox 1.0.2. I just checked secunia with Firefox
1.0.4 and it does not show the 'XXX'. Please upgrade to 1.0.4 as it contains
significant security updates and retest.
(In reply to comment #19)
> 1.0.4 and it does not show the 'XXX'. 

Heh! It SHOULD show "XXX..." because it's correct behaviour of the script.

Comment 21

12 years ago
(In reply to comment #20)

> Heh! It SHOULD show "XXX..." because it's correct behaviour of the script.

You are right of course, doh! but it looks like a variant of bug 293940 prevents
the string from displaying. You can check that the mem string is 'XXX...X' by
setting a breakpoint at line 247 in the page where 
document.getElementById('result').value = mem;

Blocks: 256197
No longer blocks: 256195

Comment 22

11 years ago
verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.