The default bug view has changed. See this FAQ.

"Assertion failure: vchars && *vchars" with uneval and setter function whose __proto__ has been changed

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {assertion, testcase, verified1.8.1.5})

Trunk
assertion, testcase, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] checked in already? need feedback=crowder)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
js> f = (function(){}); y = ({p getter: f}); f.__proto__ = []; uneval(y);
Assertion failure: vlength > n, at jsobj.c:963
(Reporter)

Updated

10 years ago
No longer blocks: 349611
(Reporter)

Updated

10 years ago
Blocks: 349611
(Reporter)

Comment 1

10 years ago
I think the fix for bug 367629 changed the assertion failure:

js> f = (function(){}); y = ({p getter: f}); f.__proto__ = []; uneval(y);
Assertion failure: vchars && *vchars, at jsobj.c:966

Comment 2

10 years ago
Yeah, I expected it to, I reworked the assertion in that area, but the patch doesn't resolve this problem.  I added mrbkap to see if he had any ideas about how to resolve this easily.
(Reporter)

Updated

10 years ago
Summary: "Assertion failure: vlength > n" with uneval and setter function whose __proto__ has been changed → "Assertion failure: vchars && *vchars" with uneval and setter function whose __proto__ has been changed
(Assignee)

Comment 3

10 years ago
Created attachment 265327 [details] [diff] [review]
One option

This is one option: if the if statement is false, then just fall back to using whatever we were given. This is such an edge case that I don't think I care what we do so long as we don't crash.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #265327 - Flags: review?(crowder)
(Assignee)

Comment 4

10 years ago
This is now a null ptr dereference btw, so I don't think it's exploitable anymore.

Comment 5

10 years ago
Comment on attachment 265327 [details] [diff] [review]
One option

The fact that this is a getter/setter is lost in the "decompilation" here.  If we're okay with that (as Blake says, it is a very edge case), then this patch is fine.
Attachment #265327 - Flags: review?(crowder) → review+
(Assignee)

Comment 6

10 years ago
So, the other thing is that I'm not sure what else we could do here. Switching to the old style getter/setter syntax would work, but:
js> ({foo getter: []})
typein:1: SyntaxError: invalid getter usage

So it seems that we're doing the best we can (unless we really want to somehow determine if the thing returned is a function).
(Assignee)

Comment 7

10 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
The 1.8 branch still has the original comment 0 symptom in the shell, do we need to backport the fix? Could you characterize the security severity of this bug? (the assertion sounds like potential buffer problems, but I haven't looked at the code.)
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 9

10 years ago
So, I'm not sure that I see an actual buffer overflow here. We will certainly read uninitialized memory (or, at least, memory that we're not expecting to read) but we don't write to it. It might be worth getting these fixes backported, though, since they do make this code a lot more stable and reliable.
(Reporter)

Comment 10

10 years ago
> js> ({foo getter: []})
> typein:1: SyntaxError: invalid getter usage

That's a type error, not a syntax error (see bug 352316).  I'd rather have a type error than be arbitrarily different from the original by omitting the fact that it's a setter.  But it is an edge case :)

Comment 11

10 years ago
Created attachment 266299 [details]
js1_7/extensions/regress-380933.js

Updated

10 years ago
Flags: in-testsuite+
Whiteboard: [sg:moderate]
I get the original assertion in 1.8.1.4, but not in 1.8.0.x

Do we want the fix for bug 367629 on the branch? it seemed to turn this one from a potential problem into a safe-enough null deref.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x-
Flags: blocking1.8.1.5?

Comment 13

10 years ago
My roll-up patch in bug 358594 should carry this patch along with it for the branch.
Depends on: 358594
Thanks for the feedback. Marking blocking on this so we remember to verify the fix using this testcase also.
Flags: blocking1.8.1.5? → blocking1.8.1.5+
(Assignee)

Comment 15

10 years ago
Brian, did you check this into the 1.8 branches as part of another checkin?
Whiteboard: [sg:moderate] → [sg:moderate] checked in already? need feedback=crowder

Comment 16

10 years ago
Yeah, went in with the big rollup patch in bug 358594
Keywords: fixed1.8.1.5

Comment 17

10 years ago
verified fixed js1_7/extensions/regress-380933.js passes 1.8.1 windows/linux/macppc browser/shell 7/16.

on 1.9.0 ./js1_7/extensions/regress-380933.js:60: TypeError: Array.prototype.toSource called on incompatible Function
Keywords: fixed1.8.1.5 → verified1.8.1.5
Group: security

Comment 18

10 years ago
Created attachment 281348 [details]
js1_7/extensions/regress-380933.js

This catches the exception introduced in bug 387501. Norris, can you r+ if the reportMatch works in Rhino?
Attachment #266299 - Attachment is obsolete: true
Attachment #281348 - Flags: review?(norrisboyd)

Comment 19

10 years ago
Norris, comment 18.

Comment 20

10 years ago
Norris, I've gone ahead and checked it in. 

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-380933.js,v  <--  regress-380933.js
initial revision: 1.1

Updated

10 years ago
Attachment #281348 - Flags: review?(norrisboyd)

Comment 21

10 years ago
verified fixed 1.9.0 linux|mac|win
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.