Bug 15003 – Function.prototype.constructor should not be DontDelete/ReadOnly (Acid3 bug)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jeff Walden (remove +bwo to email)]

13.2 Creating Function Objects

10. Set the "constructor" property of Result(9) [the newly-created prototype object for the Function object being created] to F [the Function object being created].  This property is given attributes { DontEnum }.


This breaks hiding the constructor of an object from any code which sees that object, as an internals-hiding mechanism for constructor privateness.


The relevant place to fix, at a glance, is:

http://trac.webkit.org/projects/webkit/browser/trunk/JavaScriptCore/kjs/function_object.cpp#L248

It should be trivial to convert the URL test into a JavaScriptCore test and to remove the extra attributes from that line -- for someone who has the time to do that, that is.

[David Kilzer (:ddkilzer)]

I believe there may be similar changes required in JavaScriptCore/kjs/nodes.cpp for FuncDeclNode::processFuncDecl(ExecState*) and FuncExprNode::evaluate(ExecState*) if this does indeed fix the bug.  (I think the duplicate code should be refactored into its own method as well!)

This bug reminded me of revision r19203:
http://trac.webkit.org/projects/webkit/changeset/19203

Test case confirmed with a local debug build of WebKit r25142 with Safari 3 Public Beta v. 3.0.3 (522.12.1) on Mac OS X 10.4.10 (8R218).

Both Opera 9.22 and Firefox 2.0.0.6 pass the given test.

[Ian 'Hixie' Hickson]

In the DontDelete test: I don't understand why the instance's constructor property would become Object.prototype.constructor instead of being removed altogether.

[Jeff Walden (remove +bwo to email)]

(In reply to comment #2)
> In the DontDelete test: I don't understand why the instance's constructor
> property would become Object.prototype.constructor instead of being removed
> altogether.

It *is* removed, but you're only removing it from c.prototype, which per 13.2.2 step 4 and 13.2 steps 9-11 is a value created as if by new Object().  That object has as its prototype the Object prototype object (15.2.4), which has a constructor property which is Object.prototype.constructor, i.e. the built-in Object constructor (15.2.4.1), so getting the constructor property of |new c()| won't find it on that object, won't find it on the auto-created prototype, but will find it on Object.prototype.

[Eric Seidel (no email)]

Acid3, test 87 tests for this bug.  I would think it would be as simple as changing:
JSObject* FunctionObjectImp::construct(ExecState* exec, const List& args, const Identifier& functionName, const UString& sourceURL, int lineNumber)

to not put() the constructor property with ReadOnly an DontDelete.

[Sam Weinig]

As Mr. Kilzer has already pointed out, you would also need to change FuncDeclNode::makeFunction.

[Ian 'Hixie' Hickson]

(In reply to comment #3)

Thanks, that makes sense.

[Darin Adler]

Created attachment 19003 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 19003 [details]
patch

the code change looks fine.  I have concerns about the testing coverage.  It should be possible to create a test which walked all functions on the window object and checked to make sure that their prototype.constructor was mutable, no?  What about all of the HTML* elements which are being changed by your bindings .pl change.  I would think those could be tested in some automated way.  Perhaps the ideal for this sort of thing is to find a way for fast/window-properties.html  to dump all of the DontDelete|DontEnum|ReadOnly information.

[Darin Adler]

(In reply to comment #8)
> It should be possible to create a test which walked all functions on the window
> object and checked to make sure that their prototype.constructor was mutable,
> no?  What about all of the HTML* elements which are being changed by your
> bindings .pl change.  I would think those could be tested in some automated
> way.

The reason I included the Node test was that I wanted to test at least one auto-generated example. I agree that we could expand the test to make sure it tests all the other generated constructors too.

[Jeff Walden (remove +bwo to email)]

For the record, since I recently heard skepticism expressed on IRC over the value of this:

Sometime near the beginning of August, Ajaxian had a blurb on Neil Mix's examination of Facebook's application platform and its security.  For those who don't know, Facebook's app system consists of writing apps in a subset of HTML and JavaScript, modulo some Facebook-specific additions.  The usual suspects are removed from HTML to make it safe, and the JS is run through a source-to-source translator to prevent unguarded access to any global variables, properties, etc.  Runtime checks are inserted in various places to make checks which can't be done statically.  This final code is then run on the facebook.com domain, with the full privileges of any facebook.com JS.  The rewriting and checking are critical to preventing arbitrary XSS.

I read Neil's posts on the exploits he found, and I decided to do some investigation of my own.  Over the next couple weeks I found a good handful of exploits beyond the ones Neil found, as did a coworker at the time.  Many of the exploits relied on Mozilla-specific features because that's what I happen to know best, but a fair number of them would work in any browser.

By now you might be able to see where this is going.  Neil Mix's original exploit was to do the following:

 (function(){}).constructor("alert('hi')")();

He reported this on his blog and noted the Facebook fix:

 Function.prototype.constructor = null;

This of course failed silently and horribly in Safari -- but nobody noticed.  Only later during the exploit-finding, when we realized that their strategy for preventing access to Facebook-internal methods (fbjs_event being one, if I remember correctly) was in each constructor to assign |this.constructor = null;|, was it discovered this didn't work in Safari.  In the end the solution was to ban accessing the constructor property of any object, ever -- through translation-time changes when possible, and through runtime checks of property accesses when not.  (This was already necessary for __defineGetter__, __defineSetter__, __proto__, caller, and several other properties, so it wasn't onerous, but it *was* unnecessary.)

Either having constructor be deletable or mutable -- either one -- would have fixed this problem and allowed the constructor property to be safely exposed (assuming proper vigilance by Facebook in removing the constructor from their own functions, when needed).  Real-world code relied (probably not based on a spec reading, but whatever) on changing/removing the constructor property from JS objects for security, and it was broken by this bug.

This bug report was the result of email conversation with Facebook's security people after they discovered the problem; I didn't think it a great idea to be explicit about what the problem was as they hadn't fixed the problem at the time, so I hand-waved a bit and claimed this was only important "as an internals-hiding mechanism", but that wasn't really the whole story.

[Darin Adler]

(In reply to comment #10)
> For the record, since I recently heard skepticism expressed on IRC over the
> value of this:

I think you probably misunderstood what you overheard.

Maciej and I were joking about our responsibilities at Apple. It wasn't about the importance of this particular bug at all.

[Darin Adler]

(In reply to comment #9)
> The reason I included the Node test was that I wanted to test at least one
> auto-generated example. I agree that we could expand the test to make sure it
> tests all the other generated constructors too.

By "the Node test", I mean the test case that uses a Text node.

[Darin Adler]

Committed revision 30102.