JSOP_CLOSURE unconditionally replaces properties of the variable object

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
10 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.0.15, testcase, verified1.8.1.12})

Trunk
fixed1.8.0.15, testcase, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
blocking1.8.0.next +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high?])

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
Created attachment 291203 [details]
test case overwriting window.document

Currently JSOP_CLOSURE unconditionally calls OBJ_DEFINE_PROPERTY on the variable object without checking that the property is not ReadOnly or DontDelete. It allows to replace const properties of the global object. The attached example shows that altering window.document.  

Compare that with JSOP_DEFFUN case which since landing the bug 21618 explicitly prevents such overwrites.
Flags: blocking1.9?

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(Assignee)

Comment 1

10 years ago
The patch from bug 406477 comment 10 includes the fix this bug through code sharing between JSOP_DEFUN and JSOP_CLOSURE.
Created attachment 291582 [details]
plain diff of JSOP_{DEFFUN,CLOSURE} code, w/o comments&whitespace

These are so close, I like the idea of sharing code, as you did in the other bug's patch. Could we share more? We do not want js_CheckRedeclaration if JSOP_DEFFUN. We could avoid js_GetScopeChain for that op too. Thoughts?

/be
(Assignee)

Comment 3

10 years ago
(In reply to comment #2)
> We do not want js_CheckRedeclaration if JSOP_DEFFUN.

It is exactly the js_CheckRedeclaration call in JSOP_DEFFUN that prevents this bug to happen there so adding it for JSOP_CLOSURE should addresses the issue as well. Then JSOP_CLOSURE/JSOP_DEFFUN would be essentially the same except for the scope chain initialization since it is known in JSOP_DEFFUN that the block chain is null. Plus there is a special support for the eval in DEFFUN and its absence in CLOSURE. 

The latter exposes yet another bug since it means that eval("if (1) function f() {}") creates f as a permanent property and the patch from bug 406477 comment 10 solves this as well.
(Assignee)

Comment 4

10 years ago
To Damon Sicore:

The patch bug 406477 comment 10 contains includes a fix for this bug under the carpet. Since the bug exists on 1.8.1 and its consequences can be rather bad, it would be nice to fix this under a cover bug on the trunk. Otherwise a diff of targeted fix for the trunk would point to the problem nicely.
(Assignee)

Comment 5

10 years ago
The bug was fixed as a part of a check in for bug 406477.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.12? → blocking1.8.1.12+
(Assignee)

Comment 6

10 years ago
Created attachment 296606 [details] [diff] [review]
patch for branch

For minimality the patch just protect OBJ_DEFINE_PROPERTY with js_CheckRedeclaration in JSOP_CLOSURE in the same way JSOP_DEFFUN does. I also removed that #if 0 code since it referes to attrs which the patch modifies and since the code is removed on the trunk.
Attachment #296606 - Flags: review?(brendan)

Updated

10 years ago
Attachment #296606 - Flags: review?(brendan)
Attachment #296606 - Flags: review+
Attachment #296606 - Flags: approval1.8.1.12?
Comment on attachment 296606 [details] [diff] [review]
patch for branch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #296606 - Flags: approval1.8.1.12? → approval1.8.1.12+
QA: To verify, ensure that there are only two lines of content (both with only {}) when viewing the attached testcase.

Currently, on branch, you will see "function document() {return 1;}" as a third line. This should not be the case and is not the case on trunk.
Keywords: testcase
(Assignee)

Comment 9

10 years ago
I checked in the patch from comment 6 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.94; previous revision: 3.181.2.93
done
Keywords: fixed1.8.1.12
Verified for branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre.
Keywords: fixed1.8.1.12 → verified1.8.1.12
Whiteboard: [sg:?]
Group: security
Whiteboard: [sg:?] → [sg:high?]

Comment 11

10 years ago
Comment on attachment 296606 [details] [diff] [review]
patch for branch

a=asac for 1.8.0.15
Attachment #296606 - Flags: approval1.8.0.15+

Updated

10 years ago
Flags: blocking1.8.0.15+

Updated

10 years ago
Keywords: checkin-needed
MOZILLA_1_8_0_BRANCH:

Checking in js/src/jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.35; previous revision: 3.181.2.17.2.34
done
Keywords: checkin-needed → fixed1.8.0.15

Comment 13

10 years ago
verified linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-406572.js,v  <--  regress-406572.js
initial revision: 1.1
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.