New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

o.s.web.util.JavaScriptUtils.javaScriptEscape insufficiently escapes some characters [SPR-9983] #14617

Closed
spring-issuemaster opened this Issue Nov 12, 2012 · 6 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Nov 12, 2012

Jon Passki opened SPR-9983 and commented

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


Affects: 3.0 GA, 3.1 GA, 3.2 RC1

Referenced from: commits 9982b4c, 7a7df66, f5c9fe6

0 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Jan 22, 2013

Rossen Stoyanchev commented

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?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Jan 23, 2013

Rossen Stoyanchev commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Jan 24, 2013

Jon Passki commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Feb 15, 2013

Rossen Stoyanchev commented

Thanks, very helpful!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Dec 4, 2013

Arun Neelicattu commented

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.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Dec 5, 2013

Jon Passki commented

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment