[
https://issues.apache.org/jira/browse/SLING-5946?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16988288#comment-16988288
]
Ilguiz Latypov edited comment on SLING-5946 at 12/4/19 11:27 PM:
-----------------------------------------------------------------
Please reopen this due to an improper implementation risking 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 here and encoding the opening
angle bracket {{raw <}} as {{raw \u003C}} in the {{Encode.forJavaScript}}
implementation. This will also protect against the 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]
was (Author: ilatypov):
Please reopen this due to an improper implementation risking 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 {{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 here and encoding the opening
angle bracket {{raw <}} as {{raw \u003C}} in the {{Encode.forJavaScript}}
implementation. This will also protect against the 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
> XSSAPI#encodeForJSString is not restrictive enough
> --------------------------------------------------
>
> Key: SLING-5946
> URL: https://issues.apache.org/jira/browse/SLING-5946
> Project: Sling
> Issue Type: Bug
> Components: Extensions
> Affects Versions: XSS Protection API 1.0.8
> Reporter: Vlad Bailescu
> Assignee: Robert Munteanu
> Priority: Major
> Fix For: XSS Protection API 1.0.12
>
> Attachments: SLING_5946.patch
>
>
> Since SLING-5445, {{XSSAPI#encodeForJSString}} is no longer properly encoding
> {{</script>}} and {{<!--}}. We should revert to using OWASP
> {{Encode#forJavaScript}} and handle - characters correctly for JSON too, by
> replacing them with {{\u002D}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
