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

Feature request: Introduce context-aware escaping #6

Closed
a-h opened this issue May 20, 2021 · 5 comments
Closed

Feature request: Introduce context-aware escaping #6

a-h opened this issue May 20, 2021 · 5 comments

Comments

@a-h
Copy link
Owner

a-h commented May 20, 2021

templ currently uses simple escaping rules, described below:

{% templ Example() %}
<script type="text/javascript">
  {%= "will be escaped using templ.Escape, which isn't JavaScript-aware" %}
</script>
<div onClick={%= "will be escaped using templ.Escape, which isn't JavaScript aware" %}>
  {%= "will be escaped using templ.Escape, which is fine" %}</div>  
</div>
<style type="text/css">
  {%= "will be escaped using templ.Escape, which isn't CSS-aware" %}
</style>
<div style={%= "will be escaped using templ.Escape, which isn't CSS-aware" %}</div>
<div>{%= "will be escaped using templ.Escape, which is fine" %}</div>
<a href={%= "will be escaped using templ.Escape, which isn't hyperlink-aware" %}</div>
{% endtempl %}

This approach is safe when the content being applied to the onClick / on* / style, attributes and script / style element contents are from trusted sources, but if not, they could become attack vectors.

In most cases, developers won't be adding hyperlinks, or populating onClick elements from user-controlled content, but since it's a potential attack vector, it's probably worth making developers explicitly state the safety, despite the extra developer hassle.

templ's element type should be aware of href, on* and style attributes and the contents of script and style tags, and automatically apply context-aware escaping improve the security posture of templ.

In cases where developers need to use content (css, scripts, hyperlinks) from data that's under their control (i.e. not potentially under an attacker's control), they'll need to state that the content is "safe".

Language changes

Option 1 - New string expression operator

This could be done by adding a new context unaware string expression operator that only uses the templ.Escape function, rather than context-aware escaping. The syntax could be:

  • {%!=
  • {%= safe

Option 2 - Force a function call to safehtml

Make the CSS, onClick etc. take appropriate types from https://github.com/google/safehtml and force users to use safehtml.StyleFromConstant method calls.

I think this would be irritating, because it's such a long function call.

Option 3 - Wrap the function call to safehtml

Similar, to above, but create a github.com/a-h/templ/safe package and shorten the calls:

  • {%= safe.Style("background-color: red") %}
  • {%= safe.Script("alert('hello')") %}

Decision

I don't like adding new templ expressions unless really required, so while I like option 1 (particularly the {%!= operator), I think option 3 is the way to go, because it's very clear what's going on even if you've never seen templ before, and there's not many places where developers will actually need to do this (onClick handlers, contents of <script> tags, style attributes).

While it's potentially a breaking change for some users, we're not stable, so I think it's a good change.

@a-h
Copy link
Owner Author

a-h commented Jun 5, 2021

URL sanitisation design

Sanitisation only applies to Go expressions, it wouldn't apply to constant attribute values like <a href="javascript: alert('hi!')"> because that's a compile time constant.

Users will need to migrate from using string expressions:

<a href={%= variable %}>

To using a new templ.URL function that applies URL sanitisation, returning a templ.SafeURL type with sanitisation rules applied.

<a href={%= templ.URL(variable) %}>

To bypass sanitisation, it's possible to convert a string directly to the templ.SafeURL type:

<a href={%= templ.SafeURL(s) %}>

This allows for compile time checks, because the Generator will be updated to generate specific code for the href attribute that expects a templ.SafeURL to be returned by the expression.

Notes

The sanitisation will be to check that URLs in variables are relative, or are an approved protocol.

Go's built-in templating language just checks that the URL is relative, or is approved protocol (http, https, mailto): https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/html/template/url.go#L48

There are some useful tests that describe the sanitisation behaviour at https://golang.org/src/html/template/escape_test.go

Google's safehtml package allows http, https, mailto and ftp, but also data URLs for specific MIME types:
https://github.com/google/safehtml/blob/85ae69c35a0990a1f40f265f0788e0a75be771a1/url.go#L114

I'll do Go's set as a first pass, and might add in support for data URLs later.

Bypass notes

Google's SafeHTML package doesn't allow this sort of bypass, preferring to force a safe API surface, but Angular.js / Hugo etc. provide something similar to a trustAsUrl function https://docs.angularjs.org/api/ng/service/$sce#trustAsUrl which bypasses sanitisation.

Security reviewers can check for the presence of templ.SafeURL in the *.templ files, and ban it via static analysis checks if required, since its only purpose is to skip the approved protocol checks. A check could be added to the templ command tool to enforce this in CI/CD as a future feature.

Example design

package main

import (
	"fmt"
	"strings"
)

func main() {
	safe := URL("/safe")
	unsafe := URL("javascript:alert('hi')")
	bypassedChecks := SafeURL("javascript:alert('used SafeURL to bypass sanitization')")
	fmt.Println(safe)
	fmt.Println(unsafe)
	fmt.Println(bypassedChecks)
}

// I'm British, and use an 's' in 'sanitisation', but I use US English in code to match the Go programming language.
const FailedSanitizationURL = SafeURL("about:invalid#TemplFailedSanitizationURL")

func URL(s string) SafeURL {
	//TODO: Actual checks.
	if strings.HasPrefix(s, "javascript:") {
		return FailedSanitizationURL
	}
	return SafeURL(s)
}

type SafeURL string

a-h added a commit that referenced this issue Jun 7, 2021
a-h added a commit that referenced this issue Jun 12, 2021
@a-h
Copy link
Owner Author

a-h commented Jun 12, 2021

Added support for URL sanitisation in: b7cfe4b

I've also added a new CSS capability that applies CSS value sanitisation to all expression-generated values by default (with no option to override, because the sanitisation is sensible).

I don't think it's a good idea to use a templating language to insert variables or expressions into JavaScript or CSS. It would be good to find out more about why people might be doing that and provide alternative examples.

Next steps are to:

  • Update the generator to ensure that onClick/on* attributes require a templ.SafeScript instead of a bare string. Using templ.SafeScript(variableOrFunction) means that the developer is telling templ to bypass sanitisation, in the same way that the templ.SafeURL does.
  • Update style expression attributes to generate code that expects the Go expression to return a templ.Style instead of a string that's similar to templ.Classes that takes in an array of SafeCSS elements.
  • Update the contents of style tags to only allow Go expressions that returntempl.SafeCSS instead of strings.
  • Update the contents of script tags to only allow Go expressions that return templ.SafeScript instead of strings.

@a-h
Copy link
Owner Author

a-h commented Oct 2, 2021

As a note, React currently allows unsafe strings in href atttributes, so I think templ's behaviour is better than what's commonplace out there for href attributes.

https://jsfiddle.net/adrianhesketh/j2bm149h/1/

This React code renders a link that results in an alert("xss"), with a warning in the console that this behaviour will be changed in a future release.

const Component = ({ userProvidedUrl }) => (
  <div>
    <h3>Secure</h3>
    <a href="https://google.com">Google</a>
    <h3>Insecure</h3>
    <a href={ userProvidedUrl }>Google</a>
  </div>
);

const host = document.querySelector("#app");

const userProvidedUrl = `javascript:alert("xss")`;
ReactDOM.render(<Component userProvidedUrl={ userProvidedUrl }/>, host);
["Warning: A future version of React will block javascript: URLs as a security precaution. Use event handlers instead if you can. If you need to generate unsafe HTML try using dangerouslySetInnerHTML instead. React was passed %s.%s", "\&quot;javascript:alert(\\&quot;xss\\&quot;)\&quot;", "
    at a
    at div
    at Component (&lt;anonymous&gt;:4:22)"]

@a-h
Copy link
Owner Author

a-h commented Oct 2, 2021

CSS behaviour in React

Unsafe CSS expressions an unsafe are automatically removed, so this code:

const Component = ({ width, backgroundColor }) => (
  <div>
    <div style={{ width: width, backgroundColor: backgroundColor }}>Google</div>
  </div>
);

const host = document.querySelector("#app");

const width = `expression(alert('1337'))`;
const red = "#ff0000";
ReactDOM.render(<Component width={width} backgroundColor={red}/>, host);

Will strip the width CSS property because it contains a dangerous value.

Notes on onClick / on* behaviour in React

React will complain with this code:

const Component = ({ onClick }) => (
  <div>
    <div onClick={ onClick }>Google</div>
  </div>
);

const host = document.querySelector("#app");

const onClick = `alert("css")`;
ReactDOM.render(<Component onClick={onClick} />, host);

The result is:

["Warning: Expected `%s` listener to be a function, instead got a value of `%s` type.%s", "onClick", "string", "
    at div
    at div
    at Component (&lt;anonymous&gt;:4:22)"]

@a-h
Copy link
Owner Author

a-h commented Oct 4, 2021

I'm going to close this in favour of smaller tickets now that I've got an idea of what I want to do.

For script related items:

  • Update the generator to ensure that onClick/on* attributes require a templ.SafeScript instead of a bare string. Using templ.SafeScript(variableOrFunction) means that the developer is telling templ to bypass sanitisation, in the same way that the templ.SafeURL does.
  • Update the contents of script tags to only allow Go expressions that return templ.SafeScript instead of strings.

See #23

  • Update style expression attributes to generate code that expects the Go expression to return a templ.Style instead of a string that's similar to templ.Classes that takes in an array of SafeCSS elements.
  • Update the contents of style tags to only allow Go expressions that returntempl.SafeCSS instead of strings.

See #24

@a-h a-h closed this as completed Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant