Reject, in the mitigator, JS reserved names written with escapes, to avoid misinterpretation.

Acorn and escodegen are inconsistent in their handling of program
text like de\u006Cete (an escaped reserved word). Acorn parses it as an
identifier and unescapes it, but escodegen takes the resulting parse
tree node and renders it as "delete", thus changing the interpretation
of the program. Furthermore, JS implementations also differ in their
interpretation of an escaped reserved word; therefore, we cannot even
make this necessarily safe by changing the code generator.

Fix this by rejecting all programs which we parse and which contain
identifiers which match reserved words (except for positions where
reserved words are allowed such as "foo.de\u006Cete").

Fixes <https://code.google.com/p/google-caja/issues/detail?id=1867>.

While attempting to fix this problem, I ran into issues with upgrading
Acorn and escodegen. Supporting changes to help future such effort:
* Run SES mitigation tests with minified source as well as unminified.
* Add a test for an apparent undiagnosed bug in our minifier which
  causes the mitigator to mangle regexp literals if minified.

Other supporting changes:
* test-ses-mitigation.html uses createExports.js/exportsToSES.js instead
  of custom glue.
* Normalized indentation of test cases in test-ses-mitigation.js.
* The mitigator now passes through the specific error message from
  errors thrown while parsing.

@r5630

[MarkM, 2013-10-30 01:16:31]

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:128: var table = {};
Since we're now willing to assume ES5, an object used as a StringMap should be
created with Object.create(null) rather than {}. Then the
Object.prototype.hasOwnProperty.call could be an 'in' test.

Or better would be to just use a StringMap.

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:361: 'ambiguous and not currently
permitted by SES.');
The programs are not ambiguous -- the spec says which behavior is correct. Put
the blame where it belongs:

Programs containing Unicode escapes in reserved words will misbehave on some
browsers, and so are not...

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:451: // "Error {}" so we use the
explicit stringification.
This surprised me, so I tested and you are right. FYI, there is a way to get
even better, using the "%o" formatting directive in the format string, as shown
in the attachment at
https://code.google.com/p/google-caja/issues/detail?id=1867#c11

This was just FYI. You change here is fine as is. No action needed.

https://codereview.appspot.com/19560044/diff/1/tests/com/google/caja/ses/test...
File tests/com/google/caja/ses/test-ses-mitigation.js (right):

https://codereview.appspot.com/19560044/diff/1/tests/com/google/caja/ses/test...
tests/com/google/caja/ses/test-ses-mitigation.js:73: var o = {
rewritePropertyUpdateExpr: true };
The old indentation looks correct. This new indentation looks wrong. What rule
are you following here?

[MarkM, 2013-10-30 13:31:33]

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:37: // There is a bug or undefined
behavior which is problematic for us:
Please add to this comment the URLs

https://bugs.ecmascript.org/show_bug.cgi?id=277
https://code.google.com/p/v8/issues/detail?id=2222
https://bugzilla.mozilla.org/show_bug.cgi?id=744784
https://bugzilla.mozilla.org/show_bug.cgi?id=694360
https://bugs.webkit.org/show_bug.cgi?id=90678

[kpreid2, 2013-10-30 18:41:55]

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:37: // There is a bug or undefined
behavior which is problematic for us:
On 2013/10/30 13:31:34, MarkM wrote:
> Please add to this comment the URLs
> 
> https://bugs.ecmascript.org/show_bug.cgi?id=277
> https://code.google.com/p/v8/issues/detail?id=2222
> https://bugzilla.mozilla.org/show_bug.cgi?id=744784
> https://bugzilla.mozilla.org/show_bug.cgi?id=694360
> https://bugs.webkit.org/show_bug.cgi?id=90678

Done. Also rephrased to acknowledge that the is-a-reserved-word interpretation
is correct.

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:128: var table = {};
On 2013/10/30 01:16:32, MarkM wrote:
> Since we're now willing to assume ES5,

Since this is SES support code, it has always assumed ES5.

> an object used as a StringMap should be
> created with Object.create(null) rather than {}. Then the
> Object.prototype.hasOwnProperty.call could be an 'in' test.

Object.create(null) was broken so we currently do not require it anywhere. See
https://code.google.com/p/google-caja/issues/detail?id=1811
That bug is probably dead, so we should probably back out the kludges, but until
then this is consistent.

> Or better would be to just use a StringMap.

This means loading another copy of StringMap in the utility frame, but that's
short enough that it's not worth reinventing here. Done.

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:361: 'ambiguous and not currently
permitted by SES.');
On 2013/10/30 01:16:32, MarkM wrote:
> The programs are not ambiguous -- the spec says which behavior is correct. Put
> the blame where it belongs:
> 
> Programs containing Unicode escapes in reserved words will misbehave on some
> browsers, and so are not...

Reworded:

