Spring Framework
  1. Spring Framework
  2. SPR-9983

o.s.web.util.JavaScriptUtils.javaScriptEscape insufficiently escapes some characters

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: 3.0 GA, 3.1 GA, 3.2 RC1
    • Fix Version/s: 3.2.2
    • Component/s: Web
    • Labels:
      None
    • Last commented by a User:
      true

      Description

      JavaScriptUtils.javaScriptEscape() currently does not escape all characters that are sensitive within either a JS single quoted string, JS double quoted string, or HTML script data context.

      ECMAScript 5.1 (ECMA 262) [1] defines a line terminator as either U+000A (LF), U+000D (CR), U+2028 (PS), or U+2029 (LS). Line terminators are disallowed in either string context. Their inclusion ought to result in a parse error if inserted without escaping. The javaScriptEscape() method currently escapes U+000A and removes U+000D.

      HTML 5's Tokenizer defines different states that can occur within a <script> tag [2]. If the value "<!--" is inserted, the tokenizer will be at the "Script data escaped dash dash state". From here, one can insert "<script>" and be at the "Script data double escaped state". These states are respected by HTML 5 capable browser. If the state is changed without closing the state, a parse error ought to occur.

      The escaper should be updated to Unicode escape PS, LS, "<", and ">" characters. This should prevent parse errors in most applications and potential security side effects in some applications (e.g. disabling of frame breaking JS).

      [1] http://www.ecma-international.org/publications/standards/Ecma-262.htm
      [2] http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#script-data-state

        Activity

        Hide
        Rossen Stoyanchev added a comment -

        Jon Passki, JavaScriptUtils indeed needs an update. Looking at the javadoc, it provides escaping of string literals based on the following (updated) reference. From table 2.1 with JavaScript special characters currently missing are \b and \v. I'll make sure those are added and the reference updated.

        Note that \r is removed only when it precedes \n. Otherwise it is replaced with \n.

        U+2028 (PS) and U+2029 (LS) are indeed listed as line terminators in ECMA-262. However, I don't see any special characters to represent them in a string? So what would an else if condition for those characters look like?

        Show
        Rossen Stoyanchev added a comment - Jon Passki , JavaScriptUtils indeed needs an update. Looking at the javadoc, it provides escaping of string literals based on the following (updated) reference . From table 2.1 with JavaScript special characters currently missing are \b and \v . I'll make sure those are added and the reference updated. Note that \r is removed only when it precedes \n . Otherwise it is replaced with \n . U+2028 (PS) and U+2029 (LS) are indeed listed as line terminators in ECMA-262. However, I don't see any special characters to represent them in a string? So what would an else if condition for those characters look like?
        Hide
        Rossen Stoyanchev added a comment -

        I've updated JavaScriptUtils to escape backspace and vertical tab and will leave the issue open for further comments.

        Show
        Rossen Stoyanchev added a comment - I've updated JavaScriptUtils to escape backspace and vertical tab and will leave the issue open for further comments.
        Hide
        Jon Passki added a comment -

        The < ought to be escaped. Please refer to [1] and [2] for potential security defects if it isn't escaped. <, PS, and LS can be escaped via Unicode escaping (\u####). The Coverity Security Library has an implementation at [3] for JavaScript strings that escapes these characters via Unicode escaping. Please use as a reference.

        [1] https://communities.coverity.com/blogs/security/2012/11/16/did-i-do-that-html-5-js-escapers-3
        [2] https://communities.coverity.com/blogs/security/2013/01/24/a-tale-of-two-parsers
        [3] https://github.com/coverity/coverity-security-library/blob/develop/coverity-escapers/src/main/java/com/coverity/security/Escape.java#L413

        Show
        Jon Passki added a comment - The < ought to be escaped. Please refer to [1] and [2] for potential security defects if it isn't escaped. <, PS, and LS can be escaped via Unicode escaping (\u####). The Coverity Security Library has an implementation at [3] for JavaScript strings that escapes these characters via Unicode escaping. Please use as a reference. [1] https://communities.coverity.com/blogs/security/2012/11/16/did-i-do-that-html-5-js-escapers-3 [2] https://communities.coverity.com/blogs/security/2013/01/24/a-tale-of-two-parsers [3] https://github.com/coverity/coverity-security-library/blob/develop/coverity-escapers/src/main/java/com/coverity/security/Escape.java#L413
        Hide
        Rossen Stoyanchev added a comment -

        Thanks, very helpful!

        Show
        Rossen Stoyanchev added a comment - Thanks, very helpful!
        Hide
        Arun Neelicattu added a comment -

        It looks like this exposes an exploitable XSS flaw, is that correct? If so, should this have a CVE ID assigned? I can request one via oss-sec list if required.

        Show
        Arun Neelicattu added a comment - It looks like this exposes an exploitable XSS flaw, is that correct? If so, should this have a CVE ID assigned? I can request one via oss-sec list if required.
        Hide
        Jon Passki added a comment -

        Arun, correct under certain situations the lack of escaping could result in a security defect. However I think the common case it'll result quality defect via an unexploitable parse error. I'd give it a CVSS v2 base vector of at most (AV:N/AC:M/Au:S/C:N/I:P/A:N) / 3.5.

        Show
        Jon Passki added a comment - Arun , correct under certain situations the lack of escaping could result in a security defect. However I think the common case it'll result quality defect via an unexploitable parse error. I'd give it a CVSS v2 base vector of at most (AV:N/AC:M/Au:S/C:N/I:P/A:N) / 3.5.

          People

          • Assignee:
            Rossen Stoyanchev
            Reporter:
            Jon Passki
            Last updater:
            Jon Passki
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              19 weeks, 3 days ago