Revision: 5485
Author: erights
Date: Thu Jul 11 18:45:27 2013
Log: Added repair for Function constructor bug
https://codereview.appspot.com/10181043
Repairs verification errors when Function constructor fails to verify body
[email protected]
http://code.google.com/p/google-caja/source/detail?r=5485
Modified:
/trunk/src/com/google/caja/plugin/caja.js
/trunk/src/com/google/caja/ses/exportsToSES.js
/trunk/src/com/google/caja/ses/mitigateGotchas.js
/trunk/src/com/google/caja/ses/repairES5.js
/trunk/src/com/google/caja/ses/startSES.js
=======================================
--- /trunk/src/com/google/caja/plugin/caja.js Fri Jun 7 14:10:14 2013
+++ /trunk/src/com/google/caja/plugin/caja.js Thu Jul 11 18:45:27 2013
@@ -452,9 +452,15 @@
'ARRAYS_MODIFY_READONLY': { 'permit': true },
// safe given that we use exactly one SES frame
- 'FREEZE_IS_FRAME_DEPENDENT': { 'permit': true }
+ 'FREEZE_IS_FRAME_DEPENDENT': { 'permit': true },
+ 'SYNTAX_ERRORS_ARENT_ALWAYS_EARLY': { 'permit': true }
};
}
+ ses['mitigateSrcGotchas'] = function() {
+ throw new EvalError('This function is a placeholder that should ' +
+ 'have been replaced by the real ' +
+ 'ses.mitigateSrcGotchas.');
+ };
}
var sesMaker = makeFrameMaker(config, 'ses-single-frame', frameInit);
=======================================
--- /trunk/src/com/google/caja/ses/exportsToSES.js Thu Feb 7 17:55:24 2013
+++ /trunk/src/com/google/caja/ses/exportsToSES.js Thu Jul 11 18:45:27 2013
@@ -24,7 +24,10 @@
* \@overrides exports
*/
+var ses;
+
(function(global) {
+ 'use strict';
global.ses = global.ses || {};
ses.rewriter_ = {};
@@ -32,6 +35,4 @@
ses.rewriter_.parse = exports.parse;
ses.rewriter_.generate = exports.generate;
- // No longer need exports
- delete global.exports;
})(this);
=======================================
--- /trunk/src/com/google/caja/ses/mitigateGotchas.js Tue Jul 2 11:57:39
2013
+++ /trunk/src/com/google/caja/ses/mitigateGotchas.js Thu Jul 11 18:45:27
2013
@@ -203,6 +203,7 @@
function needsRewriting(options) {
return options.rewriteTopLevelVars ||
options.rewriteTopLevelFuncs ||
+ options.rewriteFunctionCalls ||
options.rewriteTypeOf;
}
@@ -210,15 +211,15 @@
* Assumes {@code options} have already been safely canonicalized by
* startSES's {@code resolveOptions}.
*/
- ses.mitigateSrcGotchas = function(programSrc, options, logger) {
- if (!options.parseProgram) {
- return programSrc;
+ ses.mitigateSrcGotchas = function(funcBodySrc, options, logger) {
+ if (!needsRewriting(options) && !options.parseFunctionBody) {
+ return funcBodySrc;
}
try {
var dirty = false;
var path = [];
var scopeLevel = 0;
- var ast = ses.rewriter_.parse(programSrc);
+ var ast = ses.rewriter_.parse(funcBodySrc);
if (needsRewriting(options)) {
ses.rewriter_.traverse(ast, {
enter: function enter(node) {
@@ -265,7 +266,7 @@
+ " */\n"
+ ses.rewriter_.generate(ast);
} else {
- return programSrc;
+ return funcBodySrc;
}
} catch (e) {
logger.warn('Failed to parse program', e);
=======================================
--- /trunk/src/com/google/caja/ses/repairES5.js Thu Jul 11 16:56:33 2013
+++ /trunk/src/com/google/caja/ses/repairES5.js Thu Jul 11 18:45:27 2013
@@ -27,11 +27,13 @@
* create it, use it, and delete it all within this module. But we
* need to lie to the linter since it can't tell.
*
+ * //requires ses.mitigateSrcGotchas
+ * //requires ses.acceptableProblems, ses.maxAcceptableSeverityName
* //provides ses.statuses, ses.ok, ses.is, ses.makeDelayedTamperProof
* //provides ses.makeCallerHarmless, ses.makeArgumentsHarmless
+ * //provides ses.verifyStrictFunctionBody
* //provides ses.severities, ses.maxSeverity, ses.updateMaxSeverity
- * //provides ses.maxAcceptableSeverityName, ses.maxAcceptableSeverity
- * //provides ses.acceptableProblems
+ * //provides ses.maxAcceptableSeverity
*
* @author Mark S. Miller
* @requires ___global_test_function___, ___global_valueOf_function___
@@ -297,7 +299,7 @@
* verification-only strategy and fall back to ES5/3 translation.
*/
ses.ok = function ok(maxSeverity) {
- if ("string" === typeof maxSeverity) {
+ if ('string' === typeof maxSeverity) {
maxSeverity = ses.severities[maxSeverity];
}
if (!maxSeverity) {
@@ -636,7 +638,7 @@
var makeDelayedTamperProofCalled = false;
ses.makeDelayedTamperProof = function makeDelayedTamperProof() {
if (makeDelayedTamperProofCalled) {
- throw "makeDelayedTamperProof() must only be called once.";
+ throw 'makeDelayedTamperProof() must only be called once.';
}
var tamperProof = makeTamperProof();
strictForEachFn(needToTamperProof, tamperProof);
@@ -644,6 +646,159 @@
makeDelayedTamperProofCalled = true;
return tamperProof;
};
+
+ /**
+ * Fails if {@code funcBodySrc} does not parse as a strict
+ * FunctionBody.
+ *
+ * <p>ses.verifyStrictFunctionBody is exported from repairES5
+ * because the best way to perform this verification on a given
+ * platform depends on whether the platform's Function constructor
+ * <a href=
+ * "https://code.google.com/p/google-caja/issues/detail?id=1616";
+ * >fails to verify that its body parses as a FunctionBody</a>. If
+ * it does, repairES5 could have repaired the Function constructor
+ * itself, but chooses not to, since its main client, startSES, has
+ * to wrap and replace the Function constructor anyway.
+ *
+ * <p>On platforms not suffering from this bug,
+ * ses.verifyStrictFunctionBody just calls the original Function
+ * constructor to do this verification (See
+ * simpleVerifyStrictFunctionBody). Otherwise, we repair
+ * ses.verifyStrictFunctionBody
+ *
+ * <p>See verifyStrictFunctionBodyByEvalThrowing and
+ * verifyStrictFunctionBodyByParsing.
+ */
+ ses.verifyStrictFunctionBody = simpleVerifyStrictFunctionBody;
+
+ /**
+ * The unsafe* variables hold precious values that must not escape
+ * to untrusted code. When {@code eval} is invoked via {@code
+ * unsafeEval}, this is a call to the indirect eval function, not
+ * the direct eval operator.
+ */
+ var unsafeEval = eval;
+ var UnsafeFunction = Function;
+
+ /**
+ * <p>We use Crock's trick of simply passing {@code funcBodySrc} to
+ * the original {@code Function} constructor, which will throw a
+ * SyntaxError if it does not parse as a FunctionBody.
+ */
+ function simpleVerifyStrictFunctionBody(funcBodySrc) {
+ UnsafeFunction('"use strict";' + funcBodySrc);
+ }
+
+ /**
+ * If Crock's trick is not safe, then
+ * repair_CANT_SAFELY_VERIFY_SYNTAX may replace it with Ankur's trick,
+ * depending on whether the platform also suffers from bugs that
+ * would block it. See repair_CANT_SAFELY_VERIFY_SYNTAX for details.
+ *
+ * <p>To use Ankur's trick to check a FunctionBody rather than a
+ * program, we use the trick in comment 7 at
+ * https://code.google.com/p/google-caja/issues/detail?id=1616#c7
+ * The criticism of it in comment 8 is blocked by Ankur's trick,
+ * given the absence of the other bugs that
+ * repair_CANT_SAFELY_VERIFY_SYNTAX checks in order to decide.
+ *
+ * <p>Testing once revealed that Crock's trick
+ * (simpleVerifyStrictFunctionBody) executed over 100x faster on V8.
+ */
+ function verifyStrictFunctionBodyByEvalThrowing(funcBodySrc) {
+ try {
+ unsafeEval('"use strict"; throw "not a SyntaxError 1";' +
+ '(function(){' + funcBodySrc +'\n});');
+ } catch (outerErr) {
+ if (outerErr === 'not a SyntaxError 1') {
+ try {
+ unsafeEval('throw "not a SyntaxError 2";' +
+ '(function(){{' + funcBodySrc +'\n}})');
+ } catch (innerErr) {
+ if (innerErr === 'not a SyntaxError 2') {
+ // Presumably, if we got here, funcBodySrc parsed as a strict
+ // function body but was not executed, and {funcBodySrc}
+ // parsed as a non-strict function body but was not executed.
+ // We try it again non-strict so that body level nested
+ // function declarations will not get rejected. Accepting
+ // them is beyond the ES5 spec, but is known to happen in
+ // all implementations.
+ return;
+ }
+ if (innerErr instanceof SyntaxError) {
+ // This case is likely symptomatic of an attack. But the
+ // attack is thwarted and so need not be reported as
+ // anything other than the SyntaxError it is.
+ throw innerErr;
+ }
+ }
+ }
+ if (outerErr instanceof SyntaxError) {
+ throw outerErr;
+ }
+ }
+ throw new TypeError('Unexpected verification outcome');
+ }
+
+ /**
+ * Due to https://code.google.com/p/v8/issues/detail?id=2728
+ * we can't assume that SyntaxErrors are always early. If they're
+ * not, then even Ankur's trick doesn't work, so we resort to a full
+ * parse.
+ *
+ * <p>Only applicable if ses.mitigateSrcGotchas is available. To
+ * accommodate constraints of Caja's initialization order, we do not
+ * capture or invoke ses.mitigateSrcGotchas as the time repairES5 is
+ * run. Rather we only test for its presence at repair time in order
+ * to decide what verifier to use. We only use ses.mitigateSrcGotchas
+ * later when we actually verify eval'ed code, and at that time we
+ * use the current binding of ses.mitigateSrcGotchas.
+ *
+ * <p>Thus, clients (like Caja) that know they will be making a
+ * real ses.mitigateSrcGotchas available after repair can
+ * pre-install a placeholder function that, if accidentally invoked,
+ * throws an error to complain that it was not replaced by the real
+ * one. Then, sometime prior to the first verification, the client
+ * should overwrite ses.mitigateSrcGotchas with the real one.
+ */
+ function verifyStrictFunctionBodyByParsing(funcBodySrc) {
+ var safeError;
+ var newSrc;
+ try {
+ newSrc = ses.mitigateSrcGotchas(funcBodySrc,
+ {parseFunctionBody: true},
+ ses.logger);
+ } catch (error) {
+ // Shouldn't throw, but if it does, the exception is potentially
+ // from a different context with an undefended prototype chain;
+ // don't allow it to leak out.
+ try {
+ safeError = new Error(error.message);
+ } catch (metaerror) {
+ throw new Error(
+ 'Could not safely obtain error from mitigateSrcGotchas');
+ }
+ throw safeError;
+ }
+
+ // The following equality test is due to the peculiar API of
+ // ses.mitigateSrcGotchas, which (TODO(jasvir)) should probably be
+ // fixed instead. However, currently, since we're asking it only to
+ // parse and not to rewrite, if the parse is successful it will
+ // return its argument src string, which is fine.
+ //
+ // If the parse is not successful, ses.mitigateSrcGotchas
+ // indicates the problem <i>only</i> by returning a string which,
+ // if evaluated, would throw a SyntaxError with a non-informative
+ // message. Since, in this case, these are the only possibilities,
+ // we happen to be able to check for the error case by seeing
+ // simply that the string returned is not the src string passed
+ // in.
+ if (newSrc !== funcBodySrc) {
+ throw new SyntaxError('Failed to parse program');
+ }
+ }
/**
* Where the "that" parameter represents a "this" that should have
@@ -1110,9 +1265,8 @@
* should not be boxed
*/
function test_FOREACH_COERCES_THISOBJ() {
- "use strict";
var needsWrapping = true;
- [1].forEach(function(){ needsWrapping = ("string" != typeof this);
}, "f");
+ [1].forEach(function(){ needsWrapping = ('string' != typeof this);
}, 'f');
return needsWrapping;
}
@@ -1155,7 +1309,7 @@
// likely not a browser environment
return false;
}
- var f = document.createElement("form");
+ var f = document.createElement('form');
try {
Object.defineProperty(f, 'foo', {
get: getter,
@@ -1711,24 +1865,43 @@
if (x.__proto__ === Object.prototype) {
return true;
}
- return "Read-only proto was changed in a strange way.";
+ return 'Read-only proto was changed in a strange way.';
}
/**
* Detects http://code.google.com/p/v8/issues/detail?id=1624
+ * regarding variables.
*
* <p>Both a direct strict eval operator and an indirect strict eval
* function must not leak top level declarations in the string being
* evaluated into their containing context.
*/
- function test_STRICT_EVAL_LEAKS_GLOBALS() {
- (1,eval)('"use strict"; var ___global_test_variable___ = 88;');
+ function test_STRICT_EVAL_LEAKS_GLOBAL_VARS() {
+ unsafeEval('"use strict"; var ___global_test_variable___ = 88;');
if ('___global_test_variable___' in global) {
delete global.___global_test_variable___;
return true;
}
return false;
}
+
+ /**
+ * Detects http://code.google.com/p/v8/issues/detail?id=1624
+ * regarding functions
+ *
+ * <p>Both a direct strict eval operator and an indirect strict eval
+ * function must not leak top level declarations in the string being
+ * evaluated into their containing context.
+ */
+ function test_STRICT_EVAL_LEAKS_GLOBAL_FUNCS() {
+ unsafeEval('"use strict"; function ___global_test_func___(){}');
+ if ('___global_test_func___' in global) {
+ delete global.___global_test_func___;
+ return true;
+ }
+ return false;
+ }
+
/**
* Detects http://code.google.com/p/v8/issues/detail?id=2396
*
@@ -1739,7 +1912,7 @@
var x;
x = (function a() {
function a() {}
- eval("");
+ eval('');
return a;
});
// x() should be the internal function a(), not itself
@@ -2228,8 +2401,7 @@
}
function getThrowTypeError() {
- "use strict";
- return
Object.getOwnPropertyDescriptor(getThrowTypeError, "arguments").get;
+ return
Object.getOwnPropertyDescriptor(getThrowTypeError, 'arguments').get;
}
/**
@@ -2253,6 +2425,81 @@
!!Object.getOwnPropertyDescriptor(tte, 'arguments') ||
!!Object.getOwnPropertyDescriptor(tte, 'caller');
}
+
+ /**
+ * See https://code.google.com/p/v8/issues/detail?id=2728
+ * and https://code.google.com/p/google-caja/issues/detail?id=1616
+ */
+ function test_SYNTAX_ERRORS_ARENT_ALWAYS_EARLY() {
+ try {
+ unsafeEval('throw "not a SyntaxError"; return;');
+ } catch (err) {
+ if (err === 'not a SyntaxError') {
+ return true;
+ } else if (err instanceof SyntaxError) {
+ return false;
+ }
+ return 'Unexpected error: ' + err;
+ }
+ return 'Invalid text parsed';
+ }
+
+ /**
+ * See https://code.google.com/p/google-caja/issues/detail?id=1616
+ */
+ function test_CANT_SAFELY_VERIFY_SYNTAX() {
+ try {
+ // See explanation above the call to ses.verifyStrictFunctionBody
+ // below.
+ Function('/*', '*/){');
+ } catch (err) {
+ if (err instanceof SyntaxError) { return false; }
+ return 'Unexpected error: ' + err;
+ }
+ if (ses.verifyStrictFunctionBody === simpleVerifyStrictFunctionBody) {
+ return true;
+ }
+
+ if (ses.verifyStrictFunctionBody ===
verifyStrictFunctionBodyByParsing) {
+ // This might not yet be the real one. If
+ // repair_CANT_SAFELY_VERIFY_SYNTAX decides to verify by
+ // parsing, then we're just going to assume here that it is safe
+ // since we might not yet have access to the real parser to test.
+ return false;
+ }
+
+ try {
+ ses.CANT_SAFELY_VERIFY_SYNTAX_canary = false;
+ try {
+ // This test, when tried with simpleVerifyStrictFunctionBody even
on
+ // Safari 6.0.4 WebKit Nightly r151081 (the latest at the time
+ // of this writing) causes the *browser* to crash.
+ //
+ // So to avoid crashing the browser, we first check if the
+ // Function constructor itself suffers from the same
+ // underlying problem, by making a similar check that does not
+ // crash the Safari browser. See
+ // https://bugs.webkit.org/show_bug.cgi?id=106160
+ // If this check shows that the underlying problem is absent
+ // then there's no problem. If it is present and no repair to
+ // ses.verifyStrictFunctionBody has yet been attempted, then we
+ // know we have the problem even without the following check.
+ //
+ // Even on Safari, if the repair has been attempted, then we
+ // do fall through to the following check, since it will no
+ // longer crash the browser.
+ ses.verifyStrictFunctionBody(
+ '}), (ses.CANT_SAFELY_VERIFY_SYNTAX_canary = true),
(function(){');
+ } catch (err) {
+ if (err instanceof SyntaxError) { return false; }
+ return 'Unexpected error: ' + err;
+ }
+ if (ses.CANT_SAFELY_VERIFY_SYNTAX_canary === true) { return true; }
+ return 'Unexpected verification failure';
+ } finally {
+ delete ses.CANT_SAFELY_VERIFY_SYNTAX_canary;
+ }
+ }
////////////////////// Repairs /////////////////////
//
@@ -2536,12 +2783,12 @@
value: function(callback, thisArg) {
var T, k;
if (this === null || this === undefined) {
- throw new TypeError("this is null or not defined");
+ throw new TypeError('this is null or not defined');
}
var O = Object(this);
var len = O.length >>> 0;
- if (objToString.call(callback) !== "[object Function]") {
- throw new TypeError(callback + " is not a function");
+ if (objToString.call(callback) !== '[object Function]') {
+ throw new TypeError(callback + ' is not a function');
}
T = thisArg;
k = 0;
@@ -2660,7 +2907,7 @@
!fullDesc.configurable) {
logger.warn(complaint);
throw new TypeError(complaint
- + " (Object: " + base + " Property: " + name + ")");
+ + ' (Object: ' + base + ' Property: ' + name + ')');
}
return defProp(base, name, fullDesc);
}
@@ -2675,8 +2922,8 @@
'"' + name + '" already non-configurable');
}
logger.warn(complaint);
- throw new TypeError(complaint + " (During sealing. Object: "
- + base + " Property: " + name + ")");
+ throw new TypeError(complaint + ' (During sealing. Object: '
+ + base + ' Property: ' + name + ')');
}
});
}
@@ -3020,7 +3267,7 @@
writable: true
});
}
-
+
function repair_PUSH_IGNORES_SEALED() {
var push = Array.prototype.push;
var sealed = Object.isSealed;
@@ -3145,6 +3392,32 @@
}
});
}
+
+ /**
+ * Note that this repair does not repair the Function constructor
+ * itself at this stage. Rather, it repairs ses.verifyStrictFunctionBody,
+ * which startSES uses to build a safe Function constructor from the
+ * unsafe one.
+ *
+ * <p>The repair strategy depends on what other bugs this platform
+ * suffers from. In the absence of SYNTAX_ERRORS_ARENT_ALWAYS_EARLY,
+ * STRICT_EVAL_LEAKS_GLOBAL_VARS, and
+ * STRICT_EVAL_LEAKS_GLOBAL_FUNCS, then we can use the cheaper
+ * verifyStrictFunctionBodyByEvalThrowing. Otherwise, if a parser is
+ * available, we use verifyStrictFunctionBodyByParsing. Otherwise we
+ * fail to repair.
+ */
+ function repair_CANT_SAFELY_VERIFY_SYNTAX() {
+ if (!test_SYNTAX_ERRORS_ARENT_ALWAYS_EARLY() &&
+ !test_STRICT_EVAL_LEAKS_GLOBAL_VARS() &&
+ !test_STRICT_EVAL_LEAKS_GLOBAL_FUNCS()) {
+ ses.verifyStrictFunctionBody =
verifyStrictFunctionBodyByEvalThrowing;
+ } else if (typeof ses.mitigateSrcGotchas === 'function') {
+ ses.verifyStrictFunctionBody = verifyStrictFunctionBodyByParsing;
+ } else {
+ // No known repairs under these conditions
+ }
+ }
////////////////////// Kludge Records /////////////////////
//
@@ -3670,16 +3943,27 @@
tests: [] // TODO(erights): Add to test262
},
{
- id: 'STRICT_EVAL_LEAKS_GLOBALS',
+ id: 'STRICT_EVAL_LEAKS_GLOBAL_VARS',
description: 'Strict eval function leaks variable definitions',
- test: test_STRICT_EVAL_LEAKS_GLOBALS,
+ test: test_STRICT_EVAL_LEAKS_GLOBAL_VARS,
repair: void 0,
- preSeverity: severities.SAFE_SPEC_VIOLATION,
+ preSeverity: severities.UNSAFE_SPEC_VIOLATION,
canRepair: false,
urls: ['http://code.google.com/p/v8/issues/detail?id=1624'],
sections: ['10.4.2.1'],
tests: ['S10.4.2.1_A1']
},
+ {
+ id: 'STRICT_EVAL_LEAKS_GLOBAL_FUNCS',
+ description: 'Strict eval function leaks function definitions',
+ test: test_STRICT_EVAL_LEAKS_GLOBAL_FUNCS,
+ repair: void 0,
+ preSeverity: severities.UNSAFE_SPEC_VIOLATION,
+ canRepair: false,
+ urls: ['http://code.google.com/p/v8/issues/detail?id=1624'],
+ sections: ['10.4.2.1'],
+ tests: ['S10.4.2.1_A1']
+ },
{
id: 'EVAL_BREAKS_MASKING',
description: 'Eval breaks masking of named functions in non-strict
code',
@@ -3934,6 +4218,33 @@
// TODO(kpreid): find or file Chrome bug (has a .prototype!)
sections: ['13.2.3'],
tests: [] // TODO(kpreid): add to test262
+ },
+ {
+ id: 'SYNTAX_ERRORS_ARENT_ALWAYS_EARLY',
+ description: 'SyntaxErrors aren\'t always early',
+ test: test_SYNTAX_ERRORS_ARENT_ALWAYS_EARLY,
+ repair: void 0,
+ preSeverity: severities.UNSAFE_SPEC_VIOLATION,
+ canRepair: false,
+ urls: ['https://code.google.com/p/v8/issues/detail?id=2728',
+ 'https://code.google.com/p/google-caja/issues/detail?id=1616'],
+ sections: [],
+ tests: []
+ },
+ {
+ id: 'CANT_SAFELY_VERIFY_SYNTAX',
+ description: 'Function constructor does not verify syntax',
+ test: test_CANT_SAFELY_VERIFY_SYNTAX,
+ // This does not repair Function but only
ses.verifyStrictFunctionBody
+ // (see above)
+ repair: repair_CANT_SAFELY_VERIFY_SYNTAX,
+ preSeverity: severities.NOT_ISOLATED,
+ canRepair: true,
+ urls: ['https://code.google.com/p/google-caja/issues/detail?id=1616',
+ 'http://code.google.com/p/v8/issues/detail?id=2470',
+ 'https://bugs.webkit.org/show_bug.cgi?id=106160'],
+ sections: ['15.3.2.1'],
+ tests: []
}
];
=======================================
--- /trunk/src/com/google/caja/ses/startSES.js Tue Jul 2 11:57:39 2013
+++ /trunk/src/com/google/caja/ses/startSES.js Thu Jul 11 18:45:27 2013
@@ -18,9 +18,12 @@
* <p>Assumes ES5 plus a WeakMap that conforms to the anticipated ES6
* WeakMap spec. Compatible with ES5-strict or anticipated ES6.
*
+ * //requires ses.makeCallerHarmless, ses.makeArgumentsHarmless
+ * //requires ses.verifyStrictFunctionBody
* //optionally requires ses.mitigateSrcGotchas
* //provides ses.startSES ses.resolveOptions, ses.securableWrapperSrc
* //provides ses.makeCompiledExpr
+ *
* @author Mark S. Miller,
* @author Jasvir Nagra
* @requires WeakMap
@@ -292,8 +295,8 @@
var options = {};
if (opt_mitigateOpts === undefined || opt_mitigateOpts === null) {
options.maskReferenceError = true;
+ options.parseFunctionBody = true;
- options.parseProgram = true;
options.rewriteTopLevelVars = true;
options.rewriteTopLevelFuncs = true;
options.rewriteFunctionCalls = true;
@@ -301,15 +304,8 @@
options.forceParseAndRender = false;
} else {
options.maskReferenceError = resolve('maskReferenceError', true);
+ options.parseFunctionBody = resolve('parseFunctionBody', false);
- if (opt_mitigateOpts.parseProgram === false) {
- ses.logger.warn(
- 'Refused to disable parsing for safety on all browsers');
- }
- // TODO(jasvir): This should only be necessary if a to-be-added
- // test in repairES5.js indicates that this platform has the
- // Function constructor bug
- options.parseProgram = true;
options.rewriteTopLevelVars = resolve('rewriteTopLevelVars', true);
options.rewriteTopLevelFuncs = resolve('rewriteTopLevelFuncs', true);
options.rewriteFunctionCalls = resolve('rewriteFunctionCalls', true);
@@ -330,11 +326,11 @@
* {@code options} are assumed to already be canonicalized by {@code
* resolveOptions} and says which mitigations to apply.
*/
- function mitigateSrcGotchas(programSrc, options) {
+ function mitigateSrcGotchas(funcBodySrc, options) {
var safeError;
if ('function' === typeof ses.mitigateSrcGotchas) {
try {
- return ses.mitigateSrcGotchas(programSrc, options, ses.logger);
+ return ses.mitigateSrcGotchas(funcBodySrc, options, ses.logger);
} catch (error) {
// Shouldn't throw, but if it does, the exception is potentially
from a
// different context with an undefended prototype chain; don't
allow it
@@ -348,7 +344,7 @@
throw safeError;
}
} else {
- return '' + programSrc;
+ return '' + funcBodySrc;
}
}
@@ -512,30 +508,6 @@
*/
var unsafeEval = eval;
var UnsafeFunction = Function;
-
- /**
- * Fails if {@code programSrc} does not parse as a strict Program
- * production, or, almost equivalently, as a FunctionBody
- * production.
- *
- * <p>We use Crock's trick of simply passing {@code programSrc} to
- * the original {@code Function} constructor, which will throw a
- * SyntaxError if it does not parse as a FunctionBody. We used to
- * use Ankur's trick (need link) which is more correct, in that it
- * will throw if {@code programSrc} does not parse as a Program
- * production, which is the relevant question. However, the
- * difference -- whether return statements are accepted -- does
- * not matter for our purposes. And testing reveals that Crock's
- * trick executes over 100x faster on V8.
- */
- function verifyStrictProgram(programSrc) {
- try {
- UnsafeFunction('"use strict";' + programSrc);
- } catch (err) {
- // debugger; // Useful for debugging -- to look at programSrc
- throw err;
- }
- }
/**
* Fails if {@code exprSource} does not parse as a strict
@@ -543,14 +515,21 @@
*
* <p>To verify that exprSrc parses as a strict Expression, we
* verify that, when surrounded by parens and followed by ";", it
- * parses as a strict Program, and that when surrounded with
- * double parens it still parses as a strict Program. We place a
- * newline before the terminal token so that a "//" comment
- * cannot suppress the close paren or parens.
+ * parses as a strict FunctionBody, and that when surrounded with
+ * double parens it still parses as a strict FunctionBody. We
+ * place a newline before the terminal token so that a "//"
+ * comment cannot suppress the close paren or parens.
+ *
+ * <p>We never check without parens because not all
+ * expressions, for example "function(){}", form valid expression
+ * statements. We check both single and double parens so there's
+ * no exprSrc text which can close the left paren(s), do
+ * something, and then provide open paren(s) to balance the final
+ * close paren(s). No one such attack will survive both tests.
*/
function verifyStrictExpression(exprSrc) {
- verifyStrictProgram('( ' + exprSrc + '\n);');
- verifyStrictProgram('(( ' + exprSrc + '\n));');
+ ses.verifyStrictFunctionBody('( ' + exprSrc + '\n);');
+ ses.verifyStrictFunctionBody('(( ' + exprSrc + '\n));');
}
/**
@@ -790,8 +769,8 @@
* cases. This is a less correct but faster alternative to
* rewriteTypeOf that also works when source mitigations are
* not available.
- * <li>parseProgram: check the program is syntactically
- * valid.
+ * <li>parseFunctionBody: check the src is syntactically
+ * valid as a function body.
* <li>rewriteTopLevelVars: transform vars to properties of global
* object. Defaults to true.
* <li>rewriteTopLevelFuncs: transform funcs to properties of
@@ -808,12 +787,6 @@
* maskReferenceError.
* </ul>
*
- * <p>Currently for security, parseProgram is always true and
- * cannot be unset because of the <a href=
- * "https://code.google.com/p/v8/issues/detail?id=2470";
- * >Function constructor bug</a>. TODO(jasvir): On platforms not
- * suffering from this bug, parseProgram should default to false.
- *
* <p>When SES is provided primitively, it should provide an
* analogous {@code compileProgram} function that accepts a
* Program and return a function that evaluates it to the
@@ -963,6 +936,7 @@
params = params.join(',');
// Note the EOL after modSrc to prevent trailing line comment in body
// eliding the rest of the wrapper.
+ ses.verifyStrictFunctionBody(body);
var exprSrc = '(function(' + params + '\n){' + body + '\n})';
return compileExpr(exprSrc)(sharedImports);
}
--
---
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 [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