Programs containing Unicode escapes in reserved words will be misparsed on some
platforms and are not currently permitted by SES.

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:451: // "Error {}" so we use the
explicit stringification.
On 2013/10/30 01:16:32, MarkM wrote:
> This surprised me, so I tested and you are right. FYI, there is a way to get
> even better, using the "%o" formatting directive in the format string, as
shown
> in the attachment at
> https://code.google.com/p/google-caja/issues/detail?id=1867#c11

Is this supported in all console-having browsers including IE? In any case,
given that this is parameterized (I didn't look at why) I'd rather avoid
requiring the caller to implement % codes, so leaving it as is.

https://codereview.appspot.com/19560044/diff/1/tests/com/google/caja/ses/test...
File tests/com/google/caja/ses/test-ses-mitigation.js (right):

https://codereview.appspot.com/19560044/diff/1/tests/com/google/caja/ses/test...
tests/com/google/caja/ses/test-ses-mitigation.js:73: var o = {
rewritePropertyUpdateExpr: true };
On 2013/10/30 01:16:32, MarkM wrote:
> The old indentation looks correct. This new indentation looks wrong. What rule
> are you following here?

The rule which is followed by all the rest of our tests and much other code
besides. Code of the form
  foo(..., function() { ...long body... });
is formatted as
  foo(..., function() {
    ...long body...
  });
and when the first line is too long it is wrapped with four-space indent.

[kpreid2, 2013-10-30 18:44:15]

Acorn and escodegen are inconsistent in their handling of program
text like de\u006Cete (an escaped reserved word). Acorn parses it as an
identifier and unescapes it, but escodegen takes the resulting parse
tree node and renders it as "delete", thus changing the interpretation
of the program. Furthermore, JS implementations also differ in their
interpretation of an escaped reserved word; therefore, we cannot even
make this necessarily safe by changing the code generator.

Fix this by rejecting all programs which we parse and which contain
identifiers which match reserved words (except for positions where
reserved words are allowed such as "foo.de\u006Cete").

Fixes <https://code.google.com/p/google-caja/issues/detail?id=1867>.

While attempting to fix this problem, I ran into issues with upgrading
Acorn and escodegen. Supporting changes to help future such effort:
* Run SES mitigation tests with minified source as well as unminified.
* Add a test for an apparent undiagnosed bug in our minifier which
  causes the mitigator to mangle regexp literals if minified.

Other supporting changes:
* test-ses-mitigation.html uses createExports.js/exportsToSES.js instead
  of custom glue.
* Normalized indentation of test cases in test-ses-mitigation.js.
* The mitigator now passes through the specific error message from
  errors thrown while parsing.

[MarkM, 2013-10-30 18:52:55]

LGTM

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/19560044/diff/1/src/com/google/caja/ses/mitiga...
src/com/google/caja/ses/mitigateGotchas.js:451: // "Error {}" so we use the
explicit stringification.
On 2013/10/30 18:41:56, kpreid2 wrote:
> On 2013/10/30 01:16:32, MarkM wrote:
> > This surprised me, so I tested and you are right. FYI, there is a way to get
> > even better, using the "%o" formatting directive in the format string, as
> shown
> > in the attachment at
> > https://code.google.com/p/google-caja/issues/detail?id=1867#c11
> 
> Is this supported in all console-having browsers including IE? In any case,
> given that this is parameterized (I didn't look at why) I'd rather avoid
> requiring the caller to implement % codes, so leaving it as is.

I believe it is supported everywhere, but leaving it as is here is fine.

https://codereview.appspot.com/19560044/diff/1/tests/com/google/caja/ses/test...
File tests/com/google/caja/ses/test-ses-mitigation.js (right):

https://codereview.appspot.com/19560044/diff/1/tests/com/google/caja/ses/test...
tests/com/google/caja/ses/test-ses-mitigation.js:73: var o = {
rewritePropertyUpdateExpr: true };
On 2013/10/30 18:41:56, kpreid2 wrote:
> On 2013/10/30 01:16:32, MarkM wrote:
> > The old indentation looks correct. This new indentation looks wrong. What
rule
> > are you following here?
> 
> The rule which is followed by all the rest of our tests and much other code
> besides. Code of the form
>   foo(..., function() { ...long body... });
> is formatted as
>   foo(..., function() {
>     ...long body...
>   });
> and when the first line is too long it is wrapped with four-space indent.

Ah, good point. I see that now. Thanks.

https://codereview.appspot.com/19560044/diff/20001/src/com/google/caja/ses/mi...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/19560044/diff/20001/src/com/google/caja/ses/mi...
src/com/google/caja/ses/mitigateGotchas.js:144: return table.has.bind(table);
cute

[kpreid2, 2013-10-30 19:00:30]

Thanks, MarkM. Ihab?

[Jasvir, 2013-10-30 19:44:46]

LGTM with the fixes.

[kpreid2, 2013-10-30 19:47:21]

On 2013/10/30 19:44:46, Jasvir wrote:
> LGTM with the fixes.

Okay, this is hereby an undisclosed security patch ready to go.