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

XSS: remove forward slash #516

Merged
merged 1 commit into from Nov 9, 2020
Merged

XSS: remove forward slash #516

merged 1 commit into from Nov 9, 2020

Conversation

@89z
Copy link
Contributor

@89z 89z commented Nov 8, 2020

Encoding forward slash is an unneeded usage of the encoding. The only
characters that should be encoded would be the reserved characters &, <,
> and ":

https://developer.mozilla.org/Glossary/Entity

Further these implementations from major programming language vendors dont
encode forward slash:

I found other implementations as well with same behavior. Note that these
implementations are doing encoding, but just not with the forward slash
character. Further, its not recommended by the HTML5 specification:

https://w3.org/TR/html52/syntax.html#serializing-html-fragments

or even the HTML Living Standard:

https://html.spec.whatwg.org/multipage/parsing.html#serialising-html-fragments

Steven Penny
Encoding forward slash is an unneeded usage of the encoding. The only
characters that should be encoded would be the reserved characters `&`, `<`,
`>` and `"`:

https://developer.mozilla.org/Glossary/Entity

Further these implementations from major programming language vendors dont
encode forward slash:

- https://docs.microsoft.com/dotnet/api/system.web.httputility.htmlencode
- https://golang.org/pkg/html#EscapeString
- https://php.net/function.htmlentities
- https://docs.python.org/library/html.html#html.escape
- https://ruby-doc.org/stdlib/libdoc/cgi/rdoc/CGI/Util.html#method-i-escapeHTML

I found other implementations as well with same behavior. Note that these
implementations are doing encoding, but just not with the forward slash
character. Further, its not recommended by the HTML5 specification:

https://w3.org/TR/html52/syntax.html#serializing-html-fragments

or even the HTML Living Standard:

https://html.spec.whatwg.org/multipage/parsing.html#serialising-html-fragments
@89z 89z requested review from jmanico and mackowski as code owners Nov 8, 2020
@89z 89z mentioned this pull request Nov 8, 2020
@jmanico
jmanico approved these changes Nov 8, 2020
Copy link
Member

@jmanico jmanico left a comment

As per the OWASP Java Encoder and extensive research is done there to support encoding only the necessary characters when encoding is done correctly, I support this.

I would also ask that we build a unit test engine so we can test the encoder from any language to see how they satisfy our requirements. Well done, this is a rough one.

And as a side note, Jeff's opinion on maximum encoding is still very insightful and he might be right and this may haunt us someday.

@planetlevel
Copy link

@planetlevel planetlevel commented Nov 9, 2020

This is not about FUD and boogeymen. It's about assurance. There are different policies we could adopt.

1- Minimum -- probably & < and " would be sufficient in most situations
2- What everyone else does -- no known attacks that violate these rules
3- OWASP existing policy -- based on characters relevant to parsing HTML, no downside to anyone
4- Maximum -- encode all special characters, possible processing speed, document size downsides

I don't think you've justified your choice over any other the others (but see below). You just listed what others do. I personally battled with the Java Servlet Spec team for 10 years to try to improve security. I had some wins, like ending null byte injection into files and a few others. But I lost most of these battles, including ending header injection and others. The point is that "what others do" is a shit reason for adopting a policy. Others are often not thoughtful about security.

In the early 2000s, we figured out context-sensitive encoding. I personally created this cheat sheet to capture some rules about what to encode and what not to. Before that, there was no concept of context. I'm not trying to say we got it perfect. I'm saying we had to choose policies that would keep developers safe even when parser implementations were changing rapidly, inconsistent, and full of bugs (which I think is still the case).

Encoding all the special chars used in context-open and -close delimiters (3) seems like the simplest, easiest policy to follow in this environment and it works for every kind of parser, not just HTML. This policy makes closing a context (and therefore injection) impossible (unless directly putting untrusted data into code - which isn't really injection), at the tiny cost of encoding a few characters which are possibly not strictly necessary to prevent injection in some parsers. It's insanely difficult to prove which characters are required as @garethheyes has repeatedly demonstrated over the years.

Still maybe I can help your argument. Perhaps you're arguing that more developers would benefit by being able to use their platform default encoding instead of building their own, and are therefore more likely to do encoding properly. If that's what you're thinking I'm totally on board. I'm a little sad because we lost the battle yet again. But I'll take it if it brings more security to more apps.

@mackowski
Copy link
Collaborator

@mackowski mackowski commented Nov 9, 2020

I am approving it for now.
Reasoning: There is no proof that escaping forward slash will improve defense against XSS, if all other special characters are escaped properly, but it forces developers to use non-standard implementation of the HTML escaping, what increases the risk of the mistake and makes the implementation harder.

That said I created a new issue to refresh the current XSS Prevention Cheat Sheet: #517.

See my comment in the: #515 with more details.

@mackowski mackowski merged commit d7a6823 into OWASP:master Nov 9, 2020
2 of 3 checks passed
2 of 3 checks passed
lint lint
Details
link-check
Details
Publishing Check
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants