[
https://issues.apache.org/jira/browse/SLING-8879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16989011#comment-16989011
]
Ilguiz Latypov commented on SLING-8879:
---------------------------------------
I see SLING-6679 replaced JSONObject with Johnzon in year 2017 which has a
simple serializer that does not reuse the "for javascript" encoder. On the
other hand, Johnzon would not protect against the closing script tag injection
or the open comment tag injection in cases where the javascript string literal
is embedded with the HTML response.
I see Johnzon substitutes the double quotes, as required by JSON and by the
need to protect against the attack by dropping out of the javascript string
literal context in a Javascript embedded in HTML. Of course.
It would be nice if Johnzon replaced the open angle bracket {{raw <}} with
an ASCII presentation of the respective Unicode, {{raw \u003C}}, which is
also compatible with both JSON and Javascript.
https://github.com/apache/johnzon/blob/master/johnzon-core/src/main/java/org/apache/johnzon/core/Strings.java#L64
XSSAPI encodeForJSString() could reuse Johnzon's above serialization. (I guess
this would require accessing Johnzon via Sling's service provider interface to
an abstract JSON implementation).
Our older AEM code still uses the older JSONObject, but I guess there would not
be a security (built-in XSS protection against abuse of javascript code with
string literals embedded in HTML) or stability (string javascript engine
failing to parse the backslash-forwardslash encoding I observed in
JSONObject#toString) update to that version of Sling.
> Make JSONObject#toString and XSSAPI#encodeForJSString both safe and correct
> for pasting into a javascript string literal
> ------------------------------------------------------------------------------------------------------------------------
>
> Key: SLING-8879
> URL: https://issues.apache.org/jira/browse/SLING-8879
> Project: Sling
> Issue Type: Bug
> Components: XSS Protection API
> Reporter: Ilguiz Latypov
> Priority: Minor
>
> The current implementation risks exceptions in strict javascript engines.
> |return source == null ? null :
> Encode.forJavaScript(source).replace("\\-", "\\u002D");|
> Substitutes on top of the encoder's result with the intent to correct the
> encoder are near-sighted (i.e. suffer from the context-free approach). If
> {{source}} had a backslash followed by a dash, i.e. {{raw \-}}, the
> {{forJavaScript}} call would properly change the backslash into 2 backslashes
> {{raw \\-}} (this would result in the javascript engine
> turning the string literal back to {{raw \-}}). But the subsequent
> {{replace}} call will destroy the context of the second backslash, turning
> the string into {{raw \\u002D}} which would turn to {{raw
> \u002D}} in the javascript engine's variable.
> I argue for dropping the {{.replace()}} call (aiming at disabling malicious
> comment injection) here and encoding the opening angle bracket {{raw <}} as
> {{raw \u003C}} in the {{Encode.forJavaScript}} implementation. This will
> protect against both the malicious comment injection and the injection of
> closing script tags {{raw <\script>}} forcing the javascript
> interpreter to drop out of the string literal context and drop out of the
> script context.
> The existing prefixing of forward slashes with a backslash agrees with JSON
> but not with Javascript. It should be removed in favour of replacing just the
> opening angle bracket.
> {noformat}
> SingleEscapeCharacter :: one of
> ' " \ b f n r t v
> {noformat}
> [https://www.ecma-international.org/ecma-262/6.0/#sec-literals-string-literals]
> [https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#Escape_notation]
> I noticed that JSONObject#toString suffers from the same idea of a
> non-universal protection of the forward slash. I guess both XSSAPI and
> JSONObject#toString reuse the same code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
