Closed
Bug 539766
Opened 15 years ago
Closed 14 years ago
Object.defineProperty can set arguments.length without setting the "tampered with" bit
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: jorendorff, Assigned: Waldo)
Details
(Whiteboard: [ccbr] fixed-in-tracemonkey)
Attachments
(1 file)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
function f() {
Object.defineProperty(arguments, "length", {value: 4});
assertEq(arguments.length, 4);
}
f();
Here the assertion fails because the interpreter (in JSOP_ARGCNT) doesn't bother looking at the actual property. It looks at the "tampered with" bit of argobj->fslots[JSSLOT_ARGS_LENGTH], which is clear, and assumes it can use fp->argc.
This can be used to trigger a crash near NULL:
function f() {
var q = arguments;
Object.defineProperty(arguments, "length", {value: {}});
for (var i = 0; i < 20; i++) {
print(q.length);
}
}
f();
Related: bug 539553
Comment 1•15 years ago
|
||
When defining a data property over top of a "data, but really JSPropertyOp native accessor" property, should we call clasp->delProperty? args_delProperty would set the magic override bit.
/be
Assignee | ||
Comment 2•15 years ago
|
||
I can believe that.
Comment 3•15 years ago
|
||
Do the length guards added in bug 539553 fix this bug too? If so we should still probably add this variant testcase to the regression test suite.
If not, is this one also a regression like bug 539553? If so please set status1.9.1 to "unaffected" so we don't have to worry it.
Where does the near-null value come from? Is it always going to be near-null, or would different values for "value" get us somewhere more interesting?
Whiteboard: [sg:critical?]
Assignee | ||
Updated•15 years ago
|
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•15 years ago
|
||
I would doubt the guards there fix this bug.
Since Object.defineProperty is new in 1.9.3, 1.9.1 is unaffected. Likewise for 1.9.2.
Having not looked, I can't comment on the near-null value.
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Comment 5•15 years ago
|
||
No longer crashes tm tip -- suggest unrestricting access and lowering sg: rating.
/be
Updated•15 years ago
|
Group: core-security
Whiteboard: [sg:critical?]
Updated•15 years ago
|
blocking2.0: --- → beta1+
Updated•15 years ago
|
Priority: -- → P2
Updated•15 years ago
|
blocking2.0: beta1+ → beta2+
Updated•14 years ago
|
blocking2.0: beta2+ → betaN+
Assignee | ||
Comment 6•14 years ago
|
||
Still correctness bugs lurking here, although I believe nothing more interesting than that:
[jwalden@find-waldo-now tests]$ ../dbg/js -j
js> function test()
{
var length = "length";
Object.defineProperty(arguments, length, { value: 17 });
print(arguments.length);
print(arguments[length]);
}
js> test()
0
17
Looking at this now to figure out the best course of action...
Assignee | ||
Comment 7•14 years ago
|
||
There are actually quite a few ways this botch is observable, beyond the one instance I noted above: everything that ever relies on the value gotten by js_GetLengthProperty, which is quite a bit! There are also a couple one-off locations that call isArgsLengthOverridden() directly, where the arguments object might have a bogus override bit. (Some uses are inside getters/setters that wouldn't be invoked by a redefined arguments.length, because once that happened you'd have to delete arguments.length to get such a property, but you'd then invoke args_delProperty and end up flipping the bit in the process.)
The tests here should cover all the one-off cases, and they cover the first few js_GetLengthProperty uses (the first few before I realized I'd be writing a dozen-odd more intricate tests to cover them all, for not a ton of likely real-world prevention). More could be written, but I suspect doing so wouldn't win us enough (if any) bug prevention/detection down the road to make it worthwhile.
Attachment #479982 -
Flags: review?(brendan)
Comment 8•14 years ago
|
||
Comment on attachment 479982 [details] [diff] [review]
Patch and tests
Good general fix, phrased in terms of old JSClass hooks. We truly need to re-code our internal MOP in ES5 terms.
/be
Attachment #479982 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 9•14 years ago
|
||
Whiteboard: [ccbr] → [ccbr] fixed-in-tracemonkey
Comment 10•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•