Implement Encode.forJSON()
OpenGG opened this issue · 6 comments
OpenGG commented
The XSS prevention cheat sheet mentioned the anti-pattern, placing json inside js block.
<script>
var initData = <%= data.to_json %>; // Do NOT do this without encoding the data with one of the techniques listed below.
</script>This has been a common practice, and is an important part of redux's server rendering. But owasp-java-encoder doesn't has the right encoding function to deal with it.
Is it consider doable and necessary, to implement Encode.forJSON()?
From what I read on the cheat sheet, converting < to \u003c would be good enough.
jmanico commented
To safely embed JSON I continue to advise NOT encoding it. Instead
serialize it to safely embed it.
Like so...
https://github.com/yahoo/serialize-javascript
<script>window.__INITIAL_STATE = <%= serialize(data.to_json) %></script>
This is so an attack like
{ haxorXSS: '</script>' }
would render safely as
{"haxorXSS":"\\u003C\\u002Fscript\\u003E"}
****
You can encode it, but you do not need a JSON specific encoder.
What you can do is HTML escape it per
https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.233.1_-_HTML_escape_JSON_values_in_an_HTML_context_and_read_the_data_with_JSON.parse
...
(from the cheatsheet)
<div id="init_data" style="display: none">
<%= html_escape(data.to_json) %>
</div>
// external js file
var dataElement = document.getElementById('init_data');
// decode and parse the content of the div
var initData = JSON.parse(dataElement.textContent);
So a new encoding routine should not be needed.
Aloha, Jim
…On 5/9/18 1:16 AM, OpenGG wrote:
The XSS prevention cheat sheet
<https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.233.1_-_HTML_escape_JSON_values_in_an_HTML_context_and_read_the_data_with_JSON.parse>
mentioned the anti-pattern, placing json inside js block.
<script>
var initData = <%= data.to_json %>; // Do NOT do this without encoding
the data with one of the techniques listed below.
</script>
This has been a common practice, and is an important part of redux's
server rendering <https://redux.js.org/recipes/server-rendering>. But
|owasp-java-encoder| doesn't has the right encoding function to deal
with it.
Is this consider doable and necessary, to implement |Encode.forJSON()|?
From what I read on the cheat sheet, converting |<| to |\u003c| would
be good enough.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#16>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AAgcCeTEkGYsm6Tl6N2z-rL_jyzKGpLzks5twtAogaJpZM4T4JEh>.
--
Jim Manico
Manicode Security
https://www.manicode.com
jmanico commented
So I am digging into this more. The Yahoo serializer is old and only
supports certain features.
The more I look into this the more I think the escaping method listed
below is best because HTML escaping in this context is always safe
regardless of the JSON features...
<div id="init_data" style="display: none">
<%= Encoder.encodeForHTML(data.to_json) %>
</div>
And parsing JSON with JSON.parse is always safe.
This will work without needing to worry about the serialization feature set.
// external js file
var dataElement = document.getElementById('init_data');
// decode and parse the content of the div
var initData = JSON.parse(dataElement.textContent);
******
I also wonder (but have not tested) if just JS escaping here is the
right move. Note it's a quoted string and JS encoded. I know this step
is safe...
<script>
var initData = '<%= Encoder.encodeForJavascript(data.to_json) %>';
</script>
And parsing JSON with JSON.parse is always safe.
// decode and parse the content of the div
var initData = JSON.parse(initData);
Can you do a quick test to see if basic JS encoding works since you're
sticking it in a JS variable?
Aloha, Jim
… To safely embed JSON I continue to advise NOT encoding it. Instead
serialize it to safely embed it.
Like so...
https://github.com/yahoo/serialize-javascript
<script>window.__INITIAL_STATE = <%= serialize(data.to_json) %></script>
This is so an attack like
{ haxorXSS: '</script>' }
would render safely as
{"haxorXSS":"\\u003C\\u002Fscript\\u003E"}
****
You can encode it, but you do not need a JSON specific encoder.
What you can do is HTML escape it per
https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.233.1_-_HTML_escape_JSON_values_in_an_HTML_context_and_read_the_data_with_JSON.parse
...
(from the cheatsheet)
<div id="init_data" style="display: none">
<%= html_escape(data.to_json) %>
</div>
// external js file
var dataElement = document.getElementById('init_data');
// decode and parse the content of the div
var initData = JSON.parse(dataElement.textContent);
So a new encoding routine should not be needed.
Aloha, Jim
On 5/9/18 1:16 AM, OpenGG wrote:
>
> The XSS prevention cheat sheet
> <https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.233.1_-_HTML_escape_JSON_values_in_an_HTML_context_and_read_the_data_with_JSON.parse>
> mentioned the anti-pattern, placing json inside js block.
>
> <script>
> var initData = <%= data.to_json %>; // Do NOT do this without
> encoding the data with one of the techniques listed below.
> </script>
>
> This has been a common practice, and is an important part of redux's
> server rendering <https://redux.js.org/recipes/server-rendering>. But
> |owasp-java-encoder| doesn't has the right encoding function to deal
> with it.
>
> Is this consider doable and necessary, to implement |Encode.forJSON()|?
>
> From what I read on the cheat sheet, converting |<| to |\u003c| would
> be good enough.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#16>, or mute the
> thread
> <https://github.com/notifications/unsubscribe-auth/AAgcCeTEkGYsm6Tl6N2z-rL_jyzKGpLzks5twtAogaJpZM4T4JEh>.
>
--
Jim Manico
Manicode Security
https://www.manicode.com
--
Jim Manico
Manicode Security
https://www.manicode.com
OpenGG commented
- Encoding everything in the unicode format
\u0000inflates the output, and may not be the default setting for most JSON encoders. - An extra HTML-encode-js-decode operation bloats the problem and make it more complicated. Consider the situation that multiple JSON being embed into one HTML.
var initData = JSON.parse('<%= Encoder.forJavaScript(data.to_json) %>');does work, but not every browser supportsJSON.parse(), hence server-side encoding introduces client-side dependency.
For now I can go with JSON.parse('<%= Encoder.forJavaScript(data.to_json) %>'), but I really hope for a more intuitive Encode.forJSON().
As a common practice, embedding JSON in HTML is popular. Implementing such encoding function can help the community moving forward.
jmanico commented
Encoding is about the slot you put data into HTML, not about the content you’re embedding. There is no all-purpose JSON encoding format that will universally work in all HTML locations. I’m glad the JS trick worked for Javascript locations like..
<script>
JSON.parse('<%= Encoder.encodeForJavascript(data.to_json) %>')
</script>
If you don’t like the name encodeForJavascript may I suggest building a wrapper class with your encodeForJSON and just call our function?
Again, we can’t guarantee an encoding JSON format that will work in all situations. It depends where you embed JSON.
Browsers that do support JSON.parse are really old and unsafe to handle JSON, but folks did use eval() for that back in the day - try using JQuerys function for JSON parsing. It’s standard practice to push all JSON through JSON.parse.
So still, I’m close marking this “wont fix” because I’d don’t see a clear way to do it.
… On May 9, 2018, at 7:06 PM, OpenGG ***@***.***> wrote:
@jmanico
Encoding everything in the unicode format \u0000 inflates the output, and may not be the default setting for most JSON encoders.
An extra HTML-encode-js-decode operation bloats the problem and make it more complicated. Consider the situation that multiple JSON being embed into one HTML.
var initData = JSON.parse('<%= Encoder.encodeForJavascript(data.to_json) %>'); does work, but not every browser supports JSON.parse(), hence server-side encoding introduce client-side dependency.
For now I can go with JSON.parse('<%= Encoder.encodeForJavascript(data.to_json) %>'), but I really hope for a more intuitive Encode.forJSON().
As a common practice, embedding JSON in HTML is popular. Implementing such encoding function can help the community moving forward.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
OpenGG commented
Again, we can’t guarantee an encoding JSON format that will work in all situations. It depends where you embed JSON.
Make sense, Consider the following two cases, one encoding function Encode.forJSON() won't work well in both of them.
<a onclick="javascript:x=${Encode.forJSON(data)}">
<script>
x = ${Encode.forJSON(data)}
</script>This could be the limitation of encoder libraries like owasp-java-encoder. Without further information of the context, an encoding library can't properly escape complex data type like JSON.
I will look into contextual automatic escaping templates, and see if they can do better. Meanwhile, please mark this issue as "wont fix".
jmanico commented
I agree that automatic encoding templates are the best path forward. Thanks for jumping in here with this thread.