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: suggest to add HTMLEscape for / to &#x2F #14033

Closed
iamzhout opened this issue Jan 20, 2016 · 8 comments
Closed

html/template: suggest to add HTMLEscape for / to &#x2F #14033

iamzhout opened this issue Jan 20, 2016 · 8 comments

Comments

@iamzhout
Copy link

Is it possible to enrich the html Escape by adding / --> &#x2F conversion?

code: text/template/funcs.go - HTMLEscape()

currently there are " ' & < > totally 5 characters escaped

@bradfitz
Copy link
Contributor

/cc @adg @robpike

@iamzhout iamzhout changed the title html/template: suggest to add HTMLEscape for ' to &#x27 html/template: suggest to add HTMLEscape for / to &#x2F Jan 20, 2016
@iamzhout
Copy link
Author

refer to owasp 2.2 RULE #1

In addition to the 5 characters significant in XML (&, <, >, ", '), 
the forward slash is included as it helps to end an HTML entity.

@mikesamuel
Copy link
Contributor

/ is technically a meta-character in HTML, but it's not necessary to escape it as long as you're escaping < and &.

If you're really worried about someone sneaking a closing tag through without <, escape U+226E which denormalizes to < followed by a / and the full-width U+FF1C and U+FE64.

@adg
Copy link
Contributor

adg commented Jan 21, 2016

@mikesamuel do you think it is worth doing, though?

@bradfitz bradfitz added this to the Unplanned milestone Jan 21, 2016
@mikesamuel
Copy link
Contributor

I'm trying to get enough credentials with my OWASP login to get the blame for that file so I can ask the author about their reasoning, but given what I know now, I wouldn't.

There's no harm in doing so except file size.

I can imagine inputs that have many URL attributes might blow up but not in general.

I did a quick char count on some benchmark HTML files.
4285 bytes are '/' not following a '<' out of 192564 including those in it for about 2.2%.

> cat /tmp/benchmark.html | perl -pe 's/<\/|[^\/]//g' | wc -c

If that file is representative then replacing with &#57; might lead to an 8% increase in sanitized file size pre-gzip, or 10% if you use &#x2f; as in the OP.

To test post-gzip I did

~ [16:10:39]> gzip -c /tmp/benchmark.html | wc -c
   45283
~ [16:10:53]> cat /tmp/benchmark.html | perl -pe 's/(?<![<])\//&#57;/g' | gzip -c | wc -c
   45866

which looks like a 1.3% size increase or 1.1% with gzip -9.

@iamzhout
Copy link
Author

Actually I just noticed the rule in OWASP, and submit this suggestion to add escape for /.
I agree with @mikesamuel, if all think this will not introduce much security enhancement but performance reduction, it's better not to add it.

@mikesamuel
Copy link
Contributor

I traded emails with Jeff Williams. He made that list by going over an HTML grammar and enumerating meta-characters and included '/' out of an abundance of caution.

@adg
Copy link
Contributor

adg commented Jan 25, 2016

In that case, let's leave the html/template package as-is. Thanks for the info, @mikesamuel.

@adg adg closed this as completed Jan 25, 2016
@golang golang locked and limited conversation to collaborators Jan 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants