Add mitigation options for makeScopeObject

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSrcGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSrcOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. Added a rewriteFunctionCalls which remains to be
implemented
https://code.google.com/p/google-caja/issues/detail?id=1755

[MarkM, 2013-06-01 22:12:29]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[MarkM, 2013-06-01 22:13:56]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[MarkM, 2013-06-01 22:30:38]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[MarkM, 2013-06-01 22:30:51]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[MarkM, 2013-06-01 22:49:51]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[MarkM, 2013-06-01 23:07:00]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[kpreid2, 2013-06-03 18:33:30]

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:319: * program with the same semantics as
the original but with some of
Uninformed reader asks "Some? Why not all?"

Please explain the category of gotchas which are handled here and refer to where
the other ones are handled. I suspect (not having finished review) that the
answer is that this mitigates gotchas via source-code transformations whereas
the rest are by scope-object modifications. If so, please say so.

Also, 'mitigateSomeGotchas' is an underinformative name. Perhaps
'mitigateSrcGotchas'?

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:647: if (options.wrapImportedFunction &&
This seems like a bad idea to me, for two reasons:

1) the 'imported function' will be unexpectedly !== to 'itself', as well as
possibly having accessor properties misbehave. (That is, this code is like an
incoherent membrane.)

2) This is putting additional code, which allocates new objects, in a very hot
path. (Now that I think about it, this path (using an accessor to alias rather
than copying the property) may be hotter than it needs to be; I'll look into
that separately.)

Is there a reason why we do not perform this mitigation by source-to-source
transform of 'foo()', when foo is not shadowed or perhaps even if it is, into
'(0,foo)()'? This may not help the jQuery case but it will the normal
'untrusted-code-in-the-wild' case.

If we must do this, then please minimize unnecessary work by moving the
definition of wrappedResult into a function called inside the conditional and
turn the lookups on 'options' into lexical variables.

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:668: //     wrappedResult rather than the
original.
This doesn't seem to be a desirable thing?

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:681: // without parsing or proxies, that
isn't possible.
Please add a note that mitigateGotchas' rewrite takes care of that under normal
circumstances.

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:810: *     cases.
Explicitly state that this is a less-correct-but-faster alternative to
rewriteTypeOf.

[MarkM, 2013-06-04 17:04:57]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSomeGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSomeOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. wrapImportedFunction addresses a portion of 
https://code.google.com/p/google-caja/issues/detail?id=1752

[MarkM, 2013-06-04 17:07:36]

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:319: * program with the same semantics as
the original but with some of
On 2013/06/03 18:33:30, kpreid2 wrote:
> Uninformed reader asks "Some? Why not all?"
> 
> Please explain the category of gotchas which are handled here and refer to
where
> the other ones are handled. I suspect (not having finished review) that the
> answer is that this mitigates gotchas via source-code transformations whereas
> the rest are by scope-object modifications. If so, please say so.
> 
> Also, 'mitigateSomeGotchas' is an underinformative name. Perhaps
> 'mitigateSrcGotchas'?

Done.

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:647: if (options.wrapImportedFunction &&
On 2013/06/03 18:33:30, kpreid2 wrote:
> This seems like a bad idea to me, for two reasons:
> 
> 1) the 'imported function' will be unexpectedly !== to 'itself', as well as
> possibly having accessor properties misbehave. (That is, this code is like an
> incoherent membrane.)
> 
> 2) This is putting additional code, which allocates new objects, in a very hot
> path. (Now that I think about it, this path (using an accessor to alias rather
> than copying the property) may be hotter than it needs to be; I'll look into
> that separately.)
> 
> Is there a reason why we do not perform this mitigation by source-to-source
> transform of 'foo()', when foo is not shadowed or perhaps even if it is, into
> '(0,foo)()'? This may not help the jQuery case but it will the normal
> 'untrusted-code-in-the-wild' case.
> 
> If we must do this, then please minimize unnecessary work by moving the
> definition of wrappedResult into a function called inside the conditional and
> turn the lookups on 'options' into lexical variables.

Done. Added a "rewriteFunctionCalls" instead and noted, with a TODO(jasvir),
that this is currently unimplemented.

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:668: //     wrappedResult rather than the
original.
On 2013/06/03 18:33:30, kpreid2 wrote:
> This doesn't seem to be a desirable thing?

No. Deleted. Done.

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:681: // without parsing or proxies, that
isn't possible.
On 2013/06/03 18:33:30, kpreid2 wrote:
> Please add a note that mitigateGotchas' rewrite takes care of that under
normal
> circumstances.

Done.

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:810: *     cases.
On 2013/06/03 18:33:30, kpreid2 wrote:
> Explicitly state that this is a less-correct-but-faster alternative to
> rewriteTypeOf.

Done.

[kpreid2, 2013-06-04 17:26:46]

LGTM

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/9945043/diff/18001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:319: * program with the same semantics as
the original but with some of
On 2013/06/04 17:07:36, MarkM wrote:
> On 2013/06/03 18:33:30, kpreid2 wrote:
> > Also, 'mitigateSomeGotchas' is an underinformative name. Perhaps
> > 'mitigateSrcGotchas'?
> 
> Done.

