Fix and backstop bugs in finding identifier names.

Committed as 739a368f287d119c263428367df1dffd98906d0f .


ES6 extended the syntax of identifiers to include the unicode escape
sequence backslash-u-opencurly-hexdigits-closecurly. This meant that
untrusted code could name global variables that we would not notice as
a single token, and thus would not censor.

Since this regexp-based recognition of possible variable name mentions
seems fragile, we added an additional backstop mechanism: we sample
the set of global variable names and build a backstop object with a
poisoned accessor for each of these names. Each actual scopeObject
then inherits from this accessor. Thus, if atLeastFreeVarNames missed
a name, but that was the name of a global variable when the backstop
was built, then the poisoned accessor on the backstop will intercept
it instead.

We add a new ses.resampleGlobal() to the privileged api, so our client
can advise when they've added additional global variables, so we can
resample.

This change builds on https://codereview.appspot.com/285330043/ ,
which should be considered our diffbase.

[MarkM, 2016-03-05 06:04:03]

Aside from carrying forward the tests from our diffbase, snapshot 1 adds no
tests. Since the backstop should only be engaged if there's a vulnerability
elsewhere, how should we test it?

I remain largely ignorant of our testing framework. Since this bug is again
urgent, can we do an "I'll write fix and you review, you write tests of the fix
and I'll review" thing again?

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:571: ses.resampleGlobal = resampleGlobal;
Kevin, I looked at guest-manager.js but was not able to figure out where it
should call ses.resampleGlobal(). Please advise.

[MarkM, 2016-03-05 06:08:00]

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:571: ses.resampleGlobal = resampleGlobal;
On 2016/03/05 06:04:03, MarkM wrote:
> Kevin, I looked at guest-manager.js but was not able to figure out where it
> should call ses.resampleGlobal(). Please advise.

I should mention that the first sampling is delayed until first needed, which
perhaps already accomplishes what this call from run() was supposed to
accomplish.

[kpreid_google, 2016-03-07 22:18:49]

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/atLea...
File src/com/google/caja/ses/atLeastFreeVarNames.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/atLea...
src/com/google/caja/ses/atLeastFreeVarNames.js:176: // JSON.parse will reject
with an error.
This sentence is too merely descriptive. State that this limitation is for
simplicity rather than security.

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/compi...
File src/com/google/caja/ses/compileExprLater.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/compi...
src/com/google/caja/ses/compileExprLater.js:106: var quotedNames =
prep.freeNames.map(JSON.stringify);
Why not JSON.stringify(prep.freeNames) instead?

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:571: ses.resampleGlobal = resampleGlobal;
On 2016/03/05 06:04:03, MarkM wrote:
> Kevin, I looked at guest-manager.js but was not able to figure out
> where it should call ses.resampleGlobal(). Please advise.

Looked; it actually should go in ses-frame-group.js, function sesRun. I'd
suggest between the 'imports' section and the 'promise' section.

> I should mention that the first sampling is delayed until first needed,
> which perhaps already accomplishes what this call from run() was
> supposed to accomplish.

Perhaps, but I'm feeling cautious, and this does little harm.

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:581: * makeScopeOpject protections fail.
typo: "Opject"

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:936: // atLeastFreeVarNames is likely to be
more informative.
"…more informative, so it is called first."

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

