Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

Commit

Permalink
Proactively drop tags from non-HTML namespaces. The sanitizer will al…
Browse files Browse the repository at this point in the history
…ways force the namespace on output nodes to be HTML, and accepting these nodes can be abused by leveraging this XML->HTML conversion (e.g. <math><style> becomes <span><style>, and STYLE went from a MathML unknown tag to a known HTML tag with special behavior) to bypass sanitization.

RELNOTES: N/A

PiperOrigin-RevId: 389081193
Change-Id: Ie5ea80b7e67dacc454c9506bef84c8e5150c6043
  • Loading branch information
rpelizzi authored and copybara-github committed Aug 6, 2021
1 parent bb72608 commit b861734
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 11 deletions.
19 changes: 17 additions & 2 deletions closure/goog/html/sanitizer/htmlsanitizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ goog.html.sanitizer.HTML_SANITIZER_INVALID_CUSTOM_TAGS_ = {
goog.html.sanitizer.RANDOM_CONTAINER_ = '*';


/**
* The only supported namespace. We drop tags outside of this namespace.
* @private @const {string}
*/
goog.html.sanitizer.XHTML_NAMESPACE_URI_ = 'http://www.w3.org/1999/xhtml';


/**
* Creates an HTML sanitizer.
Expand Down Expand Up @@ -1220,20 +1226,29 @@ goog.html.sanitizer.HtmlSanitizer.prototype.createTextNode = function(
goog.html.sanitizer.HtmlSanitizer.prototype.createElementWithoutAttributes =
function(dirtyElement) {
'use strict';
var dirtyName =
const dirtyName =
goog.html.sanitizer.noclobber.getNodeName(dirtyElement).toUpperCase();
if (dirtyName in this.tagBlacklist_) {
// If it's blacklisted, completely remove the tag and its descendants.
return null;
}
const dirtyNamespaceURI =
goog.html.sanitizer.noclobber.getElementNamespaceURI(dirtyElement);
if (dirtyNamespaceURI != goog.html.sanitizer.XHTML_NAMESPACE_URI_) {
// We explicitly drop tags (and their descendants) in non-html
// namespaces because these can be exploited during their conversion to the
// html namespace (e.g. <MATH><STYLE><A> -> <SPAN><STYLE><A>, where STYLE
// and A were MathML tags before sanitization and HTML tags afterwards.
return null;
}
if (this.tagWhitelist_[dirtyName]) {
// If it's whitelisted, keep as is.
return document.createElement(dirtyName);
}
// If it's neither blacklisted nor whitelisted, replace with span. If the
// relevant builder option is enabled, the tag will bear the original tag
// name in a data attribute.
var spanElement = goog.dom.createElement(goog.dom.TagName.SPAN);
const spanElement = goog.dom.createElement(goog.dom.TagName.SPAN);
if (this.shouldAddOriginalTagNames_) {
goog.html.sanitizer.noclobber.setElementAttribute(
spanElement, goog.html.sanitizer.HTML_SANITIZER_SANITIZED_ATTR_NAME_,
Expand Down
18 changes: 18 additions & 0 deletions closure/goog/html/sanitizer/htmlsanitizer_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1589,4 +1589,22 @@ testSuite({
input, input,
new Builder().allowCssStyles().inlineStyleRules().build());
},

testMathStyleXSS() {
const input = '<math><style><a>{} *{background: red url(https://wherever)}';
const output = '';
assertSanitizedHtml(
input, output,
new Builder().allowStyleTag().withStyleContainer().build());
},

testMathStyleXSS_withoutMath() {
// Just checking that there are no issues without the use of MATH.
const input = '<span><style><a>{} *{background-url: url(https://wherever)}';
const output = '<span><style>*{}</style></span>';
assertSanitizedHtml(
input, output,
new Builder().allowStyleTag().withStyleContainer().build());
},

});
21 changes: 20 additions & 1 deletion closure/goog/html/sanitizer/noclobber.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ var Methods = {
SHEET_GETTER: getterOrNull('HTMLStyleElement', 'sheet'),
GET_PROPERTY_VALUE:
prototypeMethodOrNull('CSSStyleDeclaration', 'getPropertyValue'),
SET_PROPERTY: prototypeMethodOrNull('CSSStyleDeclaration', 'setProperty')
SET_PROPERTY: prototypeMethodOrNull('CSSStyleDeclaration', 'setProperty'),
NAMESPACE_URI_GETTER: getterOrNull('Element', 'namespaceURI') ||
// Edge and IE10 define this Element property on Node instead of
// Element.
getterOrNull('Node', 'namespaceURI'),
};

/**
Expand Down Expand Up @@ -433,6 +437,20 @@ function setCssProperty(cssStyle, propName, sanitizedValue) {
[propName, sanitizedValue]);
}

/**
* Returns an element's namespace URI without falling prey to things like
* <form><input name="namespaceURI"></form>.
* @param {!Element} element
* @return {string}
*/
function getElementNamespaceURI(element) {
return genericPropertyGet(
Methods.NAMESPACE_URI_GETTER, element, 'namespaceURI',
function(namespaceURI) {
return typeof namespaceURI == 'string';
});
}

exports = {
getElementAttributes: getElementAttributes,
hasElementAttribute: hasElementAttribute,
Expand All @@ -453,6 +471,7 @@ exports = {
appendNodeChild: appendNodeChild,
getCssPropertyValue: getCssPropertyValue,
setCssProperty: setCssProperty,
getElementNamespaceURI: getElementNamespaceURI,
/** @package */
Methods: Methods,
};
5 changes: 5 additions & 0 deletions closure/goog/html/sanitizer/noclobber_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ testSuite({
element = createElement('matches');
assertTrue(noclobber.elementMatches(element, '#foo'));
assertFalse(noclobber.elementMatches(element, '#bar'));

element = createElement('namespaceURI');
assertEquals(
'http://www.w3.org/1999/xhtml',
noclobber.getElementNamespaceURI(element));
},

/**
Expand Down
13 changes: 5 additions & 8 deletions closure/goog/html/sanitizer/unsafe_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,11 @@ testSuite({
},

testAlsoAllowTagsInBlacklist() {
// Simplified use case taken from KaTex output HTML. The real configuration
// would allow more attributes and apply a stricter policy on their values
// to reduce the attack surface.
const input = '<svg width="1px"><line x1="3" /><path d="M 10 30" /></svg>';
assertSanitizedHtml(input, input, ['svg', 'line', 'path'], [
{tagName: 'svg', attributeName: 'width', policy: functions.identity},
{tagName: 'line', attributeName: 'x1', policy: functions.identity},
{tagName: 'path', attributeName: 'd', policy: functions.identity},
const input = '<video controls><source src="video.mp4" type="video/mp4">';
assertSanitizedHtml(input, input, ['video', 'source'], [
{tagName: 'video', attributeName: 'controls', policy: functions.identity},
{tagName: 'source', attributeName: 'src', policy: functions.identity},
{tagName: 'source', attributeName: 'type', policy: functions.identity},
]);
},
});

0 comments on commit b861734

Please sign in to comment.