Detects a FF35 bug that allows non-extensible objects to be changed.

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-15 18:07:31]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-15 21:25:05]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[kpreid2, 2015-02-15 22:26:23]

https://codereview.appspot.com/202040043/diff/40001/src/com/google/caja/ses/r...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/202040043/diff/40001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:3216: if (e instanceof ReferenceError && i
=== 2000) {
It is peculiar that we are deliberately triggering a ReferenceError (X is not
defined); absent specific details I'd expect to do "A.length1 = 1;" and expect a
TypeError. Please add a comment explaining that this is a necessary part (if it
is).

Also, use a higher-entropy variable name than X, because if X _is_ defined prior
to SES being loaded, this code will see an "unexpected error" even if the
problem is not present.

[MarkM, 2015-02-15 23:46:11]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-15 23:47:27]

https://codereview.appspot.com/202040043/diff/40001/src/com/google/caja/ses/r...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/202040043/diff/40001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:3216: if (e instanceof ReferenceError && i
=== 2000) {
On 2015/02/15 22:26:23, kpreid2 wrote:
> It is peculiar that we are deliberately triggering a ReferenceError (X is not
> defined); absent specific details I'd expect to do "A.length1 = 1;" and expect
a
> TypeError. Please add a comment explaining that this is a necessary part (if
it
> is).
> 
> Also, use a higher-entropy variable name than X, because if X _is_ defined
prior
> to SES being loaded, this code will see an "unexpected error" even if the
> problem is not present.

Done.

[kpreid2, 2015-02-16 00:11:29]

LGTM

[kpreid2, 2015-02-16 00:12:32]

On 2015/02/16 00:11:29, kpreid2 wrote:
> LGTM

P.S. I still think the test case should be better commented but that can wait
until we know what the actual bug is.

[MarkM, 2015-02-16 00:38:40]

On 2015/02/16 00:11:29, kpreid2 wrote:
> LGTM

Now that this is approved but cannot yet be submitted, is there anything else to
do but wait?

[MarkM, 2015-02-16 00:42:05]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-16 00:55:10]

From the latest info from Mozilla, we now think a repair is possible, so this CL
no longer looks good until either a repair so it works on FF35, or I give up on
that plan.

[MarkM, 2015-02-16 05:55:33]

When doing an ant runtests, in addition to the normal coverage gaps we expect
here, I am getting two new test failures:

From the log:

ses-repair-framework-min	Failure	The title shows 1/7 fail 6/7 pass - Browser
test case (Netscape 5.0 (Macintosh))

junit.framework.AssertionFailedError: The title shows 1/7 fail 6/7 pass -
Browser test case (Netscape 5.0 (Macintosh))
	at
com.google.caja.plugin.BrowserTestCase.driveBrowser(BrowserTestCase.java:213)
	at
com.google.caja.plugin.BrowserTestCase.runBrowserTest(BrowserTestCase.java:130)
	at com.google.caja.plugin.CatalogTestCase.runTest(CatalogTestCase.java:34)
	at com.google.caja.plugin.CatalogRunner$1.evaluate(CatalogRunner.java:94)
	at com.google.caja.plugin.CatalogRunner.runChild(CatalogRunner.java:88)
	at com.google.caja.plugin.CatalogRunner.runChild(CatalogRunner.java:42)

and 


cajajs-success-min	Failure	The title shows 1/1 fail 0/1 pass - Browser test case
(Netscape 5.0 (Macintosh))

junit.framework.AssertionFailedError: The title shows 1/1 fail 0/1 pass -
Browser test case (Netscape 5.0 (Macintosh))
	at
com.google.caja.plugin.BrowserTestCase.driveBrowser(BrowserTestCase.java:213)
	at
com.google.caja.plugin.BrowserTestCase.runBrowserTest(BrowserTestCase.java:130)
	at com.google.caja.plugin.CatalogTestCase.runTest(CatalogTestCase.java:34)
	at com.google.caja.plugin.CatalogRunner$1.evaluate(CatalogRunner.java:94)
	at com.google.caja.plugin.CatalogRunner.runChild(CatalogRunner.java:88)
	at com.google.caja.plugin.CatalogRunner.runChild(CatalogRunner.java:42)


From the unclosed web pages:

1/7 fail
testRepairOutcomes — failed testRepairOutcomes


and


1/1 fail
testSuccess — failed testSuccess


I would like to get together sometime so we can pair program and you can show me
how you deal with these test failures. Thanks.

[kpreid2, 2015-02-16 15:50:39]

On 2015/02/16 05:55:33, MarkM wrote:
> When doing an ant runtests, in addition to the normal coverage gaps we expect
> here, I am getting two new test failures:
> …
> (test-repair-framework) testRepairOutcomes

This is probably because you renamed ACCIDENTALLY_REPAIRED and the test just
needs to be updated to match.

> (test-cajajs-success) testSuccess — failed testSuccess

Since this is also about 'did initSES do what we expect', it might be for the
same reason. We'll look at it.

> I would like to get together sometime so we can pair program and you can show
me
> how you deal with these test failures. Thanks.

Check your calendar.

[MarkM, 2015-02-16 17:08:44]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-16 17:20:03]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-16 17:23:48]

