Skip to content

Commit 6cff0d0

Browse files
smowtonsmowton
authored
Merge pull request #6393 from luchua-bc/java/xss-jsf
Java: CWE-079 Query to detect XSS with JavaServer Faces (JSF)
2 parents 26eafcb + a1ad1dd commit 6cff0d0

File tree

15 files changed

+2203
-10
lines changed

15 files changed

+2203
-10
lines changed

‎java/change-notes/2021-09-14-jsf-support.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* Added basic support for untrusted data sources and XSS-vulnerable sinks relating to the JavaServer Faces (JSF) framework.

‎java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ private module Frameworks {
8484
private import semmle.code.java.frameworks.Flexjson
8585
private import semmle.code.java.frameworks.guava.Guava
8686
private import semmle.code.java.frameworks.jackson.JacksonSerializability
87+
private import semmle.code.java.frameworks.javaee.jsf.JSFRenderer
8788
private import semmle.code.java.frameworks.JavaxJson
8889
private import semmle.code.java.frameworks.JaxWS
8990
private import semmle.code.java.frameworks.JoddJson

‎java/ql/lib/semmle/code/java/dataflow/FlowSources.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import semmle.code.java.frameworks.spring.SpringWebClient
2525
import semmle.code.java.frameworks.Guice
2626
import semmle.code.java.frameworks.struts.StrutsActions
2727
import semmle.code.java.frameworks.Thrift
28+
import semmle.code.java.frameworks.javaee.jsf.JSFRenderer
2829
private import semmle.code.java.dataflow.ExternalFlow
2930

3031
/** A data flow source of remote user input. */

‎java/ql/lib/semmle/code/java/frameworks/javaee/jsf/JSFRenderer.qll‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/** Provides classes and predicates for working with JavaServer Faces renderer. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.ExternalFlow
5+
6+
/**
7+
* The JSF class `FacesContext` for processing HTTP requests.
8+
*/
9+
class FacesContext extends RefType {
10+
FacesContext() {
11+
this.hasQualifiedName(["javax.faces.context", "jakarta.faces.context"], "FacesContext")
12+
}
13+
}
14+
15+
private class ExternalContextSource extends SourceModelCsv {
16+
override predicate row(string row) {
17+
row =
18+
["javax.", "jakarta."] +
19+
[
20+
"faces.context;ExternalContext;true;getRequestParameterMap;();;ReturnValue;remote",
21+
"faces.context;ExternalContext;true;getRequestParameterNames;();;ReturnValue;remote",
22+
"faces.context;ExternalContext;true;getRequestParameterValuesMap;();;ReturnValue;remote",
23+
"faces.context;ExternalContext;true;getRequestPathInfo;();;ReturnValue;remote",
24+
"faces.context;ExternalContext;true;getRequestCookieMap;();;ReturnValue;remote",
25+
"faces.context;ExternalContext;true;getRequestHeaderMap;();;ReturnValue;remote",
26+
"faces.context;ExternalContext;true;getRequestHeaderValuesMap;();;ReturnValue;remote"
27+
]
28+
}
29+
}
30+
31+
/**
32+
* The method `getResponseWriter()` declared in JSF `ExternalContext`.
33+
*/
34+
class FacesGetResponseWriterMethod extends Method {
35+
FacesGetResponseWriterMethod() {
36+
getDeclaringType() instanceof FacesContext and
37+
hasName("getResponseWriter") and
38+
getNumberOfParameters() = 0
39+
}
40+
}
41+
42+
/**
43+
* The method `getResponseStream()` declared in JSF `ExternalContext`.
44+
*/
45+
class FacesGetResponseStreamMethod extends Method {
46+
FacesGetResponseStreamMethod() {
47+
getDeclaringType() instanceof FacesContext and
48+
hasName("getResponseStream") and
49+
getNumberOfParameters() = 0
50+
}
51+
}
52+
53+
private class ExternalContextXssSink extends SinkModelCsv {
54+
override predicate row(string row) {
55+
row =
56+
[
57+
"javax.faces.context;ResponseWriter;true;write;;;Argument[0];xss",
58+
"javax.faces.context;ResponseStream;true;write;;;Argument[0];xss",
59+
"jakarta.faces.context;ResponseWriter;true;write;;;Argument[0];xss",
60+
"jakarta.faces.context;ResponseStream;true;write;;;Argument[0];xss"
61+
]
62+
}
63+
}

‎java/ql/lib/semmle/code/java/security/XSS.qll‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import semmle.code.java.frameworks.Servlets
55
import semmle.code.java.frameworks.android.WebView
66
import semmle.code.java.frameworks.spring.SpringController
77
import semmle.code.java.frameworks.spring.SpringHttp
8+
import semmle.code.java.frameworks.javaee.jsf.JSFRenderer
89
import semmle.code.java.dataflow.DataFlow
910
import semmle.code.java.dataflow.TaintTracking2
1011
import semmle.code.java.dataflow.ExternalFlow
@@ -40,7 +41,7 @@ private class DefaultXssSink extends XssSink {
4041
DefaultXssSink() {
4142
sinkNode(this, "xss")
4243
or
43-
exists(ServletWriterSourceToWritingMethodFlowConfig writer, MethodAccess ma |
44+
exists(XssVulnerableWriterSourceToWritingMethodFlowConfig writer, MethodAccess ma |
4445
ma.getMethod() instanceof WritingMethod and
4546
writer.hasFlowToExpr(ma.getQualifier()) and
4647
this.asExpr() = ma.getArgument(_)
@@ -101,12 +102,14 @@ private class DefaultXSSSanitizer extends XssSanitizer {
101102
}
102103

103104
/** A configuration that tracks data from a servlet writer to an output method. */
104-
private class ServletWriterSourceToWritingMethodFlowConfig extends TaintTracking2::Configuration {
105-
ServletWriterSourceToWritingMethodFlowConfig() {
106-
this = "XSS::ServletWriterSourceToWritingMethodFlowConfig"
105+
private class XssVulnerableWriterSourceToWritingMethodFlowConfig extends TaintTracking2::Configuration {
106+
XssVulnerableWriterSourceToWritingMethodFlowConfig() {
107+
this = "XSS::XssVulnerableWriterSourceToWritingMethodFlowConfig"
107108
}
108109

109-
override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ServletWriterSource }
110+
override predicate isSource(DataFlow::Node src) {
111+
src.asExpr() instanceof XssVulnerableWriterSource
112+
}
110113

111114
override predicate isSink(DataFlow::Node sink) {
112115
exists(MethodAccess ma |
@@ -128,9 +131,9 @@ private class WritingMethod extends Method {
128131
}
129132
}
130133

131-
/** An output stream or writer that writes to a servlet response. */
132-
class ServletWriterSource extends MethodAccess {
133-
ServletWriterSource() {
134+
/** An output stream or writer that writes to a servlet, JSP or JSF response. */
135+
class XssVulnerableWriterSource extends MethodAccess {
136+
XssVulnerableWriterSource() {
134137
this.getMethod() instanceof ServletResponseGetWriterMethod
135138
or
136139
this.getMethod() instanceof ServletResponseGetOutputStreamMethod
@@ -139,9 +142,18 @@ class ServletWriterSource extends MethodAccess {
139142
m.getDeclaringType().getQualifiedName() = "javax.servlet.jsp.JspContext" and
140143
m.getName() = "getOut"
141144
)
145+
or
146+
this.getMethod() instanceof FacesGetResponseWriterMethod
147+
or
148+
this.getMethod() instanceof FacesGetResponseStreamMethod
142149
}
143150
}
144151

152+
/**
153+
* DEPRECATED: Use `XssVulnerableWriterSource` instead.
154+
*/
155+
deprecated class ServletWriterSource = XssVulnerableWriterSource;
156+
145157
/**
146158
* Holds if `s` is an HTTP Content-Type vulnerable to XSS.
147159
*/

‎java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ class ServletWriterSourceToPrintStackTraceMethodFlowConfig extends TaintTracking
3636
this = "StackTraceExposure::ServletWriterSourceToPrintStackTraceMethodFlowConfig"
3737
}
3838

39-
override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ServletWriterSource }
39+
override predicate isSource(DataFlow::Node src) {
40+
src.asExpr() instanceof XssVulnerableWriterSource
41+
}
4042

4143
override predicate isSink(DataFlow::Node sink) {
4244
exists(MethodAccess ma |

‎java/ql/test/query-tests/security/CWE-079/semmle/tests/JsfXSS.java‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import java.io.IOException;
2+
import java.util.Map;
3+
4+
import javax.faces.component.UIComponent;
5+
import javax.faces.context.ExternalContext;
6+
import javax.faces.context.FacesContext;
7+
import javax.faces.context.ResponseWriter;
8+
import javax.faces.render.FacesRenderer;
9+
import javax.faces.render.Renderer;
10+
import javax.servlet.http.Cookie;
11+
12+
@FacesRenderer(componentFamily = "", rendererType = "")
13+
public class JsfXSS extends Renderer
14+
{
15+
@Override
16+
// BAD: directly output user input.
17+
public void encodeBegin(FacesContext facesContext, UIComponent component) throws IOException
18+
{
19+
super.encodeBegin(facesContext, component);
20+
21+
Map<String, String> requestParameters = facesContext.getExternalContext().getRequestParameterMap();
22+
String windowId = requestParameters.get("window_id");
23+
24+
ResponseWriter writer = facesContext.getResponseWriter();
25+
writer.write("<script type=\"text/javascript\">");
26+
writer.write("(function(){");
27+
writer.write("dswh.init('" + windowId + "','" // $xss
28+
+ "......" + "',"
29+
+ -1 + ",{");
30+
writer.write("});");
31+
writer.write("})();");
32+
writer.write("</script>");
33+
}
34+
35+
// GOOD: use the method `writeText` that performs escaping appropriate for the markup language being rendered.
36+
public void encodeBegin2(FacesContext facesContext, UIComponent component) throws IOException
37+
{
38+
super.encodeBegin(facesContext, component);
39+
40+
Map<String, String> requestParameters = facesContext.getExternalContext().getRequestParameterMap();
41+
String windowId = requestParameters.get("window_id");
42+
43+
ResponseWriter writer = facesContext.getResponseWriter();
44+
writer.write("<script type=\"text/javascript\">");
45+
writer.write("(function(){");
46+
writer.write("dswh.init('");
47+
writer.writeText(windowId, null);
48+
writer.write("','"
49+
+ "......" + "',"
50+
+ -1 + ",{");
51+
writer.write("});");
52+
writer.write("})();");
53+
writer.write("</script>");
54+
}
55+
56+
public void testAllSources(FacesContext facesContext) throws IOException
57+
{
58+
ExternalContext ec = facesContext.getExternalContext();
59+
ResponseWriter writer = facesContext.getResponseWriter();
60+
writer.write(ec.getRequestParameterMap().keySet().iterator().next()); // $xss
61+
writer.write(ec.getRequestParameterNames().next()); // $xss
62+
writer.write(ec.getRequestParameterValuesMap().get("someKey")[0]); // $xss
63+
writer.write(ec.getRequestParameterValuesMap().keySet().iterator().next()); // $xss
64+
writer.write(ec.getRequestPathInfo()); // $xss
65+
writer.write(((Cookie)ec.getRequestCookieMap().get("someKey")).getName()); // $xss
66+
writer.write(ec.getRequestHeaderMap().get("someKey")); // $xss
67+
writer.write(ec.getRequestHeaderValuesMap().get("someKey")[0]); // $xss
68+
}
69+
}

‎java/ql/test/query-tests/security/CWE-079/semmle/tests/options‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/servlet-api-2.4:${testdir}/../../../../../stubs/javax-ws-rs-api-2.1.1/:${testdir}/../../../../../stubs/springframework-5.3.8
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/servlet-api-2.4:${testdir}/../../../../../stubs/javax-ws-rs-api-2.1.1/:${testdir}/../../../../../stubs/springframework-5.3.8:${testdir}/../../../../../stubs/javax-faces-2.3/

‎java/ql/test/stubs/javax-faces-2.3/javax/faces/component/UIComponent.java‎

Lines changed: 89 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)