Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($sce): handle URLs through the $sce service, plus allow concatenation in that $sce contexts. #14250

Closed
wants to merge 6 commits into from
Closed

feat($sce): handle URLs through the $sce service, plus allow concatenation in that $sce contexts. #14250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3cf8837
feat($sce): handle URLs through the $sce service.
rjamet Mar 16, 2016
9e31d3c
fix(*): Move closer to the old URL sanitization plus misc fixes in te…
rjamet Mar 22, 2016
23a5484
fix(IE): Embrace canonicalization of ng-src specified URLs.
rjamet May 26, 2016
2b5de99
fix(*): handle empty srcsets, and jshint prefers === overall
rjamet May 26, 2016
4e92679
fix(*): Minor fixes addressing gkalpak's comments.
rjamet Aug 30, 2016
1f867be
fix(*): Make this branch up to date.
rjamet Aug 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 23 additions & 44 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,14 +1270,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

/**
* @ngdoc method
* @name $compileProvider#aHrefSanitizationWhitelist
* @name $compileProvider#uriSanitizationWhitelist
* @kind function
*
* @description
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
* urls during a[href] sanitization.
* urls during URL-context sanitization.
*
* The sanitization is a security measure aimed at preventing XSS attacks via html links.
* The sanitization is a security measure aimed at preventing XSS attacks.
*
* Any url about to be assigned to a[href] via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist`
Expand All @@ -1288,45 +1288,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
* chaining otherwise.
*/
this.aHrefSanitizationWhitelist = function(regexp) {
this.uriSanitizationWhitelist = function(regexp) {
if (isDefined(regexp)) {
$$sanitizeUriProvider.aHrefSanitizationWhitelist(regexp);
$$sanitizeUriProvider.uriSanitizationWhitelist(regexp);
return this;
} else {
return $$sanitizeUriProvider.aHrefSanitizationWhitelist();
return $$sanitizeUriProvider.uriSanitizationWhitelist();
}
};


/**
* @ngdoc method
* @name $compileProvider#imgSrcSanitizationWhitelist
* @kind function
*
* @description
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
* urls during img[src] sanitization.
*
* The sanitization is a security measure aimed at prevent XSS attacks via html links.
*
* Any url about to be assigned to img[src] via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `imgSrcSanitizationWhitelist`
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
*
* @param {RegExp=} regexp New regexp to whitelist urls with.
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
* chaining otherwise.
*/
this.imgSrcSanitizationWhitelist = function(regexp) {
if (isDefined(regexp)) {
$$sanitizeUriProvider.imgSrcSanitizationWhitelist(regexp);
return this;
} else {
return $$sanitizeUriProvider.imgSrcSanitizationWhitelist();
}
};

/**
* @ngdoc method
* @name $compileProvider#debugInfoEnabled
Expand Down Expand Up @@ -1447,9 +1418,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

this.$get = [
'$injector', '$interpolate', '$exceptionHandler', '$templateRequest', '$parse',
'$controller', '$rootScope', '$sce', '$animate', '$$sanitizeUri',
'$controller', '$rootScope', '$sce', '$animate',
function($injector, $interpolate, $exceptionHandler, $templateRequest, $parse,
$controller, $rootScope, $sce, $animate, $$sanitizeUri) {
$controller, $rootScope, $sce, $animate) {

var SIMPLE_ATTR_NAME = /^\w/;
var specialAttrHolder = window.document.createElement('div');
Expand Down Expand Up @@ -1629,11 +1600,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

nodeName = nodeName_(this.$$element);

if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
(nodeName === 'img' && key === 'src')) {
// sanitize a[href] and img[src] values
this[key] = value = $$sanitizeUri(value, key === 'src');
} else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) {
// img[srcset] is a bit too weird of a beast to handle through $sce.
// Instead, for now at least, sanitize each of the URIs individually.
// That works even dynamically, but it's not bypassable through the $sce.
// Instead, if you want several unsafe URLs as-is, you should probably
// use trustAsHtml on the whole tag.
if (nodeName === 'img' && key === 'srcset' && value) {

// sanitize img[srcset] values
var result = '';

Expand All @@ -1651,7 +1624,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
for (var i = 0; i < nbrUrisWith2parts; i++) {
var innerIdx = i * 2;
// sanitize the uri
result += $$sanitizeUri(trim(rawUris[innerIdx]), true);
result += $sce.getTrustedUrl(trim(rawUris[innerIdx]));
// add the descriptor
result += (' ' + trim(rawUris[innerIdx + 1]));
}
Expand All @@ -1660,7 +1633,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
var lastTuple = trim(rawUris[i * 2]).split(/\s/);

// sanitize the last uri
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaict, this change means that people using attrs.$set(key, value) will not benefit from sanitization (for <a href/xlink:href or <img src/ng-src) any more. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I assumed users shouldn't use these and instead rely on attributes at the template level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping this: there's actually a lot of uses of attrs.$set that would be relevant, and probably a bunch of vulnerabilities created by this PR. But then, as far as I can tell, URLs are the only things sanitized at this level, all the rest isn't :/

result += $$sanitizeUri(trim(lastTuple[0]), true);
result += $sce.getTrustedUrl(trim(lastTuple[0]));

// and add the last descriptor if any
if (lastTuple.length === 2) {
Expand Down Expand Up @@ -3186,6 +3159,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') {
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
return $sce.RESOURCE_URL;
} else {
return $sce.URL;
}
// maction[xlink:href] can source SVG. It's not limited to <maction>.
} else if (attrNormalizedName === 'xlinkHref' ||
Expand All @@ -3194,6 +3169,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
(tag === 'link' && attrNormalizedName === 'href')
) {
return $sce.RESOURCE_URL;
} else if (tag === 'a' && (attrNormalizedName === 'href' ||
attrNormalizedName === 'xlinkHref' ||
attrNormalizedName === 'ngHref')) {
return $sce.URL;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/ng/directive/attrs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ forEach(['src', 'srcset', 'href'], function(attrName) {
// on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
// then calling element.setAttribute('src', 'foo') doesn't do anything, so we need
// to set the property as well to achieve the desired effect.
// we use attr[attrName] value since $set can sanitize the url.
// we reuse the value put in attr[name] since $set might have sanitized the url.
if (msie && propName) element.prop(propName, attr[name]);
});
}
Expand Down
64 changes: 51 additions & 13 deletions src/ng/interpolate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ function $InterpolateProvider() {
return unwatch;
}

function isConcatenationAllowed(context) {
return context === $sce.URL;
}

/**
* @ngdoc service
* @name $interpolate
Expand Down Expand Up @@ -262,7 +266,9 @@ function $InterpolateProvider() {
textLength = text.length,
exp,
concat = [],
expressionPositions = [];
expressionPositions = [],
singleExpression = false,
contextAllowsConcatenation = isConcatenationAllowed(trustedContext);

while (index < textLength) {
if (((startIndex = text.indexOf(startSymbol, index)) !== -1) &&
Expand All @@ -275,7 +281,7 @@ function $InterpolateProvider() {
parseFns.push($parse(exp, parseStringifyInterceptor));
index = endIndex + endSymbolLength;
expressionPositions.push(concat.length);
concat.push('');
concat.push(''); // Placeholder that will get replaced with the evaluated expression.
} else {
// we did not find an interpolation, so we have to add the remainder to the separators array
if (index !== textLength) {
Expand All @@ -285,27 +291,54 @@ function $InterpolateProvider() {
}
}

if (concat.length === 1 && expressionPositions.length === 1) {
singleExpression = true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the checks at this point and defer them inside compute() instead?
Are we gaining anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability, mostly: see the other comment below.


// Concatenating expressions makes it hard to reason about whether some combination of
// concatenated values are unsafe to use and could easily lead to XSS. By requiring that a
// single expression be used for iframe[src], object[src], etc., we ensure that the value
// that's used is assigned or constructed by some JS code somewhere that is more testable or
// make it obvious that you bound the value to some user controlled value. This helps reduce
// the load when auditing for XSS issues.
if (trustedContext && concat.length > 1) {
$interpolateMinErr.throwNoconcat(text);
}
// single expression be used for some $sce-managed secure contexts (RESOURCE_URLs mostly),
// we ensure that the value that's used is assigned or constructed by some JS code somewhere
// that is more testable or make it obvious that you bound the value to some user controlled
// value. This helps reduce the load when auditing for XSS issues.
// Note that URL-context is dispensed of this, since its getTrusted method can sanitize.
// In that context, .getTrusted will be called on either the single expression or on the
// overall concatenated string (losing trusted types used in the mix, by design). Both these
// methods will sanitize plain strings. Also, HTML could be included, but since it's only used
// in srcdoc attributes, this would not be very useful.

if (!mustHaveExpression || expressions.length) {
var compute = function(values) {
for (var i = 0, ii = expressions.length; i < ii; i++) {
if (allOrNothing && isUndefined(values[i])) return;
concat[expressionPositions[i]] = values[i];
}
return concat.join('');

if (contextAllowsConcatenation) {
if (singleExpression) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be any benefit in having two distinct code paths (that essentially do the same thing).
(Keeping track of singleExpression seems unnecessary as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's mostly for readability: I was struggling when writing this part to understand the data flows and make them clearer, that's why I preferred to split them so I could comment the paths inline. I'm also reusing singleExpression below, in parseStringifyInterceptor (to keep the value of "{{foo}}" from being stringified).

That being said, I'm not that happy with it: if you have suggestions of how to write this better, I'm all ears :/

// The raw value was left as-is by parseStringifyInterceptor
return $sce.getTrusted(trustedContext, concat[0]);
} else {
return $sce.getTrusted(trustedContext, concat.join(''));
}
} else if (trustedContext) {
if (concat.length > 1) {
// there's at least two parts, so expr + string or exp + exp, and this context
// doesn't allow that.
$interpolateMinErr.throwNoconcat(text);
} else {
return concat.join('');
}
} else { // In an unprivileged context, just concatenate and return.
return concat.join('');
}
};

var getValue = function(value) {
return trustedContext ?
// In concatenable contexts, getTrusted comes at the end, to avoid sanitizing individual
// parts of a full URL. We don't care about losing the trustedness here, that's handled in
// parseStringifyInterceptor below.
return (trustedContext && !contextAllowsConcatenation) ?
$sce.getTrusted(trustedContext, value) :
$sce.valueOf(value);
};
Expand Down Expand Up @@ -344,8 +377,13 @@ function $InterpolateProvider() {

function parseStringifyInterceptor(value) {
try {
value = getValue(value);
return allOrNothing && !isDefined(value) ? value : stringify(value);
if (contextAllowsConcatenation && singleExpression) {
// No stringification in this case, to keep the trusted value until unwrapping.
return value;
} else {
value = getValue(value);
return allOrNothing && !isDefined(value) ? value : stringify(value);
}
} catch (err) {
$exceptionHandler($interpolateMinErr.interr(text, err));
}
Expand Down
54 changes: 14 additions & 40 deletions src/ng/sanitizeUri.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,67 +3,41 @@
/**
* @this
* @description
* Private service to sanitize uris for links and images. Used by $compile and $sanitize.
* Private service to sanitize uris for $sce.URL context. Used by $compile, $sce and $sanitize.
*/
function $$SanitizeUriProvider() {
var aHrefSanitizationWhitelist = /^\s*(https?|ftp|mailto|tel|file):/,
imgSrcSanitizationWhitelist = /^\s*((https?|ftp|file|blob):|data:image\/)/;

/**
* @description
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
* urls during a[href] sanitization.
*
* The sanitization is a security measure aimed at prevent XSS attacks via html links.
*
* Any url about to be assigned to a[href] via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist`
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
*
* @param {RegExp=} regexp New regexp to whitelist urls with.
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
* chaining otherwise.
*/
this.aHrefSanitizationWhitelist = function(regexp) {
if (isDefined(regexp)) {
aHrefSanitizationWhitelist = regexp;
return this;
}
return aHrefSanitizationWhitelist;
};
var uriSanitizationWhitelist = /^\s*((https?|ftp|file|blob|tel|mailto):|data:image\/)/i;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wrong there, blob shouldn't be safe for navigation. A link to a blob with user-controlled contents and a dangerous MIME type will run in the origin that created it.



/**
* @description
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
* urls during img[src] sanitization.
* urls.
*
* The sanitization is a security measure aimed at prevent XSS attacks via html links.
* The sanitization is a security measure aimed at prevent XSS attacks.
*
* Any url about to be assigned to img[src] via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `imgSrcSanitizationWhitelist`
* Any url about to be assigned to URL context via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `urlSanitizationWhitelist`
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM,
* making it inactive.
*
* @param {RegExp=} regexp New regexp to whitelist urls with.
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
* chaining otherwise.
*/
this.imgSrcSanitizationWhitelist = function(regexp) {
this.uriSanitizationWhitelist = function(regexp) {
if (isDefined(regexp)) {
imgSrcSanitizationWhitelist = regexp;
uriSanitizationWhitelist = regexp;
return this;
}
return imgSrcSanitizationWhitelist;
return uriSanitizationWhitelist;
};

this.$get = function() {
return function sanitizeUri(uri, isImage) {
var regex = isImage ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
var normalizedVal;
normalizedVal = urlResolve(uri).href;
if (normalizedVal !== '' && !normalizedVal.match(regex)) {
return function sanitizeUri(uri) {
var normalizedVal = urlResolve(uri).href;
if (normalizedVal !== '' && !normalizedVal.match(uriSanitizationWhitelist)) {
return 'unsafe:' + normalizedVal;
}
return uri;
Expand Down
Loading