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

html/template: clarify, perhaps expand allowed URLs in urlFilter #20586

Closed
erikformella opened this issue Jun 5, 2017 · 15 comments
Closed

html/template: clarify, perhaps expand allowed URLs in urlFilter #20586

erikformella opened this issue Jun 5, 2017 · 15 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@erikformella
Copy link

What version of Go are you using (go version)?

1.8

What operating system and processor architecture are you using (go env)?

darwin
amd64

What did you do?

https://play.golang.org/p/RL6eqxvEfg

What did you expect to see?

<a href="tel:555-555-5555">click me</a>

What did you see instead?

<a href="#ZgotmplZ">click me</a>

The source here only whitelists http, https, and mailto: https://github.com/golang/go/blob/master/src/html/template/url.go#L22

I also found this thread talking about it and asked if I should work on a fix. No response yet.

@bradfitz bradfitz changed the title tel: urls are not whitelisted in template/html template/html: whitelist the "tel" URL scheme Jun 5, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jun 5, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 5, 2017
@robpike robpike changed the title template/html: whitelist the "tel" URL scheme html/template: whitelist the "tel" URL scheme Jun 7, 2017
@rsc
Copy link
Contributor

rsc commented Jun 12, 2017

For reference: https://tools.ietf.org/html/rfc3966.

@ianlancetaylor
Copy link
Contributor

CC @stjj89

@ianlancetaylor
Copy link
Contributor

CC @mikesamuel

@rsc
Copy link
Contributor

rsc commented Jun 26, 2017

Unless @stjj89 or @mikesamuel object, it seems fine to treat tel: like mailto:.

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 26, 2017
@stjj89
Copy link
Contributor

stjj89 commented Jun 26, 2017

Would wrapping the tel URL in template.URL be sufficient? i.e. https://play.golang.org/p/fVJv1ORih2

I would prefer not to whitelist tel, just because those URLs can trigger unexpected actions on behalf of the user of the generated HTML (e.g. initiate a paid phone call). Requiring the developer to explicitly cast such URLs as template.URL gives us a soft guarantee that the developer has considered the implications of allowing this scheme.

@rsc
Copy link
Contributor

rsc commented Jun 27, 2017

I completely understand the escaping in

<a href="https://my.site/search?q={{.}}">

to make that safe regardless of how {{.}} is filled in. In this example, html/template is working with the user to help ensure that the results match what the user intends, namely setting the q parameter.

In contrast I do not understand the sanitizing in

<a href="{{.}}">

Why does "https://evil.site/attack.html" pass through unmodified while "ftp://ftp5.freebsd.org/" and "tel:555-1212" do not? Here it seems like html/template moves from working with the user to second-guessing, passing judgement, or even babysitting.

@rchunping
Copy link

rchunping commented Jun 27, 2017

the same problem

<img src=“{.}” />

data:image/png;base64,........

@mikesamuel
Copy link
Contributor

Why does "https://evil.site/attack.html" pass through unmodified while "ftp://ftp5.freebsd.org/"
and "tel:555-1212" do not? Here it seems like html/template moves from working with the user
to second-guessing, passing judgement, or even babysitting.

The relevant code is https://golang.org/src/html/template/url.go#L22

https://golang.org/pkg/html/template/#hdr-Security_Model explains the reasoning obliquely

This package assumes that template authors are trusted, that Execute's data parameter
is not, and seeks to preserve the properties below in the face of untrusted data:

Code Effect Property: "... only code specified by the template author should run as a result
of injecting the template output into a page and all code specified by the template author
should run as a result of the same."

So we reject javascript:... URLs in data parameters because it is not code specified by the template authors.

Since writing that, we on the security engineering team have come to the opinion that
the Code Effect property should really be an Unexpected Side Effect property.
Samuel was pointing out that there are multiple known exploits related to tel: URLs.

These exploits are widely understood by security researchers, but not by HTML developers, so qualify as an Unexpected Side Effect.
I think device manufacturers are working to fix these problems in webviews, but I don't know how many affected devices are still out there. Once that number falls low enough though, the unexpected side effect argument would no longer apply.

@stjj89
Copy link
Contributor

stjj89 commented Jun 27, 2017

In similar internal URL filters, we adopt a more permissive policy that allows http, https, mailto, ftp schemes, as well as some subset of base64 data:image/ and data:video/ URLs. We could consider making the html/template URL filter similarly permissive.

@xtofian
Copy link

xtofian commented Jun 27, 2017

In general, there's a long history of (often fairly obscure) security-relevant inconsistencies and bugs in specs and implementations of the Web Platform [Zalewski, 2012]. Based on this, we should be inclined to be fairly conservative in the design of frameworks and libraries that interact with the Web Platform.

This is for instance the reason why data: URLs have traditionally not been allowed by URL sanitizers: We know that some data: URLs can result in same-origin script exeuction and hence XSS (data:text/html;...). The syntax and semantics of data: URLs are fairly complex, raising the concern that an apparently safe subset (say, data:image/png;...) might actually include problematic instances (perhaps data:image/png;base64,<invalid base64 encoding> causes some browsers to misbehave). In this particular case, over the past few years, we have gained reasonable confidence in the correctness of current browsers' handling of, say, data:image/* URLs, and have hence started accepting them in various sanitizer implementations (e.g. Closure Templates). It'd make sense to accept this in html/template as well.

As for tel: and sms: URLs: Depending on the situation, the context of the app containing the link may confer some degree of trust, and a user might be tricked into actually dialing a 1900 number that costs them money. True, outbound http://evil.org links can be similarly problematic (and perhaps, in some phishing-related scenarios even worse). This essentially comes down to a tradeoff between being conservative and not being unreasonably restrictive: We can't realistically reject all http(s)?: URLs since they're a fundamental part of the web and essentially everywhere. tel: on the other hand seems to be relatively uncommonly used, and conceivably problematic in some contexts. This suggests that it's reasonable to to not allow tel: by default, and instead mark the URL explicitly safe via template.URL (this also alerts security reviewers to the special consideration) .

Specific concerns around tel: URLs are (as far as I recall) to a large extent due their handling in iOS Safari:

[Zalewski, 2012] Zalewski, Michal. The tangled Web: A guide to securing modern web applications. No Starch Press, 2012.

@rchunping
Copy link

@xtofian add something like .DisableSecurityCheck(true) ?

@xtofian
Copy link

xtofian commented Jun 28, 2017

@rchunping - the canonical way to suppress the default sanitization/validation for a particular value is to wrap it in an appropriate type that indicates to html/template that the value should be considered safe for a given context; in this case, template.URL. See the example that @stjj89 gave above, https://play.golang.org/p/fVJv1ORih2

@rchunping
Copy link

@xtofian thanks ,but the link was broken.

<h1>Unavailable For Legal Reasons</h1><p>If you believe this is in error, please <a href="https://golang.org/issue">file an issue</a>.</p>

@rsc
Copy link
Contributor

rsc commented Jun 28, 2017

Thanks @mikesamuel, this is a good explanation.

@stjj89, can you send a CL expanding the internal comment on func urlFilter to explain a bit more about this, and maybe also think about whether the set there needs to be expanded? We don't have to do that until next cycle, but if you want to send the CL now to get it off your plate, that's fine. Thanks.

@rsc rsc changed the title html/template: whitelist the "tel" URL scheme html/template: clarify, perhaps expand allowed URLs in urlFilter Jun 28, 2017
@gopherbot
Copy link

Change https://golang.org/cl/52853 mentions this issue: html/template: explain URL filtering

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants