Skip to content

Commit e75c96c

Browse files
Jami CogswellJami Cogswell
Jami Cogswell
authored and
Jami Cogswell
committed
Java: combine test cases; add test for StaplerResponse.forward
1 parent 5fa63ab commit e75c96c

File tree

4 files changed

+180
-185
lines changed

4 files changed

+180
-185
lines changed

‎java/ql/test/query-tests/security/CWE-552/UnsafeRequestPath.java

Lines changed: 0 additions & 52 deletions
This file was deleted.

‎java/ql/test/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java

Lines changed: 0 additions & 131 deletions
This file was deleted.

‎java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java

Lines changed: 179 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,29 @@
11
import java.io.IOException;
2+
import java.net.URLDecoder;
3+
import java.nio.file.Path;
4+
import java.nio.file.Paths;
5+
6+
import javax.servlet.Filter;
7+
import javax.servlet.FilterChain;
8+
import javax.servlet.RequestDispatcher;
9+
import javax.servlet.ServletConfig;
10+
import javax.servlet.ServletContext;
211
import javax.servlet.ServletException;
12+
import javax.servlet.ServletRequest;
13+
import javax.servlet.ServletResponse;
14+
import javax.servlet.http.HttpServlet;
315
import javax.servlet.http.HttpServletRequest;
416
import javax.servlet.http.HttpServletResponse;
517
import org.springframework.stereotype.Controller;
618
import org.springframework.web.bind.annotation.GetMapping;
719
import org.springframework.web.servlet.ModelAndView;
20+
import org.kohsuke.stapler.StaplerRequest;
21+
import org.kohsuke.stapler.StaplerResponse;
822

923
@Controller
10-
public class UrlForwardTest {
24+
public class UrlForwardTest extends HttpServlet implements Filter {
1125

26+
// (1) ORIGINAL
1227
@GetMapping("/bad1")
1328
public ModelAndView bad1(String url) {
1429
return new ModelAndView(url); // $ hasUrlForward
@@ -75,4 +90,167 @@ public void good1(String url, HttpServletRequest request, HttpServletResponse re
7590
e.printStackTrace();
7691
}
7792
}
93+
94+
// (2) UnsafeRequestPath
95+
private static final String BASE_PATH = "/pages";
96+
97+
@Override
98+
// BAD: Request dispatcher from servlet path without check
99+
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
100+
throws IOException, ServletException {
101+
String path = ((HttpServletRequest) request).getServletPath();
102+
// A sample payload "/%57EB-INF/web.xml" can bypass this `startsWith` check
103+
if (path != null && !path.startsWith("/WEB-INF")) {
104+
request.getRequestDispatcher(path).forward(request, response); // $ hasUrlForward
105+
} else {
106+
chain.doFilter(request, response);
107+
}
108+
}
109+
110+
// GOOD: Request dispatcher from servlet path with check
111+
public void doFilter2(ServletRequest request, ServletResponse response, FilterChain chain)
112+
throws IOException, ServletException {
113+
String path = ((HttpServletRequest) request).getServletPath();
114+
115+
if (path.startsWith(BASE_PATH) && !path.contains("..")) {
116+
request.getRequestDispatcher(path).forward(request, response);
117+
} else {
118+
chain.doFilter(request, response);
119+
}
120+
}
121+
122+
// GOOD: Request dispatcher from servlet path with whitelisted string comparison
123+
public void doFilter3(ServletRequest request, ServletResponse response, FilterChain chain)
124+
throws IOException, ServletException {
125+
String path = ((HttpServletRequest) request).getServletPath();
126+
127+
if (path.equals("/comaction")) {
128+
request.getRequestDispatcher(path).forward(request, response);
129+
} else {
130+
chain.doFilter(request, response);
131+
}
132+
}
133+
134+
// (3) UnsafeServletRequestDispatch
135+
@Override
136+
// BAD: Request dispatcher constructed from `ServletContext` without input validation
137+
protected void doGet(HttpServletRequest request, HttpServletResponse response)
138+
throws ServletException, IOException {
139+
String action = request.getParameter("action");
140+
String returnURL = request.getParameter("returnURL");
141+
142+
ServletConfig cfg = getServletConfig();
143+
if (action.equals("Login")) {
144+
ServletContext sc = cfg.getServletContext();
145+
RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
146+
rd.forward(request, response);
147+
} else {
148+
ServletContext sc = cfg.getServletContext();
149+
RequestDispatcher rd = sc.getRequestDispatcher(returnURL); // $ hasUrlForward
150+
rd.forward(request, response);
151+
}
152+
}
153+
154+
@Override
155+
// BAD: Request dispatcher constructed from `HttpServletRequest` without input validation
156+
protected void doPost(HttpServletRequest request, HttpServletResponse response)
157+
throws ServletException, IOException {
158+
String action = request.getParameter("action");
159+
String returnURL = request.getParameter("returnURL");
160+
161+
if (action.equals("Login")) {
162+
RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp");
163+
rd.forward(request, response);
164+
} else {
165+
RequestDispatcher rd = request.getRequestDispatcher(returnURL); // $ hasUrlForward
166+
rd.forward(request, response);
167+
}
168+
}
169+
170+
@Override
171+
// GOOD: Request dispatcher with a whitelisted URI
172+
protected void doPut(HttpServletRequest request, HttpServletResponse response)
173+
throws ServletException, IOException {
174+
String action = request.getParameter("action");
175+
176+
if (action.equals("Login")) {
177+
RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp");
178+
rd.forward(request, response);
179+
} else if (action.equals("Register")) {
180+
RequestDispatcher rd = request.getRequestDispatcher("/Register.jsp");
181+
rd.forward(request, response);
182+
}
183+
}
184+
185+
// BAD: Request dispatcher without path traversal check
186+
protected void doHead2(HttpServletRequest request, HttpServletResponse response)
187+
throws ServletException, IOException {
188+
String path = request.getParameter("path");
189+
190+
// A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check
191+
// The payload "/pages/welcome.jsp/../../%57EB-INF/web.xml" can bypass the check as well since RequestDispatcher will decode `%57` as `W`
192+
if (path.startsWith(BASE_PATH)) {
193+
request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
194+
}
195+
}
196+
197+
// GOOD: Request dispatcher with path traversal check
198+
protected void doHead3(HttpServletRequest request, HttpServletResponse response)
199+
throws ServletException, IOException {
200+
String path = request.getParameter("path");
201+
202+
if (path.startsWith(BASE_PATH) && !path.contains("..")) {
203+
request.getServletContext().getRequestDispatcher(path).include(request, response);
204+
}
205+
}
206+
207+
// GOOD: Request dispatcher with path normalization and comparison
208+
protected void doHead4(HttpServletRequest request, HttpServletResponse response)
209+
throws ServletException, IOException {
210+
String path = request.getParameter("path");
211+
Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
212+
213+
// /pages/welcome.jsp/../../WEB-INF/web.xml becomes /WEB-INF/web.xml
214+
// /pages/welcome.jsp/../../%57EB-INF/web.xml becomes /%57EB-INF/web.xml
215+
if (requestedPath.startsWith(BASE_PATH)) {
216+
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
217+
}
218+
}
219+
220+
// FN: Request dispatcher with negation check and path normalization, but without URL decoding
221+
// When promoting this query, consider using FlowStates to make `getRequestDispatcher` a sink
222+
// only if a URL-decoding step has NOT been crossed (i.e. make URLDecoder.decode change the
223+
// state to a different value than the one required at the sink).
224+
// TODO: but does this need to take into account URLDecoder.decode in a loop...?
225+
protected void doHead5(HttpServletRequest request, HttpServletResponse response)
226+
throws ServletException, IOException {
227+
String path = request.getParameter("path");
228+
Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
229+
230+
if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
231+
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ MISSING: hasUrlForward
232+
}
233+
}
234+
235+
// GOOD: Request dispatcher with path traversal check and URL decoding
236+
protected void doHead6(HttpServletRequest request, HttpServletResponse response)
237+
throws ServletException, IOException {
238+
String path = request.getParameter("path");
239+
boolean hasEncoding = path.contains("%");
240+
while (hasEncoding) {
241+
path = URLDecoder.decode(path, "UTF-8");
242+
hasEncoding = path.contains("%");
243+
}
244+
245+
if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
246+
request.getServletContext().getRequestDispatcher(path).include(request, response);
247+
}
248+
}
249+
250+
// New Tests (i.e. Added by me)
251+
public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object obj) throws IOException, ServletException {
252+
String url = req.getParameter("target");
253+
rsp.forward(obj, url, req); // $ hasUrlForward
254+
}
255+
78256
}

‎java/ql/test/query-tests/security/CWE-552/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/springframework-5.3.8/:${testdir}/../../../stubs/javax-faces-2.3/:${testdir}/../../../stubs/undertow-io-2.2/:${testdir}/../../../stubs/jboss-vfs-3.2/:${testdir}/../../../stubs/springframework-5.3.8/
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/springframework-5.3.8/:${testdir}/../../../stubs/javax-faces-2.3/:${testdir}/../../../stubs/undertow-io-2.2/:${testdir}/../../../stubs/jboss-vfs-3.2/:${testdir}/../../../stubs/stapler-1.263/:${testdir}/../../../stubs/apache-commons-fileupload-1.4/:${testdir}/../../../stubs/apache-commons-beanutils/:${testdir}/../../../stubs/saxon-xqj-9.x/:${testdir}/../../../stubs/apache-commons-lang/:${testdir}/../../../stubs/javax-servlet-2.5/

0 commit comments

Comments
 (0)