Issue 1019: InnocentCodeRewriter causes valid ES5-strict code to fail

http://code.google.com/p/google-caja/issues/detail?id=1019

  On Thu, Apr 23, 2009 at 5:53 PM, Adam Moore <adamoore@yahoo-inc.com> wrote:
  >[...]
  > YUI = function(o) {
  >
  >    var Y = this;
  >
  >    // Allow instantiation without the new operator
  >    if (!(Y instanceof YUI)) {
  >        return new YUI(o);
  >    } else {
  >[...]

  The pattern above is valid in Valija, ES5-strict, ES5-nonstrict, and
  ES3.  If YUI is called as a function rather than a method or
  constructor, then

  * in ES3 or ES5-nonstrict, 'this' will be the global object.
  * in ES5-strict, 'this' will be 'undefined'.
  * in Valija today, 'this' will be cajita.USELESS. (But we should
    also fix Valija to track ES5-strict.)

  All the above cases work because all of these values are not
  "instanceof YUI".

  However, as Adam reports, it does not currently work as transformed by
  the InnocentCodeRewriter. Currently, to protect against a privilege
  escalation attack, the InnocentCodeRewriter rewrites all functions
  containing 'this' to throw if their 'this' is bound to the global
  object. Now that we understand how this causes valid ES5-strict code
  to fail, we should fix the innocent code transformer here so to track
  ES5-strict.

  We should rewrite all occurrences of 'this' in these functions so
  that, if the real 'this' was bound to the global object, their
  rewritten 'this' will instead be bound to 'undefined'.

  I'm classifying this as Security relevant, since this bug inhibits use
  of the InnocentCodeRewriter, thereby creating a security hazard.


Submitted @3647

[MarkM, 2009-08-08 00:32:46]

Thanks for picking up this issue!

http://codereview.appspot.com/104062/diff/1/3
File src/com/google/caja/parser/quasiliteral/InnocentCodeRewriter.java (right):

http://codereview.appspot.com/104062/diff/1/3#newcode70
Line 70: // bound to the global object.
Did you mean ES5 strict mode behavior? If so, then this comment is incorrect. In
global ES5-strict code, "this" is still bound to the global object. The strict
difference only pertains to "this" within functions -- they are not coerced.
Further, if a strict exophoric function is called as a function, then its "this"
is undefined (by analogy with parameters for absent arguments).

So what should the InnocentCodeRewriter do for a top-level "this"? I think it
should unconditionally initialize "this___" with it. A top level global "this"
does not create the privilege escalation attack we are trying to prevent here.

http://codereview.appspot.com/104062/diff/1/3#newcode109
Line 109: // and emulate HTML5 strict mode behavior where it is undefined if
Did you mean "ES5 strict"?

That is not actually the ES5 strict behavior, but it is the closest we can
feasibly do with a low impact rewriter, and so is what we should do. So only the
comment needs fixing.

To actually emulate ES5 strict, we would need to distinguish between getting
called as a function vs getting called (as a method or reflectively) with the
global object explicitly passed as our "this" binding. I don't know of an easy
enough way to do that.

http://codereview.appspot.com/104062/diff/1/3#newcode114
Line 114: "var this___ = this && !this.___ ? this : void 0;");
The "this &&" here is an interesting technique, but is not quite correct. On
pre-ES5 systems, "this" will always be truthy, so this is only needed for ES5
compatibility. However, on ES5 strict, if "this" were bound to anything falsy,
e.g., "false", this would effectively coerce all falsy "this" values to
"undefined".

[MikeSamuel, 2009-08-08 01:10:15]

http://codereview.appspot.com/104062/diff/1/3
File src/com/google/caja/parser/quasiliteral/InnocentCodeRewriter.java (right):

http://codereview.appspot.com/104062/diff/1/3#newcode70
Line 70: // bound to the global object.
On 2009/08/08 00:32:46, MarkM wrote:
> Did you mean ES5 strict mode behavior? If so, then this comment is incorrect.
In
> global ES5-strict code, "this" is still bound to the global object. The strict
> difference only pertains to "this" within functions -- they are not coerced.
> Further, if a strict exophoric function is called as a function, then its
"this"
> is undefined (by analogy with parameters for absent arguments).
> 
> So what should the InnocentCodeRewriter do for a top-level "this"? I think it
> should unconditionally initialize "this___" with it. A top level global "this"
> does not create the privilege escalation attack we are trying to prevent here.

Done.

http://codereview.appspot.com/104062/diff/1/3#newcode109
Line 109: // and emulate HTML5 strict mode behavior where it is undefined if
On 2009/08/08 00:32:46, MarkM wrote:
> Did you mean "ES5 strict"?

Yes

> That is not actually the ES5 strict behavior, but it is the closest we can
> feasibly do with a low impact rewriter, and so is what we should do. So only
the
> comment needs fixing.

Done

> To actually emulate ES5 strict, we would need to distinguish between getting
> called as a function vs getting called (as a method or reflectively) with the
> global object explicitly passed as our "this" binding. I don't know of an easy
> enough way to do that.

http://codereview.appspot.com/104062/diff/1/3#newcode114
Line 114: "var this___ = this && !this.___ ? this : void 0;");
On 2009/08/08 00:32:46, MarkM wrote:
> The "this &&" here is an interesting technique, but is not quite correct. On
> pre-ES5 systems, "this" will always be truthy, so this is only needed for ES5
> compatibility. However, on ES5 strict, if "this" were bound to anything falsy,
> e.g., "false", this would effectively coerce all falsy "this" values to
> "undefined".

Fixed

[MarkM, 2009-08-08 01:40:23]

LGTM

http://codereview.appspot.com/104062/diff/1005/1006
File tests/com/google/caja/parser/quasiliteral/InnocentCodeRewriterTest.java
(right):

http://codereview.appspot.com/104062/diff/1005/1006#newcode156
Line 156: "assertEquals(assertEquals, this.assertEquals);",
What is this testing?