Update change description to match, in .appspot-change and on rietveld.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/ses-frame-group.js (right):

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/ses-frame-group.js:35: // TODO(kpreid): make sure
mitigator is applied to guest code only
You may delete this TODO: it refers to mitigating Domado-and-friends, which we
know doesn't apply.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/com...
File src/com/google/caja/ses/compileExprLater.js (right):

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/com...
src/com/google/caja/ses/compileExprLater.js:111: JSON.stringify(options) +
')));';
this entire statement should be 4-space indent

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:314: ses.resolveOptions = resolveOptions;
I am concerned that 'ses' is becoming a mixed bundle of internal components and
public interface.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:638: var result;
unused

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:639: function wrappedResult(var_args) {
unused

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:803: *     unimplemented.
Please file an issue for implementing this.

[MarkM, 2013-06-04 17:47:34]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSrcGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSrcOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. Added a rewriteFunctionCalls which remains to be
implemented
https://code.google.com/p/google-caja/issues/detail?id=1755

[MarkM, 2013-06-04 17:48:42]

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/ses-frame-group.js (right):

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/ses-frame-group.js:35: // TODO(kpreid): make sure
mitigator is applied to guest code only
On 2013/06/04 17:26:46, kpreid2 wrote:
> You may delete this TODO: it refers to mitigating Domado-and-friends, which we
> know doesn't apply.

Done.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/com...
File src/com/google/caja/ses/compileExprLater.js (right):

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/com...
src/com/google/caja/ses/compileExprLater.js:111: JSON.stringify(options) +
')));';
On 2013/06/04 17:26:46, kpreid2 wrote:
> this entire statement should be 4-space indent

Done.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:314: ses.resolveOptions = resolveOptions;
On 2013/06/04 17:26:46, kpreid2 wrote:
> I am concerned that 'ses' is becoming a mixed bundle of internal components
and
> public interface.

Acknowledged. I didn't do this at first but then found I needed to access it
from compileExprLater.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:638: var result;
On 2013/06/04 17:26:46, kpreid2 wrote:
> unused

Done.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:639: function wrappedResult(var_args) {
On 2013/06/04 17:26:46, kpreid2 wrote:
> unused

Done.

https://codereview.appspot.com/9945043/diff/22001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:803: *     unimplemented.
On 2013/06/04 17:26:46, kpreid2 wrote:
> Please file an issue for implementing this.

Done.

[kpreid2, 2013-06-04 17:52:22]

LGTM

[Mark S. Miller, 2013-06-04 17:54:52]

Could someone with a working system please check this with "ant runtests"?
Thanks.


On Tue, Jun 4, 2013 at 10:52 AM, <kpreid.switchb.org@gmail.com> wrote:

> LGTM
>
>
>
https://codereview.appspot.**com/9945043/<https://codereview.appspot.com/9945...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "Google Caja Discuss" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
google-caja-discuss+**unsubscribe@googlegroups.com<google-caja-discuss%2Bunsu...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>


-- 
    Cheers,
    --MarkM

[kpreid2, 2013-06-04 18:12:02]

On 2013/06/04 17:54:52, Mark S. Miller wrote:
> Could someone with a working system please check this with "ant runtests"?

You have one failure: 

testReferenceError: [JsUnitException Call to fail() : Expected failure

in
http://localhost:8000/ant-testlib/com/google/caja/plugin/browser-test-case.ht...

[ihab.awad, 2013-06-05 21:25:09]

Please patch in the following to get the tests to pass again --

--- tests/com/google/caja/plugin/es53-test-language-guest.html	(revision 5439)
+++ tests/com/google/caja/plugin/es53-test-language-guest.html	(working copy)
@@ -691,12 +691,16 @@
 <script>
   jsunitRegister('testReferenceError',
                  function testReferenceError() {
-    expectFailure(function() {
-      testReferenceError_thisIsUndefined;
-    }, null, function(e) {
-      return e instanceof ReferenceError && e.message ===
-          '"testReferenceError_thisIsUndefined" is not defined in this scope.';
-    });
+    if (inES5Mode) {
+      assertEquals(void 0, testReferenceError_thisIsUndefined);
+    } else {
+      expectFailure(function() {
+        testReferenceError_thisIsUndefined;
+      }, null, function(e) {
+        return e instanceof ReferenceError && e.message ===
+            '"testReferenceError_thisIsUndefined" is not defined in this
scope.';
+      });
+    }
     pass('testReferenceError');
   });
 </script>

[MarkM, 2013-06-06 03:33:42]

Adds two new mitigstion options: maskReferenceError and
wrapImportedFunction. See the doc-comment in compileExpr for their
purpose. 

Split mitigateGotchas into mitigateSrcGotchas and resolveOptions,
where resolveOptions is moved to startSES and mitigateSrcOptions
assumes its options are already resolved. That way, the same resolved
options object can also be passed to makeScopeObject for the
mitigations it performs. These latter mitigations happen even in a
minimal SES environment without mitigateGotchas.js

maskReferenceError should enable the new jquery to run without
rewriting. Added a rewriteFunctionCalls which remains to be
implemented
https://code.google.com/p/google-caja/issues/detail?id=1755