sling-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ilguiz Latypov (Jira)" <j...@apache.org>
Subject [jira] [Comment Edited] (SLING-5946) XSSAPI#encodeForJSString is not restrictive enough
Date Wed, 04 Dec 2019 23:31:00 GMT

    [ 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:30 PM:
-----------------------------------------------------------------

Please reopen this due to an improper implementation risking exceptions in strict javascript
engines.
|return source == null ? null : Encode.forJavaScript(source).replace("&#x5C;&#x5C;-",
"&#x5C;&#x5C;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 &#x5C;-}}, the {{forJavaScript}} call would properly change the backslash
into 2 backslashes {{raw &#x5C;&#x5C;-}} (this would result in the javascript engine
turning the string literal back to {{raw &#x5C;-}}). But the subsequent {{replace}} call
will destroy the context of the second backslash, turning the string into {{raw &#x5C;&#x5C;u002D}}
which would turn to {{raw &#x5C;u002D}} in the javascript engine's variable.

I argue for dropping the {{.replace()}} call here and encoding the opening angle bracket {{raw
<}} as {{raw &#x5C;u003C}} in the {{Encode.forJavaScript}} implementation. This will
also protect against the closing script tags {{raw &#x3C;&#x5C;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, 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]

> 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)

Mime
View raw message