XSS: href arbitrary code execution

[petermikitsh]

Stencil version:

 @stencil/core@1.7.3

I'm submitting a:

[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://stencil-worldwide.herokuapp.com/ or https://forum.ionicframework.com/

Current behavior:

import { Component, h } from '@stencil/core';

@Component({
  tag: 'my-component'
})
export class MyComponent {
  render() {
    return [
      <a href="javascript:alert('xss')">XSS Link</a>,
      <a href="data:text/html;base64,PHNjcmlwdD5hbGVydCgneHNzJyk8L3NjcmlwdD4=">XSS Link</a>
    ];
  }
}

Expected behavior:

Users should have to use innerHTML for this to work, to prevent the possibility of developers creating an XSS vulnerability.

[cmaas]

Could you explain how innerHTML should work in this scenario, where you have a href attribute and don't set the content of an HTML element?

Because I'm not sure if I agree that this is a problem. There are scenarios where you want to execute raw JS in a href attribute. Furthermore, there are ways to sanitize user-generated content in links.

[dgibson666]

I'm struggling with this too. I have cases where I use javascript in my href rendered by a component, even if it's void(0). InnerHTML itself creates XSS vulnerabilities, so I'm not sure how that is a solution.

[petermikitsh]

innerHTML was just one suggestion: it may not be the best option. I looked at Angular, and saw it prepends unsafe: to unsafe URLs. Stencil isn't a UI Framework, but it is well positioned to prevent this type of XSS vulnerability.

@cmaas Most scenarios where you'd want to execute raw JS from the href attribute could be handled in a more Stencil-y way. For example, you could pass in an onClick function.

For example, this raw JS:

import { Component, h } from '@stencil/core';

window.myAlert = (msg: string) => { 
  window.alert(msg);
}

@Component({
  tag: 'my-component'
})
export class MyComponent {
  render() {
    return [
      <a href="javascript:myAlert('xss')">XSS Link</a>,
    ];
  }
}

Could be written as:

import { Component, h } from '@stencil/core';

window.myAlert = (msg: string) => { 
  window.alert(msg);
}

@Component({
  tag: 'my-component'
})
export class MyComponent {
  render() {
    return [
      <a onClick={() => { window.myAlert('noxss'); }}>Safe from XSS Link</a>,
    ];
  }
}

The problem is allowing untrusted user input for the href. A bad actor could craft malicious code that steals cookies, etc., when users click the link.

[dgibson666]

Not really. An anchor tag without an href is no longer an interactive element and won't be an accessible link.

I don't disagree with preventing javascript: from being passed in as a prop but am not seeing the problem with rendering an href on a link from within a component. It looks like, from you last comment, that you mean user-defined but your initial example doesn't have the href value as a prop, which makes the report a little more confusing.

[petermikitsh]

An anchor tag without an href is no longer an interactive element and won't be an accessible link.

If you want to execute code in response to a user click, you never wanted an anchor tag in the first place. For accessibility purposes, button is the appropriate element to use.

import { Component, h } from '@stencil/core';

window.myAlert = (msg: string) => { 
  window.alert(msg);
}

@Component({
  tag: 'my-component'
})
export class MyComponent {
  render() {
    return [
      <button onClick={() => { window.myAlert('noxss'); }}>Use buttons to execute code; not links</button>,
    ];
  }
}

The purpose of my prior example was to communicate that you can use events instead of javascript: href's to run code in a more safe manner. I wasn't discussing the accessibility aspect of it, as it's less pertinent to this particular issue, although I do agree with that point.

I don't disagree with preventing javascript: from being passed in as a prop but am not seeing the problem with rendering an href on a link from within a component... you mean user-defined but your initial example doesn't have the href value as a prop, which makes the report a little more confusing.

I didn't use a @Prop decorator to keep the example simple, but here's an example of that.

import { Component, h } from '@stencil/core';

@Component({
  tag: 'my-component'
})
export class MyComponent {
  @Prop() link: string;

  render() {
    return [
      <a href={this.link}>(Could be) an XSS Link</a>
    ];
  }
}

Usage example:

<!--XSS Vulnerability -->
<my-component link="javascript:alert('xss')"></my-component>

If your link property is coming from untrusted input (perhaps from a REST API returning data that could be modified by a bad actor), there is a vulnerability.

[cmaas]

If your link property is coming from untrusted input (perhaps from a REST API returning data that could be modified by a bad actor), there is a vulnerability.

Now the question is what Stencil's role should be here. Should input like this be sanitized by default by Stencil? Or is it the task of the developer? I have a tendency for the latter, because as you said, Stencil is not a framework.

[cmaas]

Another thought: <a href="javascript:alert(1)"> is totally valid and I kind of expect Stencil to be as close as possible to what you can do in vanilla JS/HTML. Should Stencil really be concerned about where that link prop comes from and whether or not this could be potentially exploited?

[dgibson666]

Yeah, there are valid use cases to execute JS when clicking a link. I don't think Stencil should be trying to sanitize and limit what we can do.

As I said, innerHTML is prone to XSS as well. I grabbed sanitizeDOMString.ts from Ionic's project to see how they were sanitizing. Their case doesn't work for mine because they are whitelisting only a handful of standard HTML attributes, but I need to include my own custom elements via innerHTML in a few cases. This would not be possible if Stencil was doing the sanitization, or they'd need a bypass. In the end, it should be the developer's job.

[petermikitsh]

@cmaas Keep in mind that Stencil already does a lot of sanitization for you, specifically with the intent of preventing other classes of XSS vulnerabilities. Take this example:

import { Component, Prop, h } from "@stencil/core";

@Component({
  tag: "my-component",
})
export class MyComponent {
  @Prop() content: string;

  render() {
    return <div>{this.content}</div>;
  }
}

<my-component content="<script>alert('xss')</script>"></my-component>

Do you expect the script tag to execute? <script>alert('xss')</script> is totally valid HTML; however, Stencil sanitizes the string, encoding it, preventing it from executing as code.

Should this.content this be sanitized by default? Your expectation is as close to vanilla JS/HTML as possible. Stencil is not a framework. Yet, currently it sanitizes. If Stencil did no sanitization today, it'd be clearly out-of-scope. So, I think it's a gray matter, whether Stencil should or should not sanitize in the case of href's.

@dgibson666 I'd be curious to hear more on the use case where developers intend to use javascript: in anchor tag href's. As previously stated, you can add click handlers to anchor tags. For what use case would using a click handler be problematic, and why?

[cmaas]

@petermikitsh Hard to say, because I just know that all JSX is escaped by default. So in your example, I'd expect that it does NOT execute, because a raw HTML string is treated as a string in JSX. Setting raw HTML only works through innerHTML.

With props, I think I'd expect that they are passed "as is" and not escaped. But I don't care enough to be very strict about it.

I'd be curious to hear more on the use case where developers intend to use javascript: in anchor tag href's

Backwards compatibility to some libs that get included in Stencil? Not sure about any use cases. One thing though that has nothing to do with Stencil: I used a bunch of graph libraries in the past and the way most of them assigned labels to the x- and y-axis was through innerText. But I needed links as labels and this is only possible with innerHTML. So the assumption of the devs of these libraries that labels = text only was quite limiting and I ended up modifying the source of these libs to be usable.

I know, it's just a little anecdote, but it shows that "always escape" isn't always the best thing (especially if there is no API to use some "raw" functionality). I guess this is different with Stencil, since you can find other ways to assign raw attributes. I wonder if it makes sense to have two attributes for a tags: href and unsafeHref or whatever. Just don't escape by default with NO other way to set the raw thing.

[christian-bromann]

Hey folks 👋

We apologize for the delay in addressing this issue. The StencilJS team has been hard at work, and we appreciate your patience.

As StencilJS has undergone significant updates and improvements since this issue was originally posted, we believe that this specific concern may no longer be relevant with the latest version. Therefore, we are closing this issue.

If you are still experiencing this problem or have any new issues, please feel free to open a new issue with the latest details, and we will be happy to assist you.

Thank you for your understanding and support of StencilJS!