Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensible Sanitizer #36650

Open
Vandivier opened this issue Apr 15, 2020 · 7 comments
Open

Extensible Sanitizer #36650

Vandivier opened this issue Apr 15, 2020 · 7 comments
Labels
area: core Issues related to the framework runtime core: sanitization feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Milestone

Comments

@Vandivier
Copy link

Vandivier commented Apr 15, 2020

🚀 feature request

Relevant Package

@angular/core

this line and file are of key interest:

export const VALID_ATTRS = merge(URI_ATTRS, SRCSET_ATTRS, HTML_ATTRS, ARIA_ATTRS);

Description

The ability to trust a custom whitelist of HTML tags, attributes, and values with a DOM sanitizer, without bypassing the whole sanitizer.

This would allow preservation of id, style, data attributes, and other common attributes. These attributes are useful in a variety of use cases for many Angular developers. This issue is a common cause of many GitHub issues, StackOverflow questions, and other indicators that there is a real problem with demand for a secure and flexible solution.

Related SO 1
Related SO 2
Tutorial on a best practice approach to doing what I am requesting
this would close a bunch of github issues and resolve many SO questions.

Describe the solution you'd like

I would like to be able to specify three things to allow keys and values to be sanitized in a SecurityContext.HTML.

This could be implemented as an options argument to DomSanitizer.sanitize, or it could be implemented as a custom SecurityContext, but I will use options object in my description below:

    trustedFirstParagraph$ = this.translateService
        .translate('some-translation-key')
        .pipe(map(s => this.domSanitizer.sanitize(SecurityContext.HTML, s, options)));

Here, options can be an object with any of three keys:

const options = {
  trustAttributeKeyExpression: /some-regex-to-trust/,
  trustAttributeValueExpression: /some-value-to-trust/,
  trustAttribute: (el, key, value): boolean | string[] => {
      // arbitrary logic that can return a boolean
      // or [string, string] sanitized/transformed [key, value].
  }
}

trustAttributeKeyExpression and trustAttributeValueExpression must both match if both are specified. To implement an OR operation, and other more complex algorithms, a developer can use trustAttribute.

Describe alternatives you've considered

  1. hiding data inside the class attribute and parsing it
  2. using a span or div inside my element which is visually hidden but which I can access through my component dom ref and parse out the value
  3. de-DRYing my code and having component-specific content across n components instead of being able to leverage a generic service
@gkgeorgiev
Copy link

Just about to request the same feature! My use-case is the callto:// URL prefix which I would like to whitelist.

@RichardMcSorley
Copy link

Agreed this would be a much need feature. I'm unable to use data attributes for my markdown to HTML project. So without this I'm blocked.

@angular-robot angular-robot bot added the feature: under consideration Feature request for which voting has completed and the request is now under consideration label Jun 4, 2021
@Ruud-cb
Copy link

Ruud-cb commented Aug 4, 2021

Please put this a bit higher on the to-do list, top-listed stackoverflow questions suggest using bypassSecurityTrustHtml as a solution! Thus leaving a lot of applications open for XSS attacks...

@cloakedninjas
Copy link

Came here looking for the same thing - but out of curiosity, what security concerns arise when id attributes are left in? Was this an oversight or a deliberate decision ?

@iwantwin
Copy link

Came here looking for the same thing - but out of curiosity, what security concerns arise when id attributes are left in? Was this an oversight or a deliberate decision ?

When your code looks for a specific ID on the page and executes code based on what it found, and you have un-santized user-input higher up in the page, you can "steal" the element with the ID. Your code will look for the ID, and ID is supposed to be unique so the DOM will grab the first occurence, which is now controlled by the user.

As long as you are just changing some classes or something this is probably not vulnerable. But if the content or attributes of the associated element are used this could pose major threats. Or if a modal is shown to the user based on that ID? Could be used for showing a fake login screen on-site and sending the credentials to a culprits database.

@iwantwin
Copy link

I'm going to keep a close eye on this issue, as I think it would be great for Angular to have this functionality built in. Espescially for registered elements (customElements.define) would be great if we could "accept" those automatically.

In the meantime I've been using https://www.npmjs.com/package/sanitize-html which I wanted to share here. Highly customizable package that has been very reliable.

@Vandivier
Copy link
Author

Vandivier commented Apr 22, 2022

When your code looks for a specific ID on the page and executes code based on what it found, and you have un-santized user-input higher up in the page, you can "steal" the element with the ID.

In my opinion this is neither a differentiator of the ID nor a security risk. It presumes the attacker is aware of the special ID and its use case. The attacker might just as well be aware of an xpath or querySelector value that captures a special element. Indeed, ID seems to be de-risked in the sense that the attacker can only capture a single element using this pattern, where they might capture whole collections using a special querySelector.

In my view, element identification itself is not a security risk. It's the part you mention later; an attacker hijacking a user-controlled element, which is a very distinct and much more important concern for anyone familiar with web security. Another more important concern would be having to do with injection: Why was the attacker able to run an element identification logic in the first place? These important concerns are not a unique to the ID attribute and not solved by disabling ID sanitization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime core: sanitization feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests

8 participants