Skip to content

Commit 0cc324b

Browse files
aschackmullaschackmull
authored
Merge pull request #3839 from luchua-bc/uncaught-servlet-exception
Java: Uncaught servlet exception
2 parents 9eeacea + 0175a59 commit 0cc324b

File tree

12 files changed

+610
-1
lines changed

12 files changed

+610
-1
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-600/UncaughtServletException.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import java.io.InputStream;
2+
import java.io.IOException;
3+
import java.net.InetAddress;
4+
import java.net.UnknownHostException;
5+
6+
class UncaughtServletException extends HttpServlet {
7+
// BAD: Uncaught exceptions
8+
{
9+
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
10+
String ip = request.getParameter("srcIP");
11+
InetAddress addr = InetAddress.getByName(ip); //BAD: getByName(String) throws UnknownHostException.
12+
13+
String username = request.getRemoteUser();
14+
Integer.parseInt(username); //BAD: Integer.parse(String) throws RuntimeException.
15+
}
16+
}
17+
18+
// GOOD
19+
{
20+
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
21+
try {
22+
String ip = request.getParameter("srcIP");
23+
InetAddress addr = InetAddress.getByName(ip);
24+
} catch (UnknownHostException uhex) { //GOOD: Catch the subclass exception UnknownHostException of IOException.
25+
uhex.printStackTrace();
26+
}
27+
}
28+
}
29+
30+
// GOOD
31+
{
32+
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
33+
String ip = "10.100.10.81";
34+
InetAddress addr = InetAddress.getByName(ip); // OK: Hard-coded variable value or system property is not controlled by attacker.
35+
}
36+
}
37+
38+
}

‎java/ql/src/experimental/Security/CWE/CWE-600/UncaughtServletException.qhelp‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
<overview>
4+
<p>
5+
Even though the request-handling methods of <code>Servlet</code> are declared <code>throws IOException, ServletException</code>, it's a bad idea to let such exceptions be thrown. Failure to catch exceptions in a servlet could leave a system in an unexpected state, possibly resulting in denial-of-service attacks, or could lead to exposure of sensitive information because when a servlet throws an exception, the servlet container typically sends debugging information back to the user. That information could be valuable to an attacker.
6+
</p>
7+
</overview>
8+
9+
<recommendation>
10+
<p>
11+
Catch IOExceptions and/or RuntimeExceptions and display custom error messages without stack traces and sensitive information, or configure an <code>error-page</code> in web.xml to display a generic user-friendly message for any uncaught exception.
12+
</p>
13+
</recommendation>
14+
15+
<example>
16+
<p>
17+
In the first and second examples, subclasses of IOException and RuntimeException are not caught, which disclose stack traces. Because user-controlled data is passed to methods that throw, there is an opportunity for an attacker to provoke a stack dump.
18+
</p>
19+
20+
<p>
21+
In the third example, the code catches the exception. In the fourth example, the code is not of concern since the variable cannot be controlled by attackers thus no unexpected exceptions can be thrown.
22+
</p>
23+
<sample src="UncaughtServletException.java" />
24+
</example>
25+
26+
<references>
27+
<li>
28+
CWE:
29+
<a href="https://cwe.mitre.org/data/definitions/600.html">CWE-600: Uncaught Exception in Servlet</a>
30+
</li>
31+
<li>
32+
SonarSource:
33+
<a href="https://rules.sonarsource.com/java/tag/owasp/RSPEC-1989">Exceptions should not be thrown from servlet methods</a>
34+
</li>
35+
<li>
36+
OWASP:
37+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Error_Handling_Cheat_Sheet.html">Error Handling Cheat Sheet</a>
38+
</li>
39+
</references>
40+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-600/UncaughtServletException.ql‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/**
2+
* @name Uncaught Servlet Exception
3+
* @description Uncaught exceptions in a servlet could leave a system in an unexpected state, possibly resulting in denial-of-service attacks or the exposure of sensitive information disclosed in stack traces.
4+
* @kind path-problem
5+
* @id java/uncaught-servlet-exception
6+
* @tags security
7+
* external/cwe-600
8+
*/
9+
10+
import java
11+
import semmle.code.java.dataflow.FlowSources
12+
import semmle.code.java.dataflow.TaintTracking
13+
import semmle.code.java.frameworks.Servlets
14+
import semmle.code.xml.WebXML
15+
import DataFlow::PathGraph
16+
17+
/** Holds if a given exception type is caught. */
18+
private predicate exceptionIsCaught(TryStmt t, RefType exType) {
19+
exists(CatchClause cc, LocalVariableDeclExpr v |
20+
t.getACatchClause() = cc and
21+
cc.getVariable() = v and
22+
v.getType().(RefType).getASubtype*() = exType and // Detect the case that a subclass exception is thrown but its parent class is declared in the catch clause.
23+
not exists(
24+
ThrowStmt ts // Detect the edge case that exception is caught then rethrown without processing in a catch clause
25+
|
26+
ts.getEnclosingStmt() = cc.getBlock() and
27+
ts.getExpr() = v.getAnAccess()
28+
)
29+
)
30+
}
31+
32+
/** Servlet methods of `javax.servlet.http.Servlet` and subtypes. */
33+
private predicate isServletMethod(Callable c) {
34+
c.getDeclaringType() instanceof ServletClass and
35+
c.getNumberOfParameters() = 2 and
36+
c.getParameter(1).getType() instanceof ServletResponse and
37+
c.getName() in [
38+
"doGet", "doPost", "doPut", "doDelete", "doHead", "doOptions", "doTrace", "service"
39+
]
40+
}
41+
42+
/** Holds if `web.xml` has an error page configured. */
43+
private predicate hasErrorPage() {
44+
exists(WebErrorPage wep | wep.getPageLocation().getValue() != "")
45+
}
46+
47+
/** Sink of uncaught exceptions, which shall be IO exceptions or runtime exceptions since other exception types must be explicitly caught. */
48+
class UncaughtServletExceptionSink extends DataFlow::ExprNode {
49+
UncaughtServletExceptionSink() {
50+
exists(Method m, MethodAccess ma | ma.getMethod() = m |
51+
isServletMethod(ma.getEnclosingCallable()) and
52+
exists(m.getAThrownExceptionType()) and // The called method might plausibly throw an exception.
53+
ma.getAnArgument() = this.getExpr() and
54+
not exists(TryStmt t |
55+
t.getBlock() = ma.getAnEnclosingStmt() and
56+
exceptionIsCaught(t, m.getAThrownExceptionType())
57+
)
58+
)
59+
}
60+
}
61+
62+
/** Taint configuration of uncaught exceptions caused by user provided data from `RemoteFlowSource` */
63+
class UncaughtServletExceptionConfiguration extends TaintTracking::Configuration {
64+
UncaughtServletExceptionConfiguration() { this = "UncaughtServletException" }
65+
66+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
67+
68+
override predicate isSink(DataFlow::Node sink) { sink instanceof UncaughtServletExceptionSink }
69+
}
70+
71+
from DataFlow::PathNode source, DataFlow::PathNode sink, UncaughtServletExceptionConfiguration c
72+
where c.hasFlowPath(source, sink) and not hasErrorPage()
73+
select sink.getNode(), source, sink, "$@ flows to here and can throw uncaught exception.",
74+
source.getNode(), "User-provided value"

‎java/ql/src/semmle/code/xml/WebXML.qll‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,3 +130,40 @@ class WebListenerClass extends WebXMLElement {
130130
*/
131131
Class getClass() { result.getQualifiedName() = getValue() }
132132
}
133+
134+
/**
135+
* An `<error-page>` element in a `web.xml` file.
136+
*/
137+
class WebErrorPage extends WebXMLElement {
138+
WebErrorPage() { this.getName() = "error-page" }
139+
140+
/**
141+
* Gets the `<exception-type>` element of this `<error-page>`.
142+
*/
143+
WebErrorPageType getPageType() { result = getAChild() }
144+
145+
/**
146+
* Gets the `<location>` element of this `<error-page>`.
147+
*/
148+
WebErrorPageLocation getPageLocation() { result = getAChild() }
149+
}
150+
151+
/**
152+
* An `<exception-type>` element in a `web.xml` file, nested under an `<error-page>` element.
153+
*/
154+
class WebErrorPageType extends WebXMLElement {
155+
WebErrorPageType() {
156+
getName() = "exception-type" and
157+
getParent() instanceof WebErrorPage
158+
}
159+
}
160+
161+
/**
162+
* A `<location>` element in a `web.xml` file, nested under an `<error-page>` element.
163+
*/
164+
class WebErrorPageLocation extends WebXMLElement {
165+
WebErrorPageLocation() {
166+
getName() = "location" and
167+
getParent() instanceof WebErrorPage
168+
}
169+
}

‎java/ql/test/experimental/query-tests/security/CWE-600/UncaughtServletException.expected‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
edges
2+
| UncaughtServletException.java:13:15:13:43 | getParameter(...) : String | UncaughtServletException.java:14:44:14:45 | ip |
3+
| UncaughtServletException.java:16:19:16:41 | getRemoteUser(...) : String | UncaughtServletException.java:17:20:17:25 | userId |
4+
| UncaughtServletException.java:54:16:54:44 | getParameter(...) : String | UncaughtServletException.java:55:45:55:46 | ip |
5+
| UncaughtServletException.java:75:21:75:43 | getRemoteUser(...) : String | UncaughtServletException.java:76:22:76:27 | userId |
6+
nodes
7+
| UncaughtServletException.java:13:15:13:43 | getParameter(...) : String | semmle.label | getParameter(...) : String |
8+
| UncaughtServletException.java:14:44:14:45 | ip | semmle.label | ip |
9+
| UncaughtServletException.java:16:19:16:41 | getRemoteUser(...) : String | semmle.label | getRemoteUser(...) : String |
10+
| UncaughtServletException.java:17:20:17:25 | userId | semmle.label | userId |
11+
| UncaughtServletException.java:54:16:54:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
12+
| UncaughtServletException.java:55:45:55:46 | ip | semmle.label | ip |
13+
| UncaughtServletException.java:75:21:75:43 | getRemoteUser(...) : String | semmle.label | getRemoteUser(...) : String |
14+
| UncaughtServletException.java:76:22:76:27 | userId | semmle.label | userId |
15+
#select
16+
| UncaughtServletException.java:14:44:14:45 | ip | UncaughtServletException.java:13:15:13:43 | getParameter(...) : String | UncaughtServletException.java:14:44:14:45 | ip | $@ flows to here and can throw uncaught exception. | UncaughtServletException.java:13:15:13:43 | getParameter(...) | User-provided value |
17+
| UncaughtServletException.java:17:20:17:25 | userId | UncaughtServletException.java:16:19:16:41 | getRemoteUser(...) : String | UncaughtServletException.java:17:20:17:25 | userId | $@ flows to here and can throw uncaught exception. | UncaughtServletException.java:16:19:16:41 | getRemoteUser(...) | User-provided value |
18+
| UncaughtServletException.java:55:45:55:46 | ip | UncaughtServletException.java:54:16:54:44 | getParameter(...) : String | UncaughtServletException.java:55:45:55:46 | ip | $@ flows to here and can throw uncaught exception. | UncaughtServletException.java:54:16:54:44 | getParameter(...) | User-provided value |
19+
| UncaughtServletException.java:76:22:76:27 | userId | UncaughtServletException.java:75:21:75:43 | getRemoteUser(...) : String | UncaughtServletException.java:76:22:76:27 | userId | $@ flows to here and can throw uncaught exception. | UncaughtServletException.java:75:21:75:43 | getRemoteUser(...) | User-provided value |

‎java/ql/test/experimental/query-tests/security/CWE-600/UncaughtServletException.java‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import java.io.IOException;
2+
import java.net.InetAddress;
3+
import java.net.UnknownHostException;
4+
5+
import javax.servlet.http.HttpServlet;
6+
import javax.servlet.http.HttpServletRequest;
7+
import javax.servlet.http.HttpServletResponse;
8+
import javax.servlet.ServletException;
9+
10+
class UncaughtServletException extends HttpServlet {
11+
// BAD - Tests `doGet` without catching exceptions.
12+
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
13+
String ip = request.getParameter("srcIP");
14+
InetAddress addr = InetAddress.getByName(ip); // getByName(String) throws UnknownHostException
15+
16+
String userId = request.getRemoteUser();
17+
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
18+
}
19+
20+
// GOOD - Tests `doPost` with catching exceptions.
21+
public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
22+
try {
23+
String ip = request.getParameter("srcIP");
24+
InetAddress addr = InetAddress.getByName(ip);
25+
26+
String userId = request.getRemoteUser();
27+
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
28+
} catch (UnknownHostException uhex) {
29+
uhex.printStackTrace();
30+
} catch (RuntimeException re) {
31+
re.printStackTrace();
32+
}
33+
}
34+
35+
// GOOD - Tests `doPut` without user provided data and without catching exceptions.
36+
public void doPut(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
37+
String ip = "10.100.10.81";
38+
InetAddress addr = InetAddress.getByName(ip); // GOOD: hard-coded variable value or system property not controlled by attacker
39+
}
40+
41+
// GOOD - Tests rethrowing caught exceptions without stack trace, which the typical programming practice.
42+
public void doDelete(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
43+
try {
44+
String ip = request.getParameter("srcIP");
45+
InetAddress addr = InetAddress.getByName(ip);
46+
} catch (UnknownHostException uhex) {
47+
throw new IOException("Host not found "+uhex.getMessage());
48+
}
49+
}
50+
51+
// BAD - Tests rethrowing caught exceptions with stack trace.
52+
public void doOptions(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
53+
try {
54+
String ip = request.getParameter("srcIP");
55+
InetAddress addr = InetAddress.getByName(ip);
56+
} catch (UnknownHostException uhex) {
57+
uhex.printStackTrace();
58+
throw uhex;
59+
}
60+
}
61+
62+
// GOOD - Tests invoking another top-level method.
63+
public void doHead(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
64+
doGet(request, response);
65+
}
66+
67+
// BAD - Tests nested try-blocks without catching runtime exceptions.
68+
public void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
69+
try {
70+
String ip = request.getParameter("srcIP");
71+
InetAddress addr = null;
72+
try {
73+
addr = InetAddress.getByName(ip);
74+
75+
String userId = request.getRemoteUser();
76+
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
77+
} catch (UnknownHostException uhex) {
78+
throw new UnknownHostException("Got exception "+uhex.getMessage());
79+
}
80+
} catch (IOException ie) {
81+
ie.printStackTrace();
82+
}
83+
}
84+
85+
// GOOD - Tests nested try-blocks with catching all exceptions.
86+
public void doTrace(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
87+
try {
88+
try {
89+
String ip = request.getParameter("srcIP");
90+
InetAddress addr = null;
91+
try {
92+
addr = InetAddress.getByName(ip);
93+
94+
String userId = request.getRemoteUser();
95+
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
96+
} catch (UnknownHostException uhex) {
97+
throw new UnknownHostException("Got exception "+uhex.getMessage());
98+
}
99+
} catch (IOException ie) {
100+
ie.printStackTrace();
101+
}
102+
} catch (RuntimeException re) {
103+
re.printStackTrace();
104+
}
105+
}
106+
}

‎java/ql/test/experimental/query-tests/security/CWE-600/UncaughtServletException.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-600/UncaughtServletException.ql

‎java/ql/test/experimental/query-tests/security/CWE-600/UncaughtServletException2.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import java.io.IOException;
2+
import java.net.InetAddress;
3+
import java.net.UnknownHostException;
4+
5+
import javax.servlet.http.HttpServlet;
6+
import javax.servlet.http.HttpServletRequest;
7+
import javax.servlet.http.HttpServletResponse;
8+
import javax.servlet.ServletException;
9+
10+
class UncaughtServletException2 extends HttpServlet {
11+
// BAD - Tests rethrowing caught exceptions with stack trace using `initCause(...)`
12+
// Note this special case is not being handled by the query since in 99% of cases we're looking for `catch(Exception e) { ... throw e; }`
13+
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
14+
try {
15+
String ip = request.getParameter("srcIP");
16+
InetAddress addr = InetAddress.getByName(ip);
17+
} catch (UnknownHostException uhex) {
18+
IOException ioException = new IOException();
19+
ioException.initCause(uhex);
20+
throw ioException;
21+
}
22+
}
23+
24+
// BAD - Tests rethrowing caught exceptions with stack trace using the same exception variable.
25+
// Note this special case is not being handled by the query since in 99% of cases we're looking for `catch(Exception e) { ... throw e; }`
26+
public void doHead(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
27+
try {
28+
String ip = request.getParameter("srcIP");
29+
InetAddress addr = InetAddress.getByName(ip);
30+
} catch (UnknownHostException uhex) {
31+
throw new IOException(uhex);
32+
}
33+
}
34+
35+
// BAD - Tests rethrowing caught exceptions with stack trace using `addSuppressed(...)`.
36+
// Note this special case is not being handled by the query since in 99% of cases we're looking for `catch(Exception e) { ... throw e; }`
37+
public void doTrace(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
38+
try {
39+
String ip = request.getParameter("srcIP");
40+
InetAddress addr = InetAddress.getByName(ip);
41+
} catch (UnknownHostException uhex) {
42+
IOException ioException = new IOException();
43+
ioException.addSuppressed(uhex);
44+
throw ioException;
45+
}
46+
}
47+
48+
// BAD - Tests rethrowing caught exceptions with stack trace using `initCause(...)`
49+
// Note this special case is not being handled by the query since in 99% of cases we're looking for `catch(Exception e) { ... throw e; }`
50+
public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
51+
try {
52+
String ip = request.getParameter("srcIP");
53+
InetAddress addr = InetAddress.getByName(ip);
54+
} catch (UnknownHostException uhex) {
55+
IOException ioException = new IOException();
56+
throw new IOException(ioException.initCause(uhex));
57+
}
58+
}
59+
}

‎java/ql/test/experimental/query-tests/security/CWE-600/options‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4

0 commit comments

Comments
 (0)