Merge pull request #1098 from dvoytenko/mustache-sec1

Security review: builtin calls and prototype chain restrictions
ampproject · Dec 9, 2015 · f1f397c · f1f397c
2 parents 66c24b2 + 8b9ef6c
commit f1f397c
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 29 deletions.
diff --git a/src/sanitizer.js b/src/sanitizer.js
@@ -33,25 +33,32 @@ const SELF_CLOSING_TAGS = {
 };
 
 
-/** @const {!Object<string, boolean>} */
-const WHITELISTED_FORMAT_TAGS = {
-  'b': true,
-  'br': true,
-  'code': true,
-  'del': true,
-  'em': true,
-  'i': true,
-  'ins': true,
-  'mark': true,
-  'q': true,
-  's': true,
-  'small': true,
-  'strong': true,
-  'sub': true,
-  'sup': true,
-  'time': true,
-  'u': true,
-};
+/** @const {!Array<string>} */
+const WHITELISTED_FORMAT_TAGS = [
+  'b',
+  'br',
+  'code',
+  'del',
+  'em',
+  'i',
+  'ins',
+  'mark',
+  'q',
+  's',
+  'small',
+  'strong',
+  'sub',
+  'sup',
+  'time',
+  'u',
+];
+
+
+/** @const {!Array<string>} */
+const BLACKLISTED_ATTR_VALUES = [
+  /*eslint no-script-url: 0*/ 'javascript:',
+  /*eslint no-script-url: 0*/ 'vbscript:',
+];
 
 
 /**
@@ -132,7 +139,7 @@ export function sanitizeHtml(html) {
  */
 export function sanitizeFormattingHtml(html) {
   return htmlSanitizer.sanitizeWithPolicy(html, function(tagName, attrs) {
-    if (!WHITELISTED_FORMAT_TAGS[tagName]) {
+    if (WHITELISTED_FORMAT_TAGS.indexOf(tagName) == -1) {
       return null;
     }
     return {
@@ -161,10 +168,12 @@ export function isValidAttr(attrName, attrValue) {
     return false;
   }
 
-  // No attributes with "javascript" in them.
-  if (attrValue.toLowerCase().indexOf(
-          /*eslint no-script-url: 0*/ 'javascript:') != -1) {
-    return false;
+  // No attributes with "javascript" or other blacklisted substrings in them.
+  const attrValueLowercase = attrValue.toLowerCase();
+  for (let i = 0; i < BLACKLISTED_ATTR_VALUES.length; i++) {
+    if (attrValueLowercase.indexOf(BLACKLISTED_ATTR_VALUES[i]) != -1) {
+      return false;
+    }
   }
 
   return true;

diff --git a/test/functional/test-mustache.js b/test/functional/test-mustache.js
@@ -40,4 +40,32 @@ describe('Mustache', () => {
     expect(mustache.render('{{{value}}}', {value: '<b>abc</b>'})).to.equal(
         '<B>ABC</B>');
   });
+
+  it('should only expand own properties', () => {
+    const parent = {value: 'bc'};
+    const child = Object.create(parent);
+    const container = {parent: parent, child: child};
+    expect(mustache.render('a{{value}}', parent)).to.equal('abc');
+    expect(mustache.render('a{{value}}', child)).to.equal('a');
+    expect(mustache.render('a{{parent.value}}', container)).to.equal('abc');
+    expect(mustache.render('a{{child.value}}', container)).to.equal('a');
+  });
+
+  it('should NOT allow calls to builtin functions', () => {
+    // Calls on x.pop in classical Mustache would lead to builtin call and
+    // mutate on the 't' object. Here we will not allow it. We explicitly
+    // prohibit such calls.
+    const obj = {
+      't': {
+        '0': '0',
+        '1': '1',
+        'length': 2,
+        'x': []
+      }
+    };
+    expect(mustache.render(
+        '{{#t}}{{x.pop}}X{{x.pop}}{{/t}}' +
+        '{{#t}}{{0}}Y{{1}}{{/t}}',
+        obj)).to.equal('X0Y1');
+  });
 });
diff --git a/test/functional/test-sanitizer.js b/test/functional/test-sanitizer.js
@@ -79,6 +79,10 @@ describe('sanitizeHtml', () => {
         'a<a>b</a>');
     expect(sanitizeHtml('a<a href="JAVASCRIPT:alert">b</a>')).to.be.equal(
         'a<a>b</a>');
+    expect(sanitizeHtml('a<a href="vbscript:alert">b</a>')).to.be.equal(
+        'a<a>b</a>');
+    expect(sanitizeHtml('a<a href="VBSCRIPT:alert">b</a>')).to.be.equal(
+        'a<a>b</a>');
   });
 
   it('should NOT output security-sensitive attributes', () => {

diff --git a/third_party/mustache/mustache.js b/third_party/mustache/mustache.js
@@ -42,7 +42,8 @@
    * including its prototype, has a given property
    */
   function hasProperty (obj, propName) {
-    return obj != null && typeof obj === 'object' && (propName in obj);
+    return obj != null && typeof obj === 'object' &&
+        Object.prototype.hasOwnProperty.call(obj, propName);
   }
 
   // Workaround for https://issues.apache.org/jira/browse/COUCHDB-577
@@ -397,14 +398,21 @@
            * `undefined` and we want to avoid looking up parent contexts.
            **/
           while (value != null && index < names.length) {
+            if (!hasProperty(value, names[index])) {
+              value = null;
+              break;
+            }
             if (index === names.length - 1)
-              lookupHit = hasProperty(value, names[index]);
-
+              lookupHit = true;
             value = value[names[index++]];
           }
         } else {
-          value = context.view[name];
-          lookupHit = hasProperty(context.view, name);
+          if (!hasProperty(context.view, name)) {
+            value = null;
+          } else {
+            value = context.view[name];
+            lookupHit = true;
+          }
         }
 
         if (lookupHit)