Incorrect line number in e4x tokens leads to buffer overreads and possible crashes

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: gkw, Assigned: Waldo)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+, status2.0 unaffected, blocking1.9.2 needed, status1.9.2 .18-fixed, blocking1.9.1 needed, status1.9.1 wanted)

Details

(Whiteboard: [ccbr][has patch][sg:moderate], fixed-in-tracemonkey)

Attachments

(3 attachments)

Posted file testcase
Attached testcase crashes both js debug and opt shell on TM changeset 0d4b01278890 without -m nor -j.

Nominating blocking2.0? and setting s-s because I have no idea what this is capable of. It seems to access a weird memory location at 0x1006627d6 in 64-bit.

This also reproduces in 32-bit.
Posted file stacks
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   62719:2d44fc834071
user:        Nicholas Nethercote
date:        Thu Feb 17 19:02:48 2011 -0800
summary:     Bug 634444 - Errors in long lines cause memory spikes when a console is in use.  r=brendan, a=blocking.
Blocks: 634444
blocking2.0: ? → final+
Whiteboard: [ccbr] → [ccbr][hardblocker]
Investigating.
Assignee: general → jwalden+bmo
I don't crash on this.  On the other hand, dvander gets an error running it in valgrind.  Will investigate.
Could be related to bug 635144, which indicates that there are some latent problems with parsing of XML tokens that bug 634444 seems to have exposed.

I won't be able to take a look until Monday morning Melbourne time (2pm Sunday, MV time).  Backing out the bug 634444 patch is an option;  without the javascript.options.strict flag set the chance of having many error messages on very long lines while a console is attached is not that high.
Thinking some more:  let's just back out bug 634444.  This crash plus the strangeness in 635144 are worrying, and I don't think it's worth the effort and risk right now.  The memory spikes we get are at least well understood, they do go back down (assuming your machine hasn't ground to a halt) and this can be fixed in a 4.0.1 release.

Waldo, can you back out the two patches in bug 634444 today?  If not, I'll do it on Monday morning Melbourne time.
Bug 629858 also looks related to bug 634444.  I suspect there may be some latent problems with the lineno/index calculations in the parser, though I may have broken something myself.  Let's back bug 634444 out!
(In reply to comment #6)
> Thinking some more:  let's just back out bug 634444.  This crash plus the
> strangeness in 635144 are worrying, and I don't think it's worth the effort and
> risk right now.  The memory spikes we get are at least well understood, they do
> go back down (assuming your machine hasn't ground to a halt) and this can be
> fixed in a 4.0.1 release.

Besides bug 634444 and bug 635144, a fragile testcase in bug 629858 may be related. (A small patch in the latter seems to touch lines that the patch in bug 634444 does.)

Just to summarize the plethora of bugs:

Without the patch in bug 634444: implications vary from increasing memory usage in bug 634444, and / or unreliable crashes in bug 629858.

With the patch: problems in bug 635144 and this bug 635235.
Posted patch e4x_TCO++Splinter Review
I debugged this before seeing the latest comments, with which I concur completely.  I'll back out that change later tonight, an easy tree-watch since I have a m-c push to make.
Attachment #513667 - Flags: review?(jorendorff)
Thanks, Waldo.  I wonder if your patch will fix bug 635144.
Whiteboard: [ccbr][hardblocker] → [ccbr][hardblocker][has patch]
Comment on attachment 513667 [details] [diff] [review]
e4x_TCO++

Stealing the review.  I get two Valgrind warnings without the patch, and they disappear with the patch.  Thanks, Waldo!
Attachment #513667 - Flags: review?(jorendorff) → review+
(In reply to comment #8)
> 
> Without the patch in bug 634444: implications vary from increasing memory usage
> in bug 634444, and / or unreliable crashes in bug 629858.

Bug 629858 was present since at least 1.9.1, but the patch in bug 634444 exposed it.

> With the patch: problems in bug 635144 and this bug 635235.

Bug 635144 is likely a dup of this bug, I'll confirm that later.
Summary: Crash [@ __memcpy] or [@ js::TokenStream::reportCompileErrorNumberVA] → Incorrect line number in e4x tokens leads to buffer overruns and possible crashes
Turns out this isn't a regression.  I've confirmed that this code:

  eval("try{}catch(e){} <x> </\nx>");

leads to buffer overruns (reported by Valgrind) in both 1.9.1 and 1.9.2.  So I've requested that this block 1.9.1 and 1.9.2.

I tracked this bug back, it exists in revision 1 in the hg repository, which means it's at least 4 years old.  I didn't track it back further than that, the thought was too depressing because (a) I'd have to deal with CVS, and (b) I'm guessing I'd learn that it's been present since the initial e4x work was done in late 2004.

Two thumbs up to Gary and Waldo for finding and fixing it!
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Keywords: regression
Whiteboard: [ccbr][hardblocker][has patch] → [ccbr][hardblocker][has patch][sg:critical]
Whiteboard: [ccbr][hardblocker][has patch][sg:critical] → [ccbr][hardblocker][has patch][sg:critical], fixed-in-tracemonkey
Yeah, this is an old bug. We have older. In 2004, we didn't have fuzz-testing like we do now, or valgrind in its current state of the art, or more than 11 employees at Mozilla Foundation, or more than the early and uncertain hope of browser competition restarting due to Firefox growing fast since earlier that year, or a functioning Ecma TC39 TG1 (E4X got that body back together, but Microsoft's rep was manager of the C# compiler team, having fun doing paper designs and taking trips -- the IE6 team had been disbanded and only a skeleton crew remained), or a great many other things we now enjoy.

So, cheer up, and no retcons please.

Thanks to Gary and Waldo. Keep going, there are other such bugs hiding from the old days.

/be
OS: Mac OS X → Windows 7
Talking to njn, reviewing the details, it looks like this bug is not an sg:critical full remove euid exploit. It's a read-chars-past-end-of-userbuf bug. Privacy loss, not control flow integrity loss or memory overwrite.

/be
"full remote", of course.

/be
(In reply to comment #15)
> 
> So, cheer up, and no retcons please.

I thought I knew what that word meant -- "rewriting history", more or less.  Maybe I've got it's meaning wrong, though, because all I did was point out that this is an old bug.
(In reply to comment #13)
> (a) I'd have to deal with CVS

You could try the mozilla-cvs in git repo, it has bisection too :)

http://muizelaar.blogspot.com/2011/02/updated-mozilla-cvs-history-git-repo.html
This might only be an overread.  In the code at issue here I *think* that's all it is.  (Even this soon after it's getting a little hazy in my mind.)  But I would not be surprised if other locations rely on this information being exactly correct (looks like the position comparison operators do, and they're hard to grep for unfortunately).  So I'd still counsel paranoia even if the symptom here looks "not entirely horrible".
Cut the FUD. TokenPos comparison operators are used for def-use fixups in transplanted comprehension expressions. Meanwhile, we have bug bounties going out to real sg:critical reporters. That coin should not be debased by exaggeration here.

/be
Sorry.  I'd just thought it was standard practice to err on the side of caution when the extent of a problem was not easily grasped, as I do not (believe I) grasp the extent of the problem here.  It's the same approach I've taken lots of other times when asked to assess the security severity of a bug, and that I thought others have taken as well.
We'd like this bug on branches but due to comment 16 we won't block s security release on it. Please ask for approval on the patch if it is the same for branches.
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
lowering to sg:moderate for potentially revealing memory contents, although could be sg:high if you can read arbitrary amounts of memory and get reliable access to it from the script. Not blocking a branch release but we'd love to take this patch when it's ready and baked on trunk.
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
Summary: Incorrect line number in e4x tokens leads to buffer overruns and possible crashes → Incorrect line number in e4x tokens leads to buffer overreads and possible crashes
Whiteboard: [ccbr][hardblocker][has patch][sg:critical], fixed-in-tracemonkey → [ccbr][hardblocker][has patch][sg:moderate], fixed-in-tracemonkey
Attachment #513667 - Flags: approval1.9.2.14?
Attachment #513667 - Flags: approval1.9.1.17?
It's baked on trunk for a few days now.  It's a one line patch and very low risk, IMO.
(In reply to comment #25)
> It's baked on trunk for a few days now.  It's a one line patch and very low
> risk, IMO.

You mean baked on tracemonkey?
(In reply to comment #26)
> 
> You mean baked on tracemonkey?

Oh, yes, sorry for any confusion.
Shouldn't it bake on trunk first? Also it's too late for requesting approval for 1.9.1.17 and 1.9.2.14. Those builds are already in beta stage.
Attachment #513667 - Flags: approval1.9.2.15?
Attachment #513667 - Flags: approval1.9.2.14?
Attachment #513667 - Flags: approval1.9.1.18?
Attachment #513667 - Flags: approval1.9.1.17?
Per conversation with drivers, only sg:moderate so we don't think it needs to block. Please renominate if you disagree.
blocking2.0: final+ → .x+
Whiteboard: [ccbr][hardblocker][has patch][sg:moderate], fixed-in-tracemonkey → [ccbr][has patch][sg:moderate], fixed-in-tracemonkey
Comment on attachment 513667 [details] [diff] [review]
e4x_TCO++

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Attachment #513667 - Flags: approval1.9.2.15?
Attachment #513667 - Flags: approval1.9.2.15+
Attachment #513667 - Flags: approval1.9.1.18?
Attachment #513667 - Flags: approval1.9.1.18+
This bug's patch is in tm, so it is gonna ship in Firefox 4 AFAIK, and it's as safe a fix as you could want. Just sayin'.

/be
This already landed:

http://hg.mozilla.org/mozilla-central/rev/335dbb9502e0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 513667 [details] [diff] [review]
e4x_TCO++

moving approvals to the next release.
Attachment #513667 - Flags: approval1.9.2.18+
Attachment #513667 - Flags: approval1.9.2.17+
Attachment #513667 - Flags: approval1.9.1.20+
Attachment #513667 - Flags: approval1.9.1.19+
Group: core-security
You need to log in before you can comment on or make changes to this bug.