Revision: 3625
Author: metaweta
Date: Fri Jul 31 16:33:01 2009
Log: Restrict numeric whitelisting to nonnegative integers.
http://codereview.appspot.com/96200
* Changed CajitaRewriter.java and Rule.java to whitelist expressions of
the form @o...@s & -1 <<< 1] instead of @o...@s]
* Changed cajita.js to check that index >= 0 when allowing numeric reads
* Added tests to CajitaRewriterTest and domita_test_untrusted.html to
check that neither Rhino nor the browser return anything but undefined
on reading a negative index.
* Updated structural tests in CajitaTest.java
[email protected]
http://code.google.com/p/google-caja/source/detail?r=3625
Modified:
/trunk/src/com/google/caja/cajita.js
/trunk/src/com/google/caja/parser/quasiliteral/CajitaRewriter.java
/trunk/src/com/google/caja/parser/quasiliteral/Rule.java
/trunk/tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java
/trunk/tests/com/google/caja/plugin/domita_test_untrusted.html
=======================================
--- /trunk/src/com/google/caja/cajita.js Tue Jul 21 15:19:23 2009
+++ /trunk/src/com/google/caja/cajita.js Fri Jul 31 16:33:01 2009
@@ -1374,7 +1374,7 @@
* property of x, then canReadPub(y, 'foo') must be true.
*/
function canReadPub(obj, name) {
- if (typeof name === 'number') { return name in obj; }
+ if (typeof name === 'number' && name >= 0) { return name in obj; }
name = String(name);
if (obj === null) { return false; }
if (obj === void 0) { return false; }
@@ -1388,7 +1388,7 @@
}
function hasOwnPropertyOf(obj, name) {
- if (typeof name === 'number') { return hasOwnProp(obj, name); }
+ if (typeof name === 'number' && name >= 0) { return hasOwnProp(obj,
name); }
name = String(name);
if (obj && obj[name + '_canRead___'] === obj) { return true; }
return canReadPub(obj, name) && myOriginalHOP.call(obj, name);
@@ -1416,13 +1416,17 @@
* If it can't then <tt>readPub</tt> returns <tt>undefined</tt> instead.
*/
function readPub(obj, name) {
- if (typeof name === 'number') {
+ if (typeof name === 'number' && name >= 0) {
if (typeof obj === 'string') {
// In partial anticipation of ES3.1.
// TODO(erights): Once ES3.1 settles, revisit this and
// correctly implement the agreed semantics.
- // Mike Samuel suggests also making it conditional on
+ // Mike Samuel suggested also making it conditional on
// (+name) === (name & 0x7fffffff)
+ // but then realized that it violates the requirement
+ // that the string form be the canonical form of the
+ // number. So 'foo'['00'] would be treated the same
+ // as 'foo'['0'] which is incorrect.
return obj.charAt(name);
} else {
return obj[name];
@@ -1466,7 +1470,7 @@
return pumpkin;
}
}
- if (typeof name === 'number') {
+ if (typeof name === 'number' && name >= 0) {
if (myOriginalHOP.call(obj, name)) { return obj[name]; }
return pumpkin;
}
@@ -1764,6 +1768,7 @@
// the check is expensive in this position.
// val = asFirstClass(val);
if (typeof name === 'number' &&
+ name >= 0 &&
// See issue 875
obj instanceof Array &&
obj.FROZEN___ !== obj) {
=======================================
--- /trunk/src/com/google/caja/parser/quasiliteral/CajitaRewriter.java Fri
Jul 17 17:55:20 2009
+++ /trunk/src/com/google/caja/parser/quasiliteral/CajitaRewriter.java Fri
Jul 31 16:33:01 2009
@@ -1048,14 +1048,15 @@
@Override
@RuleDescription(
name="readNumPublic",
- synopsis="Recognize that numeric indexing is inherently safe.",
+ synopsis="Recognize that array indexing is inherently safe.",
reason="When the developer knows that their index expression is"
+
- " numeric, they can indicate this with the unary plus
operator" +
- " -- which coerces to a number. Since numeric properties
are" +
- " necessarily readable, we can pass these through directly
to" +
- " JavaScript.",
- matches="@o...@s]",
- substitutes="@o...@s]")
+ " an array index, they can indicate this with the" +
+ " 'absolute value operator', really an expression which" +
+ " coerces to a nonnegative 32-bit integer. Since these" +
+ " properties are necessarily readable, we can pass them " +
+ " through directly to JavaScript.",
+ matches="@o...@s&(-1>>>1)]",
+ substitutes="@o...@s&(-1>>>1)]")
public ParseTreeNode fire(
ParseTreeNode node, Scope scope, MessageQueue mq) {
return transform(node, scope, mq);
@@ -1066,9 +1067,9 @@
@Override
@RuleDescription(
name="readNumWithConstantIndex",
- synopsis="Recognize that numeric indexing is inherently safe.",
- reason="Numeric properties are always readable, we can pass
these"
- + " through directly to JavaScript.",
+ synopsis="Recognize that array indexing is inherently safe.",
+ reason="Nonnegative integer properties are always readable;" +
+ " we can pass these through directly to JavaScript.",
matches="@o...@numliteral]",
substitutes="@o...@numliteral]")
public ParseTreeNode fire(
@@ -1077,9 +1078,14 @@
if (bindings != null) {
ParseTreeNode index = bindings.get("numLiteral");
if (index instanceof NumberLiteral) {
- return substV(
- "o", expand(bindings.get("o"), scope, mq),
- "numLiteral", expand(index, scope, mq));
+ double indexValue =
+ ((NumberLiteral) index).getValue().doubleValue();
+ if (indexValue >= 0 &&
+ indexValue == Math.floor(indexValue)) {
+ return substV(
+ "o", expand(bindings.get("o"), scope, mq),
+ "numLiteral", expand(index, scope, mq));
+ }
}
}
return NONE;
@@ -1484,7 +1490,7 @@
ops.getUncajoledLValue(), aNode.children().get(1));
Operation assignment = ops.makeAssignment(rhs);
return commas(newCommaOperation(ops.getTemporaries()),
- (Expression) expand(assignment, scope, mq));
+ (Expression) expand(assignment, scope, mq));
}
return NONE;
}
=======================================
--- /trunk/src/com/google/caja/parser/quasiliteral/Rule.java Wed May 20
11:46:54 2009
+++ /trunk/src/com/google/caja/parser/quasiliteral/Rule.java Fri Jul 31
16:33:01 2009
@@ -595,8 +595,10 @@
ParseTreeNode rightExpanded = rewriter.expand(uncajoledKey, scope,
mq);
Identifier tmpVar = scope.declareStartOfScopeTempVariable();
key = new Reference(tmpVar);
- if (isToNumberOp(rightExpanded)) {
- key = Operation.create(key.getFilePosition(), Operator.TO_NUMBER,
key);
+ if (QuasiBuilder.match("@s&(-1>>>1)", rightExpanded)) {
+ // TODO(metaweta): Figure out a way to leave key alone and
+ // protect propertyAccess from rewriting instead.
+ key = (Expression) QuasiBuilder.substV("@key&(-1>>>1)", "key",
key);
}
temporaries.add((Expression) QuasiBuilder.substV(
"@tmpVar = @right;",
@@ -646,11 +648,6 @@
if (!(e instanceof Reference)) { return false; }
return ReservedNames.IMPORTS.equals(((Reference)
e).getIdentifierName());
}
-
- private static boolean isToNumberOp(ParseTreeNode n) {
- return n instanceof Operation
- && Operator.TO_NUMBER == ((Operation) n).getOperator();
- }
/**
* The operands in a read/assign operation.
=======================================
---
/trunk/tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java
Thu Jul 23 09:16:30 2009
+++
/trunk/tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java
Fri Jul 31 16:33:01 2009
@@ -935,44 +935,44 @@
public final void testReadNumPublic() throws Exception {
checkSucceeds(
"var p, q, x;" +
- "p = q[+x];",
+ "p = q[x&(-1>>>1)];",
"var p, q, x;" +
- "p = q[+x];");
+ "p = q[x&(-1>>>1)];");
// Make sure that setPub is used on assignment to test frozenness.
checkSucceeds(
"var p, q, x;" +
- "q[+x] = p;",
+ "q[x&(-1>>>1)] = p;",
"var p, q, x;" +
- "___.setPub(q, +x, p);");
+ "___.setPub(q, x&(-1>>>1), p);");
checkSucceeds(
"var p, q;" +
- "p[+q] += 2;",
+ "p[q&(-1>>>1)] += 2;",
"var x0___;" +
"var x1___;" +
"var p, q;" +
"x0___ = p," +
- "x1___ = +q," +
- "___.setPub(x0___, +x1___, x0___[+x1___] + 2);");
+ "x1___ = q&(-1>>>1)," +
+ "___.setPub(x0___, x1___&(-1>>>1), x0___[x1___&(-1>>>1)] + 2);");
checkSucceeds(
"var p, q;" +
- "p[+q]--;",
+ "p[q&(-1>>>1)]--;",
"var x0___;" +
"var x1___;" +
"var x2___;" +
"var p, q;" +
"x0___ = p," +
- "x1___ = +q," +
- "x2___ = +x0___[+x1___]," +
- "___.setPub(x0___, +x1___, x2___ - 1), x2___;");
+ "x1___ = q&(-1>>>1)," +
+ "x2___ = +x0___[x1___&(-1>>>1)]," +
+ "___.setPub(x0___, x1___&(-1>>>1), x2___ - 1), x2___;");
checkSucceeds(
"var p, q;" +
- "--p[+q];",
+ "--p[q&(-1>>>1)];",
"var x0___;" +
"var x1___;" +
"var p, q;" +
"x0___ = p," +
- "x1___ = +q," +
- "___.setPub(x0___, +x1___, x0___[+x1___] - 1);");
+ "x1___ = q&(-1>>>1)," +
+ "___.setPub(x0___, x1___&(-1>>>1), x0___[x1___&(-1>>>1)] - 1);");
}
public final void testNumWithConstantIndex() throws Exception {
=======================================
--- /trunk/tests/com/google/caja/plugin/domita_test_untrusted.html Thu Jul
30 19:16:24 2009
+++ /trunk/tests/com/google/caja/plugin/domita_test_untrusted.html Fri Jul
31 16:33:01 2009
@@ -360,6 +360,8 @@
</table>
</div>
+<p class="testcontainer" id="test-negative-indices" />
+
<p class="testcontainer" id="test-document-body-appendChild"
>I should be the last element until something is appended to
document.body</p>
@@ -2481,6 +2483,24 @@
pass('test-row-cell');
});
+jsunitRegister('testNegativeIndices',
+ function testNegativeIndices() {
+ // Under Firefox, there are many undocumented properties
+ // with negative indices; see
+ //
http://www.thespanner.co.uk/2009/07/14/hidden-firefox-properties-revisited/
+ //
http://webreflection.blogspot.com/2009/06/javascript-arguments-weridness.html
+ var i;
+ function f(a, b, c) {}
+ (function () {
+ for (i = -1; i > -10; --i) {
+ assertEquals(f[i], void 0);
+ assertEquals((new RegExp('.'))[i], void 0);
+ assertEquals(arguments[i], void 0);
+ }
+ })();
+ pass('test-negative-indices');
+});
+
jsunitRegister('testDocumentBodyAppendChild',
function testDocumentBodyAppendChild() {
var notWhitespaceRe = new RegExp('\\S');
