All ReadOnly properties can be overwritten by using Object and try..throw..catch

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: Aiko, Assigned: mrbkap)

Tracking

({testcase, verified1.8.0.5, verified1.8.1})

Trunk
mozilla1.9alpha1
testcase, verified1.8.0.5, verified1.8.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
With a little bit of trickery, all ReadOnly properties can be set to any value:

1. Set Object to a function returning the object whose property you want to write to.
2. throw the new value
3. catch the new value and let the catch clause identifier be the property name you want to overwrite.

As far as I can tell this is merely a trivial problem for the JS shell. I don't know whether it's possible to overwrite vital data in the browser this way, though.
(Reporter)

Comment 1

12 years ago
Created attachment 222774 [details]
testcase for shell or browser
(Assignee)

Comment 2

12 years ago
Hmm, this might actually be per-spec. There's no wording in the spec to tell the implementation here to find the original Object constructor or prototype or anything like that.

On the other hand, if we wanted to do that, we'd need a new way for the exception-emitting code to lookup the original Object constructor. Currently, it emits |name "Object"; pushobj; newinit| which will find the overridden Object. Do we need a new bytecode?
OS: Windows Server 2003 → All
Hardware: PC → All
(Reporter)

Comment 3

12 years ago
The wording regarding ReadOnly properties seems unambigous, see section 4.2: "when the ReadOnly attribute for a property is set to true, any attempt by executed ECMAScript code to change the value of the property has no effect." 

I believe the right fix would be the following: If the "new Object" returned an object with has a ReadOnly property with the same name as the catch identifier, don't put the thrown value on that object and execute the catch block. The catch block would not have access to the thrown value but who cares? I believe this is what IE and Opera do.

Alternatively, simply make Object ReadOnly. That's not compatible with the spec but may be more sensible. Is there a good reason to overwrite Object anyway?
(Assignee)

Comment 4

12 years ago
(In reply to comment #3)
> I believe the right fix would be the following: If the "new Object" returned an
> object with has a ReadOnly property with the same name as the catch identifier,
> don't put the thrown value on that object and execute the catch block. The

I thought about that, but this loses the returned value. Further thinking led me to the same conclusion "who cares". I say the answer to that question is "nobody".

Similarly, this patch changes the behavior of |try { throw 42 } catch(__parent__) { print(__parent__) }| I think that anybody depending on the old behavior was broken anyway...

> Alternatively, simply make Object ReadOnly. That's not compatible with the spec
> but may be more sensible. Is there a good reason to overwrite Object anyway?

We've been considering this, and it's currently proposed for ECMAScript v4. Brendan considers this too much of a compatibility hit for Firefox 2, however.
(Assignee)

Comment 5

12 years ago
Created attachment 222788 [details] [diff] [review]
Minimal fix
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #222788 - Flags: review?(brendan)
Comment on attachment 222788 [details] [diff] [review]
Minimal fix

>             /* Define obj[id] to contain rval and to be permanent. */
>             SAVE_SP_AND_PC(fp);
>-            ok = OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL,
>-                                     JSPROP_PERMANENT, NULL);

The "Define obj[id]..." comment seems lost now, or a bit premature at any rate.

>+
>+            /*
>+             * It's possible for an evil script to substitute a random object
>+             * for the new object. Check to make sure that we don't override a
>+             * readonly property with the below OBJ_DEFINE_PROPERTY.
>+             */
>+            ok = OBJ_GET_ATTRIBUTES(cx, obj, id, NULL, &attrs);
>             if (!ok)
>                 goto out;
>+            if (!(attrs & JSPROP_READONLY)) {

This is good, it mimics ECMA-262 [[CanPut]] testingn from [[Put]].  It's conservative, in that if someone forgot to use JSPROP_PERMANENT as well, we would not overwrite, even though readonly without permanent is futile in light of delete.

Anyway, this is the "new opcode" -- a fixed version of the old JSOP_INITCATCHVAR, which was always buggy in light of ECMA-262 Edition 3's inconsistent choice to use the current value of 'Object' when constructing the scope for the catch block (see 12.14, semantics for the Catch production).  For Edition 4 we will make catch blocks use lexical scope.  I'll probably do this for JS1.7 if there is time.

r=me with comment nit picked, if it can be picked cleanly.

/be
Attachment #222788 - Flags: review?(brendan) → review+
(Assignee)

Comment 7

12 years ago
Fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 8

12 years ago
Created attachment 222948 [details] [diff] [review]
What I checked in

Nominating for the branches. This is a small fix and given that the severity of the problem here is unknown, it seems better to be safe than sorry.
Attachment #222788 - Attachment is obsolete: true
Attachment #222948 - Flags: review+
Attachment #222948 - Flags: approval1.8.0.5?
Attachment #222948 - Flags: approval-branch-1.8.1?(brendan)
Comment on attachment 222948 [details] [diff] [review]
What I checked in

Agreed -- this should go into the 1.8.0 branch too.

/be
Attachment #222948 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
(Reporter)

Comment 10

12 years ago
In the browser the testcase still sets document.documentElement, which usually results in a "setting a property that has only a getter" error. Is this behaviour expected and harmless?
(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

12 years ago
Created attachment 224473 [details] [diff] [review]
Fix that too

With the current trunk, the testcase fails to work at all (thanks to the early |goto out| in js_Interpret), but this patch will fix it anyway (since we shouldn't blow over permanent properties or properties that already have getters or setters).
Attachment #224473 - Flags: review?(brendan)
Comment on attachment 224473 [details] [diff] [review]
Fix that too

Great -- we'll get this into the js1.7a landing for the 1.8 branch, but could you also file a bug for after that landing, to switch to an ES4-like lexical block for catch vars?  Thanks,

/be
Attachment #224473 - Flags: review?(brendan)
Attachment #224473 - Flags: review+
Attachment #224473 - Flags: approval1.8.0.5?
(Assignee)

Comment 13

12 years ago
Fix checked into trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Comment on attachment 222948 [details] [diff] [review]
What I checked in

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #222948 - Flags: approval1.8.0.5? → approval1.8.0.5+
Comment on attachment 224473 [details] [diff] [review]
Fix that too

approved for 1.8.0 branch, a=dveditz for drivers

Please land these on the 1.8 branch also
Attachment #224473 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 16

12 years ago
Fix checked into the 1.8 branches.
Keywords: fixed1.8.0.5, fixed1.8.1
Is it intentional that the exception not be caught at all in:

Object = function () { return Math };
try 
{ 
  throw 1990; 
}
catch (LN2) 
{
}

(Assignee)

Comment 18

12 years ago
(In reply to comment #17)
> Is it intentional that the exception not be caught at all in:

I don't think so. Could you file a bug on this?
Checking in regress-338709.js;
/cvsroot/mozilla/js/tests/js1_5/Object/regress-338709.js,v  <--  regress-338709.js
initial revision: 1.1

passes in 1.8.0.5, 1.8.1 winxp from today but fails due to uncaught exception in 1.9a1

filed Bug 341828
Flags: in-testsuite+

Comment 20

12 years ago
Verified fixed on mac 1.5.0. branch build id 2006062008 and today's 2.0 branch build. The testcase does not display on trunk.
Status: RESOLVED → VERIFIED

Updated

12 years ago
Keywords: fixed1.8.0.5, fixed1.8.1 → verified1.8.0.5, verified1.8.1
You need to log in before you can comment on or make changes to this bug.