On 2015/02/16 15:50:39, kpreid2 wrote:
> On 2015/02/16 05:55:33, MarkM wrote:
> > When doing an ant runtests, in addition to the normal coverage gaps we
expect
> > here, I am getting two new test failures:
> > …
> > (test-repair-framework) testRepairOutcomes
> 
> This is probably because you renamed ACCIDENTALLY_REPAIRED and the test just
> needs to be updated to match.

New snapshot. Does fix this one.



> 
> > (test-cajajs-success) testSuccess — failed testSuccess
> 
> Since this is also about 'did initSES do what we expect', it might be for the
> same reason. We'll look at it.

Did not fix this one.

[MarkM, 2015-02-17 02:44:53]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-17 02:47:01]

On 2015/02/16 00:55:10, MarkM wrote:
> From the latest info from Mozilla, we now think a repair is possible, so this
CL
> no longer looks good until either a repair so it works on FF35, or I give up
on
> that plan.

New snapshot with repair as suggested by Mozilla. PTAL.

[MarkM, 2015-02-17 02:53:57]

On 2015/02/17 02:47:01, MarkM wrote:
> On 2015/02/16 00:55:10, MarkM wrote:
> > From the latest info from Mozilla, we now think a repair is possible, so
this
> CL
> > no longer looks good until either a repair so it works on FF35, or I give up
> on
> > that plan.
> 
> New snapshot with repair as suggested by Mozilla. PTAL.

"ant runtests" now once again only gives the expected error.

Perhaps "(test-cajajs-success) testSuccess — failed testSuccess" was because,
without this repair, SES must indeed reject FF35 as NOT ISOLATED ?

[kpreid2, 2015-02-17 19:23:07]

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:31: * require its absence, but the linter
doesn't know that.
Given that you've pointed out precedent for using globals like this, please name
NonexistentGlobal using the same style as the existing ones (but see below
comment about trying an existent global first).

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3217: A.length1 = NonexistentGlobal;
Will this also provoke the bug if we use an actually existent global? That would
be more robust.

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3912: defProp(Object,
'getOwnPropertyNames', {
All property descriptors should be configurable:true so that further repairs can
be applied.

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3914: return
gopn(obj).filter(isNotBogusName);
Worth noting that this is not sound in a writable-globals world (i.e. repairES5
without SES) because you're using the filter method.

I hear this same problem applies to WeakMap.js.

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3926: function makeBogus(obj) {
Unhelpful function name, sounds constructorish. How about "addDummyProperty"?

'Dummy' instead of 'bogus' because 'dummy' has the connotation of "we have no
real [this thing]s so here's a placeholder one".

[MarkM, 2015-02-17 22:27:14]

Vulnerability reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1125389#c6 which you may
not be able to see.
See https://code.google.com/p/google-caja/issues/detail?id=1954

This bug is non-disclosed, causes loss of isolation, and has no known
workaround. Thus, this CL, once submitted, will prevent Caja and SES
from running on FF35, which is the current release. If this bug
becomes public or we are aware that it is being exploited, we should
release immediately anyway. Otherwise, I suggest we wait until FF36 is
released, which is currently expected on 2/23/2015.

At that point Google still considers FF35 a supported browser, until
FF37 is released. So submitting this CL after the FF36 release will
cause Caja and SES not to run on a Google supported browser.

[MarkM, 2015-02-17 22:29:15]

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:31: * require its absence, but the linter
doesn't know that.
On 2015/02/17 19:23:07, kpreid2 wrote:
> Given that you've pointed out precedent for using globals like this, please
name
> NonexistentGlobal using the same style as the existing ones (but see below
> comment about trying an existent global first).

That worked. See reply below.

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3217: A.length1 = NonexistentGlobal;
On 2015/02/17 19:23:07, kpreid2 wrote:
> Will this also provoke the bug if we use an actually existent global? That
would
> be more robust.

Good intuition and good news:

Not only does an existent global still provoke the bug, so does an existent
local, making the testing logic much more self contained. Thanks!

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3912: defProp(Object,
'getOwnPropertyNames', {
On 2015/02/17 19:23:07, kpreid2 wrote:
> All property descriptors should be configurable:true so that further repairs
can
> be applied.

Added comment both here and in WeakMap.js explaining that an omitted attribute
preserves its current setting.

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3914: return
gopn(obj).filter(isNotBogusName);
On 2015/02/17 19:23:07, kpreid2 wrote:
> Worth noting that this is not sound in a writable-globals world (i.e.
repairES5
> without SES) because you're using the filter method.
> 
> I hear this same problem applies to WeakMap.js.

Done.

https://codereview.appspot.com/202040043/diff/120001/src/com/google/caja/ses/...
src/com/google/caja/ses/repairES5.js:3926: function makeBogus(obj) {
On 2015/02/17 19:23:07, kpreid2 wrote:
> Unhelpful function name, sounds constructorish. How about "addDummyProperty"?
> 
> 'Dummy' instead of 'bogus' because 'dummy' has the connotation of "we have no
> real [this thing]s so here's a placeholder one".

Done.

[kpreid2, 2015-02-17 23:59:22]

LGTM