Revision: 5630
Author: [email protected]
Date: Thu Nov 21 20:21:46 2013 UTC
Log: Reject, in the mitigator, JS reserved names written with escapes,
to avoid misinterpretation.
https://codereview.appspot.com/19560044
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.
[email protected], [email protected]
http://code.google.com/p/google-caja/source/detail?r=5630
Added:
/trunk/tests/com/google/caja/ses/test-ses-mitigation-min.html
Modified:
/trunk/build.xml
/trunk/src/com/google/caja/ses/exportsToSES.js
/trunk/src/com/google/caja/ses/mitigateGotchas.js
/trunk/tests/com/google/caja/ses/ses-tests.json
/trunk/tests/com/google/caja/ses/test-ses-mitigation.html
/trunk/tests/com/google/caja/ses/test-ses-mitigation.js
=======================================
--- /dev/null
+++ /trunk/tests/com/google/caja/ses/test-ses-mitigation-min.html Thu Nov
21 20:21:46 2013 UTC
@@ -0,0 +1,36 @@
+<!doctype html>
+<!--
+ - Copyright (C) 2013 Google Inc.
+ -
+ - Licensed under the Apache License, Version 2.0 (the "License");
+ - you may not use this file except in compliance with the License.
+ - You may obtain a copy of the License at
+ -
+ - http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+-->
+
+<html>
+ <head>
+ <meta charset="utf-8">
+ <title>SES mitigation test</title>
+ <link rel="stylesheet" href="../plugin/browser-test.css"
type="text/css">
+ <script type="text/javascript"
src="/ant-lib/com/google/caja/console.js"></script>
+ <script type="text/javascript" src="../plugin/jsUnitCore.js"></script>
+ <script type="text/javascript" src="../plugin/jsunit.js"></script>
+
+ <!-- we don't need all of SES, but this is most convenient for getting
the
+ minified version to test -->
+ <script type="text/javascript"
src="/ant-lib/com/google/caja/plugin/utility-frame.opt.js"></script>
+ <script type="text/javascript"
src="/ant-lib/com/google/caja/ses/initSES-minified.js"></script>
+ </head>
+ <body class="readytotest">
+ <p id="toolbar"><a href="test-ses-mitigation.html">unminified</a></p>
+ <script type="text/javascript" src="test-ses-mitigation.js"></script>
+ </body>
+</html>
=======================================
--- /trunk/build.xml Tue Nov 5 23:53:19 2013 UTC
+++ /trunk/build.xml Thu Nov 21 20:21:46 2013 UTC
@@ -798,6 +798,7 @@
<output file="${lib.caja}/plugin/utility-frame.js"
language="javascript" renderer="concat"/>
<input file="${src.caja}/plugin/caja-iframe-build-version.js"/>
+ <input file="${src.caja}/ses/StringMap.js"/>
<input file="${src.caja}/ses/createExports.js"/>
<input file="${third_party}/js/acorn/acorn.js" jslint="false"/>
<input file="${third_party}/js/escodegen/escodegen.js"
jslint="false"/>
=======================================
--- /trunk/src/com/google/caja/ses/exportsToSES.js Fri Jul 12 01:45:27 2013
UTC
+++ /trunk/src/com/google/caja/ses/exportsToSES.js Thu Nov 21 20:21:46 2013
UTC
@@ -31,6 +31,7 @@
global.ses = global.ses || {};
ses.rewriter_ = {};
+ ses.rewriter_.tokTypes = exports.tokTypes;
ses.rewriter_.traverse = exports.traverse;
ses.rewriter_.parse = exports.parse;
ses.rewriter_.generate = exports.generate;
=======================================
--- /trunk/src/com/google/caja/ses/mitigateGotchas.js Thu Nov 7 23:36:27
2013 UTC
+++ /trunk/src/com/google/caja/ses/mitigateGotchas.js Thu Nov 21 20:21:46
2013 UTC
@@ -27,12 +27,48 @@
* //requires ses.rewriter_
* //provides ses.mitigateSrcGotchas
* @author Jasvir Nagra ([email protected])
+ * @requires JSON, StringMap
* @overrides ses
*/
var ses;
(function() {
+ // There is a bug which is problematic for us: JS implementations vary on
+ // whether they consider a reserved word written using unicode escapes to
+ // actually be a reserved word; for example, the following program can be
+ // parsed two ways:
+ // de\u006Cete /"x/ //";
+ //
+ // The specification is somewhat unclear but the consensus is that the
correct
+ // behavior is to parse an escaped reserved word as a reserved word. See:
+ // 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
+ //
+ // This has two consequences for us:
+ // 1. We cannot rely on the JS implementation to parse such a program in
the
+ // same way we do (unless we were to vary our parser's behavior based
on a
+ // feature test, which we currently do not), so if we parse a
program, then
+ // we must also rerender it in an unambiguous way matching our
+ // interpretation (rather than passing the original source to the
browser's
+ // parser); but our parser, Acorn, takes the incorrect "de\u006Cete
is an
+ // identifier" option which cannot be supported on all platforms.
+ // 2. Our parser, Acorn, and renderer, escodegen, are inconsistent with
each
+ // other: an escaped reserved word is parsed as an identifier, but
will be
+ // rendered unescaped.
+ //
+ // Due to the above issues, and since such programs are therefore
unportable,
+ // we currently take the simplest approach, namely to reject such
programs.
+ // In order to do so, we must traverse all programs we parse; the
constant
+ // defined here marks the locations where kludges to do so have been
inserted.
+ // If implementations stop being inconsistent about this issue (or if
acorn,
+ // escodegen, and the current JS implementation happen to agree), then we
+ // could change this to false.
+ var ESCAPED_KEYWORD_AMBIGUITY = true;
+
function introducesVarScope(node) {
return node.type === 'FunctionExpression' ||
node.type === 'FunctionDeclaration';
@@ -94,6 +130,28 @@
function isFunctionCall(node) {
return node.type === 'CallExpression' && isId(node.callee);
}
+
+ var nameIsReservedWord = (function() {
+ // TODO(kpreid): Fragile; find a better way to do this that does not
depend
+ // on Acorn's data structures quite so much.
+ var tokTypes = ses.rewriter_.tokTypes;
+ var table = new StringMap();
+ for (var k in tokTypes) {
+ if ('keyword' in tokTypes[k]) {
+ table.set(tokTypes[k].keyword, 0);
+ }
+ }
+ return table.has.bind(table);
+ })();
+
+ // Is the node one whose identifier child is an IdentifierName in the ES5
+ // grammar, and therefore allowed to be a ReservedWord?
+ function isIdentifierNameContext(node) {
+ var type = node.type;
+ // property initializer nodes have no .type; they have a .kind but so
do
+ // labels
+ return type === 'MemberExpression' || !!(node.kind && node.key);
+ }
/**
* Rewrite func decls in place by appending assignments on the global
object
@@ -296,6 +354,17 @@
function rewrite(scope, node) {
ses.rewriter_.traverse(node, {
enter: function enter(node, parentNode) {
+
+ if (ESCAPED_KEYWORD_AMBIGUITY && isId(node)) {
+ if (nameIsReservedWord(node.name) &&
+ !isIdentifierNameContext(parentNode)) {
+ throw new SyntaxError(
+ 'Programs containing Unicode escapes in reserved words '
+
+ 'will be misparsed on some platforms and are not
currently ' +
+ 'permitted by SES.');
+ }
+ }
+
if (scope.options.rewriteTopLevelFuncs &&
isFunctionDecl(node) && scope.scopeLevel === 0) {
rewriteFuncDecl(scope, node, parentNode);
@@ -337,7 +406,7 @@
}
function rewriteProgram(options, ast) {
- if (needsRewriting(options)) {
+ if (ESCAPED_KEYWORD_AMBIGUITY || needsRewriting(options)) {
var scope = {
options: options,
dirty: false,
@@ -378,12 +447,15 @@
return funcBodySrc;
}
} catch (e) {
- logger.warn('Failed to parse program', e);
- // TODO(jasvir): Consider using the thrown exception to provide
- // a more useful descriptive error message. Be aware of naively
- // interpolating error message strings.
+ var message = '' + e;
+ // Chrome console does not display an Error object usefully but as
+ // "Error {}" so we use the explicit stringification.
+ logger.warn('Failed to parse program: ' + message);
+ var quotedMessage = JSON.stringify(message);
return '' +
- '(function() { throw new SyntaxError("Failed to parse program");
})()';
+ '(function() { throw new SyntaxError("Failed to parse program: "
+ ' +
+ quotedMessage +
+ '); })()';
}
};
=======================================
--- /trunk/tests/com/google/caja/ses/ses-tests.json Tue Oct 8 16:46:28
2013 UTC
+++ /trunk/tests/com/google/caja/ses/ses-tests.json Thu Nov 21 20:21:46
2013 UTC
@@ -48,7 +48,16 @@
},
{
"label": "mitigation",
- "bare": "../ses/test-ses-mitigation.html"
+ "tests": [
+ {
+ "label": "nomin",
+ "bare": "../ses/test-ses-mitigation.html"
+ },
+ {
+ "label": "min",
+ "bare": "../ses/test-ses-mitigation-min.html"
+ }
+ ]
},
{
"label": "parts",
=======================================
--- /trunk/tests/com/google/caja/ses/test-ses-mitigation.html Thu Nov 7
19:59:16 2013 UTC
+++ /trunk/tests/com/google/caja/ses/test-ses-mitigation.html Thu Nov 21
20:21:46 2013 UTC
@@ -24,6 +24,7 @@
<script type="text/javascript" src="../plugin/jsUnitCore.js"></script>
<script type="text/javascript" src="../plugin/jsunit.js"></script>
+ <script type="text/javascript" src="StringMap.js"></script>
<!-- createExports / exportsToSES glue up the modules -->
<script type="text/javascript" src="createExports.js"></script>
<script type="text/javascript"
src="/ant-testlib/js/acorn/acorn.js"></script>
@@ -32,7 +33,9 @@
<script type="text/javascript" src="exportsToSES.js"></script>
<script type="text/javascript"
src="/ant-testlib/com/google/caja/ses/mitigateGotchas.js"></script>
+ </head>
+ <body class="readytotest">
+ <p id="toolbar"><a href="test-ses-mitigation-min.html">minified</a></p>
<script type="text/javascript" src="test-ses-mitigation.js"></script>
- </head>
- <body class="readytotest"></body>
+ </body>
</html>
=======================================
--- /trunk/tests/com/google/caja/ses/test-ses-mitigation.js Wed Jul 31
16:40:58 2013 UTC
+++ /trunk/tests/com/google/caja/ses/test-ses-mitigation.js Thu Nov 21
20:21:46 2013 UTC
@@ -57,57 +57,110 @@
function assertNoMitigate(src, options) {
assertMitigate(src, src, options);
}
+
+function assertMitigateFails(src, options, error) {
+ var out = '';
+ var logger = { warn: function (var_args) {
+ out += Array.prototype.join.call(arguments, ' ');
+ }};
+
+ ses.mitigateSrcGotchas(src, options, logger);
+ assertEquals(src, error, out);
+}
jsunitRegister('testRewritePropertyUpdateExpr',
function testRewritePropertyUpdateExpr() {
- var o = { rewritePropertyUpdateExpr: true };
+ var o = { rewritePropertyUpdateExpr: true };
- assertNoMitigate('x++;', o);
+ assertNoMitigate('x++;', o);
- assertMitigate('o[(1, "x")]++;', 'o.x++;', o);
- assertMitigate('o[(1, "x")]--;', 'o.x--;', o);
- assertMitigate('o[(1, "x")]++;', 'o["x"]++;', o);
- assertMitigate('o[(1, "x")]--;', 'o["x"]--;', o);
- assertMitigate('o[(1, 3)]++;', 'o[3]++;', o);
- assertMitigate('o[(1, 3)]--;', 'o[3]--;', o);
+ assertMitigate('o[(1, "x")]++;', 'o.x++;', o);
+ assertMitigate('o[(1, "x")]--;', 'o.x--;', o);
+ assertMitigate('o[(1, "x")]++;', 'o["x"]++;', o);
+ assertMitigate('o[(1, "x")]--;', 'o["x"]--;', o);
+ assertMitigate('o[(1, 3)]++;', 'o[3]++;', o);
+ assertMitigate('o[(1, 3)]--;', 'o[3]--;', o);
- assertNoMitigate('o[3 + 4]++;', o);
- assertNoMitigate('o[p]++;', o);
+ assertNoMitigate('o[3 + 4]++;', o);
+ assertNoMitigate('o[p]++;', o);
- assertMitigate('o[q[(1, "y")]++]--;', 'o[q.y++]--;', o);
- assertMitigate('foo(q[(1, "y")]++)[(1, 3)]--;', 'foo(q.y++)[3]--;',
o);
+ assertMitigate('o[q[(1, "y")]++]--;', 'o[q.y++]--;', o);
+ assertMitigate('foo(q[(1, "y")]++)[(1, 3)]--;', 'foo(q.y++)[3]--;', o);
- jsunitPass('testRewritePropertyUpdateExpr');
- });
+ jsunitPass();
+});
jsunitRegister('testRewritePropertyCompoundAssignmentExpr',
function testRewritePropertyCompoundAssignmentExpr() {
- var o = { rewritePropertyCompoundAssignmentExpr: true };
+ var o = { rewritePropertyCompoundAssignmentExpr: true };
+
+ assertNoMitigate('x += 1;', o);
+ assertNoMitigate('o.x = 1;', o);
+ assertNoMitigate('o.x = o.x + 1;', o);
+
+ assertMitigate('o[(1, "x")] += 1;', 'o.x += 1;', o);
+ assertMitigate('o[(1, "x")] += 1;', 'o["x"] += 1;', o);
+ assertMitigate('o[(1, 3)] += 1;', 'o[3] += 1;', o);
+ assertMitigate('o[(1, "x")] -= 1;', 'o.x -= 1;', o);
+ assertMitigate('o[(1, "x")] *= 1;', 'o.x *= 1;', o);
+ assertMitigate('o[(1, "x")] /= 1;', 'o.x /= 1;', o);
+ assertMitigate('o[(1, "x")] &= 1;', 'o.x &= 1;', o);
+ assertMitigate('o[(1, "x")] |= 1;', 'o.x |= 1;', o);
+ assertMitigate('o[(1, "x")] ^= 1;', 'o.x ^= 1;', o);
+ assertMitigate('o[(1, "x")] %= 1;', 'o.x %= 1;', o);
+ assertMitigate('o[(1, "x")] <<= 1;', 'o.x <<= 1;', o);
+ assertMitigate('o[(1, "x")] >>= 1;', 'o.x >>= 1;', o);
+ assertMitigate('o[(1, "x")] >>>= 1;', 'o.x >>>= 1;', o);
+
+ assertMitigate(
+ 'foo(o[(1, "x")] += 1)[(1, "z")] += (q[(1, "y")] += 1)',
+ 'foo(o.x += 1).z += (q.y += 1);',
+ o);
+
+ jsunitPass();
+});
+
+
+jsunitRegister('testEscapedKeyword', function testEscapedKeyword() {
+ // To best exercise the relevant code path, provide options which
require a
+ // parse, but do not require any normal rewriting.
+ var o = { parseFunctionBody: true };
+
+ var out = '';
+ var logger = { warn: function (var_args) {
+ out += Array.prototype.join.call(arguments, ' ');
+ }};
+
+ // note double backslash; this is a unicode escape sequence in the
program
+ // text
+ assertMitigateFails('de\\u006Cete /"x/ //";', o,
+ 'Failed to parse program: SyntaxError: Programs containing Unicode '
+
+ 'escapes in reserved words will be misparsed on some platforms and
are ' +
+ 'not currently permitted by SES.');
- assertNoMitigate('x += 1;', o);
- assertNoMitigate('o.x = 1;', o);
- assertNoMitigate('o.x = o.x + 1;', o);
+ // Reserved words in IdentifierName positions are not rejected.
+ assertNoMitigate('({delete: 1})', o);
+ assertNoMitigate('({get delete() {}})', o);
+ assertNoMitigate('({set delete(x) {}})', o);
+ assertNoMitigate('foo.delete', o);
+ assertNoMitigate('foo().delete', o);
+ assertNoMitigate('foo.delete()', o);
- assertMitigate('o[(1, "x")] += 1;', 'o.x += 1;', o);
- assertMitigate('o[(1, "x")] += 1;', 'o["x"] += 1;', o);
- assertMitigate('o[(1, 3)] += 1;', 'o[3] += 1;', o);
- assertMitigate('o[(1, "x")] -= 1;', 'o.x -= 1;', o);
- assertMitigate('o[(1, "x")] *= 1;', 'o.x *= 1;', o);
- assertMitigate('o[(1, "x")] /= 1;', 'o.x /= 1;', o);
- assertMitigate('o[(1, "x")] &= 1;', 'o.x &= 1;', o);
- assertMitigate('o[(1, "x")] |= 1;', 'o.x |= 1;', o);
- assertMitigate('o[(1, "x")] ^= 1;', 'o.x ^= 1;', o);
- assertMitigate('o[(1, "x")] %= 1;', 'o.x %= 1;', o);
- assertMitigate('o[(1, "x")] <<= 1;', 'o.x <<= 1;', o);
- assertMitigate('o[(1, "x")] >>= 1;', 'o.x >>= 1;', o);
- assertMitigate('o[(1, "x")] >>>= 1;', 'o.x >>>= 1;', o);
+ jsunitPass();
+});
- assertMitigate(
- 'foo(o[(1, "x")] += 1)[(1, "z")] += (q[(1, "y")] += 1)',
- 'foo(o.x += 1).z += (q.y += 1);',
- o);
+// Note for context: A prior attempt to upgrade third_party/js/escodegen
+// resulted in a bug where every character in regexp literals was doubled,
only
+// with minified SES. This test is provided to notice/explain the problem
if we
+// hit it again.
+jsunitRegister('testRegexpRegression', function testRegexpRegression() {
+ var options = {
+ forceParseAndRender: true,
+ rewriteFunctionCalls: true
+ };
+ assertMitigate('(1, f)(/31337/g);', 'f(/31337/g);', options);
- jsunitPass('testRewritePropertyCompoundAssignmentExpr');
- });
+ jsunitPass();
+});
jsunitRun();
--
---
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.