https://codereview.appspot.com/283510043/diff/1/tests/com/google/caja/ses/tes...
tests/com/google/caja/ses/test-ses-parts.js:84:
jsunitRegister('testAtLeastFreeVarNamesOutput', function() {
We want a test which checks that \u{...} escapes are rejected. (In the event
that they are later supported, we can add examples of them to the two existing
tests instead.) Here's the test skeleton; I leave it to you to invent test data:

jsunitRegister('testAtLeastFreeVarNamesOnNewUnicodeEscapes', function() {
  var names = [...];
  names.forEach(function(name) {
    try {
      ses.atLeastFreeVarNames(name);
    } catch (e) {
      assertTrue(e instanceof SyntaxError);
      return;
    }
    fail('Unexpectedly succeeded to parse ' + name);
  });
  jsunitPass();
});

[MarkM, 2016-03-08 03:52:11]

ES6 extended the syntax of identifiers to include the unicode escape
sequence backslash-u-opencurly-hexdigits-closecurly. This meant that
untrusted code could name global variables that we would not notice as
a single token, and thus would not censor.

Since this regexp-based recognition of possible variable name mentions
seems fragile, we added an additional backstop mechanism: we sample
the set of global variable names and build a backstop object with a
poisoned accessor for each of these names. Each actual scopeObject
then inherits from this accessor. Thus, if atLeastFreeVarNames missed
a name, but that was the name of a global variable when the backstop
was built, then the poisoned accessor on the backstop will intercept
it instead.

We add a new ses.resampleGlobal() to the privileged api, so our client
can advise when they've added additional global variables, so we can
resample.

This change builds on https://codereview.appspot.com/285330043/ ,
which should be considered our diffbase.

[MarkM, 2016-03-08 03:55:11]

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/atLea...
File src/com/google/caja/ses/atLeastFreeVarNames.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/atLea...
src/com/google/caja/ses/atLeastFreeVarNames.js:176: // JSON.parse will reject
with an error.
On 2016/03/07 22:18:48, kpreid_google wrote:
> This sentence is too merely descriptive. State that this limitation is for
> simplicity rather than security.

Done.

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/compi...
File src/com/google/caja/ses/compileExprLater.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/compi...
src/com/google/caja/ses/compileExprLater.js:106: var quotedNames =
prep.freeNames.map(JSON.stringify);
On 2016/03/07 22:18:48, kpreid_google wrote:
> Why not JSON.stringify(prep.freeNames) instead?

Done. (I don't know how I missed that, thanks)

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:571: ses.resampleGlobal = resampleGlobal;
On 2016/03/07 22:18:48, kpreid_google wrote:
> On 2016/03/05 06:04:03, MarkM wrote:
> > Kevin, I looked at guest-manager.js but was not able to figure out
> > where it should call ses.resampleGlobal(). Please advise.
> 
> Looked; it actually should go in ses-frame-group.js, function sesRun. I'd
> suggest between the 'imports' section and the 'promise' section.
> 
> > I should mention that the first sampling is delayed until first needed,
> > which perhaps already accomplishes what this call from run() was
> > supposed to accomplish.
> 
> Perhaps, but I'm feeling cautious, and this does little harm.
> 

Done.

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:581: * makeScopeOpject protections fail.
On 2016/03/07 22:18:48, kpreid_google wrote:
> typo: "Opject"

Done.

https://codereview.appspot.com/283510043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:936: // atLeastFreeVarNames is likely to be
more informative.
On 2016/03/07 22:18:48, kpreid_google wrote:
> "…more informative, so it is called first."

Done.

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

https://codereview.appspot.com/283510043/diff/1/tests/com/google/caja/ses/tes...
tests/com/google/caja/ses/test-ses-parts.js:84:
jsunitRegister('testAtLeastFreeVarNamesOutput', function() {
On 2016/03/07 22:18:48, kpreid_google wrote:
> We want a test which checks that \u{...} escapes are rejected. (In the event
> that they are later supported, we can add examples of them to the two existing
> tests instead.) Here's the test skeleton; I leave it to you to invent test
data:
> 
> jsunitRegister('testAtLeastFreeVarNamesOnNewUnicodeEscapes', function() {
>   var names = [...];
>   names.forEach(function(name) {
>     try {
>       ses.atLeastFreeVarNames(name);
>     } catch (e) {
>       assertTrue(e instanceof SyntaxError);
>       return;
>     }
>     fail('Unexpectedly succeeded to parse ' + name);
>   });
>   jsunitPass();
> });

Done.

https://codereview.appspot.com/283510043/diff/20001/tests/com/google/caja/ses...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/283510043/diff/20001/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:116: // atLeastFreeVarNames
recognizes that some non-identifiers
Note that I has to add this case too, since I had enhanced atLeastFreeVarNames
to skip names that began with a digit.

[MarkM, 2016-03-08 12:02:21]

ES6 extended the syntax of identifiers to include the unicode escape
sequence backslash-u-opencurly-hexdigits-closecurly. This meant that
untrusted code could name global variables that we would not notice as
a single token, and thus would not censor.

Since this regexp-based recognition of possible variable name mentions
seems fragile, we added an additional backstop mechanism: we sample
the set of global variable names and build a backstop object with a
poisoned accessor for each of these names. Each actual scopeObject
then inherits from this accessor. Thus, if atLeastFreeVarNames missed
a name, but that was the name of a global variable when the backstop
was built, then the poisoned accessor on the backstop will intercept
it instead.

We add a new ses.resampleGlobal() to the privileged api, so our client
can advise when they've added additional global variables, so we can
resample.

This change builds on https://codereview.appspot.com/285330043/ ,
which should be considered our diffbase.

[kpreid_google, 2016-03-08 17:33:18]

LGTM

https://codereview.appspot.com/283510043/diff/40001/tests/com/google/caja/ses...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/283510043/diff/40001/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:183:
jsunitRegister('testAtLeastFreeVarNamesOnNewUnicodeEscapes', function() {
I think this deserves a comment that this test isn't because we have a reason to
reject them, but only to ensure we aren't omitting them from the list.

[MarkM, 2016-03-08 18:18:19]

ES6 extended the syntax of identifiers to include the unicode escape
sequence backslash-u-opencurly-hexdigits-closecurly. This meant that
untrusted code could name global variables that we would not notice as
a single token, and thus would not censor.

Since this regexp-based recognition of possible variable name mentions
seems fragile, we added an additional backstop mechanism: we sample
the set of global variable names and build a backstop object with a
poisoned accessor for each of these names. Each actual scopeObject
then inherits from this accessor. Thus, if atLeastFreeVarNames missed
a name, but that was the name of a global variable when the backstop
was built, then the poisoned accessor on the backstop will intercept
it instead.

We add a new ses.resampleGlobal() to the privileged api, so our client
can advise when they've added additional global variables, so we can
resample.

This change builds on https://codereview.appspot.com/285330043/ ,
which should be considered our diffbase.

[MarkM, 2016-03-08 18:20:15]

Hi Kevin, all done. Since this is undisclosed, am I done on my end? Are you
taking it from here?

https://codereview.appspot.com/283510043/diff/40001/tests/com/google/caja/ses...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/283510043/diff/40001/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:183:
jsunitRegister('testAtLeastFreeVarNamesOnNewUnicodeEscapes', function() {
On 2016/03/08 17:33:18, kpreid_google wrote:
> I think this deserves a comment that this test isn't because we have a reason
to
> reject them, but only to ensure we aren't omitting them from the list.

Done.

[kpreid_google, 2016-03-08 18:20:51]

On 2016/03/08 18:20:15, MarkM wrote:
> Hi Kevin, all done. Since this is undisclosed, am I done on my end? Are you
> taking it from here?

Yes.