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

fix($compile): secure form[action] & iframe[srcdoc] #4933

Closed
wants to merge 1 commit into from
Closed

fix($compile): secure form[action] & iframe[srcdoc] #4933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1733,10 +1733,15 @@ function $CompileProvider($provide) {


function getTrustedContext(node, attrNormalizedName) {
if (attrNormalizedName == "srcdoc") {
Copy link
Contributor

Choose a reason for hiding this comment

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

unless we are sure that we have an exhaustive list of sensitive html attributes I wouldn't even bother with this fix. the purpose of this check is to guide developers not to use data-binding with onclick and similar events and not to prevent attackers that can exploit server-side security holes from using data-binding as an attack vector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are actually two separate checks we already do:

  1. Disallow interpolation for onclick, formaction, and
    friends.  These attribs are always Javascript and we
    have better alternatives.  This fix is not about
    that.
  2. Require $sce.getTrustedResourceUrl() for
    iframe[src], etc.  This PR is about this point.

[code.google.com/p/mustache-security][] has nice things to
say about the security of 1.2.  There are only 2 points
called out – of which only the first is significant and this
PR addresses it.

I agree with you that this isn't exhaustive.

That said, we aren't missing much and an affected app would
have to actively be using an attribute we've
missed.  Merging this PR is keeping in the spirit of what
we're already doing to *[src] and xlink-href – otherwise
we should remove those checks and just leave it to the
developer.

return $sce.HTML;
}
var tag = nodeName_(node);
// maction[xlink:href] can source SVG. It's not limited to <maction>.
if (attrNormalizedName == "xlinkHref" ||
(nodeName_(node) != "IMG" && (attrNormalizedName == "src" ||
attrNormalizedName == "ngSrc"))) {
(tag == "FORM" && attrNormalizedName == "action") ||
(tag != "IMG" && (attrNormalizedName == "src" ||
attrNormalizedName == "ngSrc"))) {
return $sce.RESOURCE_URL;
}
}
Expand Down
70 changes: 70 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4016,6 +4016,76 @@ describe('$compile', function() {
}));
});

describe('form[action]', function() {
it('should pass through action attribute for the same domain', inject(function($compile, $rootScope, $sce) {
element = $compile('<form action="{{testUrl}}"></form>')($rootScope);
$rootScope.testUrl = "different_page";
$rootScope.$apply();
expect(element.attr('action')).toEqual('different_page');
}));

it('should clear out action attribute for a different domain', inject(function($compile, $rootScope, $sce) {
element = $compile('<form action="{{testUrl}}"></form>')($rootScope);
$rootScope.testUrl = "http://a.different.domain.example.com";
expect(function() { $rootScope.$apply() }).toThrowMinErr(
"$interpolate", "interr", "Can't interpolate: {{testUrl}}\nError: [$sce:insecurl] Blocked " +
"loading resource from url not allowed by $sceDelegate policy. URL: " +
"http://a.different.domain.example.com");
}));

it('should clear out JS action attribute', inject(function($compile, $rootScope, $sce) {
element = $compile('<form action="{{testUrl}}"></form>')($rootScope);
$rootScope.testUrl = "javascript:alert(1);";
expect(function() { $rootScope.$apply() }).toThrowMinErr(
"$interpolate", "interr", "Can't interpolate: {{testUrl}}\nError: [$sce:insecurl] Blocked " +
"loading resource from url not allowed by $sceDelegate policy. URL: " +
"javascript:alert(1);");
}));

it('should clear out non-resource_url action attribute', inject(function($compile, $rootScope, $sce) {
element = $compile('<form action="{{testUrl}}"></form>')($rootScope);
$rootScope.testUrl = $sce.trustAsUrl("javascript:doTrustedStuff()");
expect($rootScope.$apply).toThrowMinErr(
"$interpolate", "interr", "Can't interpolate: {{testUrl}}\nError: [$sce:insecurl] Blocked " +
"loading resource from url not allowed by $sceDelegate policy. URL: javascript:doTrustedStuff()");
}));

it('should pass through $sce.trustAs() values in action attribute', inject(function($compile, $rootScope, $sce) {
element = $compile('<form action="{{testUrl}}"></form>')($rootScope);
$rootScope.testUrl = $sce.trustAsResourceUrl("javascript:doTrustedStuff()");
$rootScope.$apply();

expect(element.attr('action')).toEqual('javascript:doTrustedStuff()');
}));
});

if (!msie || msie >= 11) {
describe('iframe[srcdoc]', function() {
it('should NOT set iframe contents for untrusted values', inject(function($compile, $rootScope, $sce) {
element = $compile('<iframe srcdoc="{{html}}"></iframe>')($rootScope);
$rootScope.html = '<div onclick="">hello</div>';
expect(function() { $rootScope.$digest(); }).toThrowMinErr('$interpolate', 'interr', new RegExp(
/Can't interpolate: {{html}}\n/.source +
/[^[]*\[\$sce:unsafe\] Attempting to use an unsafe value in a safe context./.source));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error message should read ".... in sensitive context" not "safe context"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged.  Changing this requires also changing about 10 other places (in sce.js and sceSpecs.js) so let's do that in a separate commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

}));

it('should NOT set html for wrongly typed values', inject(function($rootScope, $compile, $sce) {
element = $compile('<iframe srcdoc="{{html}}"></iframe>')($rootScope);
$rootScope.html = $sce.trustAsCss('<div onclick="">hello</div>');
expect(function() { $rootScope.$digest(); }).toThrowMinErr('$interpolate', 'interr', new RegExp(
/Can't interpolate: {{html}}\n/.source +
/[^[]*\[\$sce:unsafe\] Attempting to use an unsafe value in a safe context./.source));
}));

it('should set html for trusted values', inject(function($rootScope, $compile, $sce) {
element = $compile('<iframe srcdoc="{{html}}"></iframe>')($rootScope);
$rootScope.html = $sce.trustAsHtml('<div onclick="">hello</div>');
$rootScope.$digest();
expect(angular.lowercase(element[0].srcdoc)).toEqual('<div onclick="">hello</div>');
}));
});
}

describe('ngAttr* attribute binding', function() {

it('should bind after digest but not before', inject(function($compile, $rootScope) {
Expand Down