hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 11:15:13 +02:00
Code Issues Packages Projects Releases Wiki Activity

Simplify the query and add more test cases

Browse Source
This commit is contained in:
luchua-bc
2020-10-31 01:33:24 +00:00
parent 67af9b0f3e
commit 756db4c03a
4 changed files with 92 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/ql/src/experimental/Security/CWE/CWE-600/UncaughtServletException.qhelp
View File

@@ -4,19 +4,19 @@
<qhelp>
<overview>
<p>
Even though the signatures for methods in a servlet include <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 a vulnerable state, possibly resulting in denial-of-service attacks, or the exposure of sensitive information because when a servlet throws an exception, the servlet container typically sends debugging information back to the user. And that information could be very valuable to an attacker.
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.
</p>
</overview>
<recommendation>
<p>
Handle method calls that throw 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.
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.
</p>
</recommendation>
<example>
<p>
In the first and second examples, subclasses of IOException and RuntimeException are not caught, which disclose stack traces.
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.
</p>
<p>

32
java/ql/src/experimental/Security/CWE/CWE-600/UncaughtServletException.ql
View File

@@ -1,6 +1,6 @@
/**
* @name Uncaught Servlet Exception
* @description Uncaught exceptions in a servlet could leave a system in a vulnerable state, possibly resulting in denial-of-service attacks or the exposure of sensitive information disclosed in stack traces.
* @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.
* @kind path-problem
* @id java/uncaught-servlet-exception
* @tags security
@@ -19,12 +19,12 @@ class IOException extends RefType {
IOException() { this.hasQualifiedName("java.io", "IOException") }
}
/** Check whether a given exception type is caught. */
private predicate catchesEx(TryStmt t, RefType exType) {
/** Holds if a given exception type is caught. */
private predicate exceptionIsCaught(TryStmt t, RefType exType) {
exists(CatchClause cc, LocalVariableDeclExpr v |
t.getACatchClause() = cc and
cc.getVariable() = v and
v.getType().(RefType).getASubtype*() = exType //Detect the case that a subclass exception is thrown but its parent class is declared in the catch clause.
v.getType().(RefType).getASubtype*() = exType // Detect the case that a subclass exception is thrown but its parent class is declared in the catch clause.
)
}
@@ -33,16 +33,8 @@ private predicate isServletMethod(Callable c) {
c.getDeclaringType() instanceof ServletClass and
c.getNumberOfParameters() = 2 and
c.getParameter(1).getType() instanceof ServletResponse and
(
c.getName() = "doGet" or
c.getName() = "doPost" or
c.getName() = "doPut" or
c.getName() = "doDelete" or
c.getName() = "doHead" or
c.getName() = "doOptions" or
c.getName() = "doTrace" or
c.getName() = "service"
)
c.getName() in ["doGet", "doPost", "doPut", "doDelete", "doHead", "doOptions", "doTrace",
"service"]
}
/** Holds if `web.xml` has an error page configured. */
@@ -50,27 +42,21 @@ private predicate hasErrorPage() {
exists(WebErrorPage wep | wep.getPageLocation().getValue() != "")
}
/** Sink of uncaught IO exceptions or runtime exceptions since other exception types must be explicitly caught. */
/** Sink of uncaught exceptions, which shall be IO exceptions or runtime exceptions since other exception types must be explicitly caught. */
class UncaughtServletExceptionSink extends DataFlow::ExprNode {
UncaughtServletExceptionSink() {
exists(Method m, MethodAccess ma | ma.getMethod() = m |
isServletMethod(ma.getEnclosingCallable()) and
(
m.getAThrownExceptionType().getASupertype*() instanceof IOException or
m
.getAThrownExceptionType()
.getASupertype*()
.hasQualifiedName("java.lang", "RuntimeException")
) and
ma.getAnArgument() = this.getExpr() and
not exists(TryStmt t |
t.getBlock() = ma.getEnclosingStmt().getEnclosingStmt*() and
catchesEx(t, m.getAThrownExceptionType())
exceptionIsCaught(t, m.getAThrownExceptionType())
)
)
}
}
/** Taint configuration of uncaught exceptions caused by user provided data from `RemoteFlowSource` */
class UncaughtServletExceptionConfiguration extends TaintTracking::Configuration {
UncaughtServletExceptionConfiguration() { this = "UncaughtServletException" }

4
java/ql/test/experimental/query-tests/security/CWE-600/UncaughtServletException.expected
View File

@@ -1,11 +1,15 @@
edges
| UncaughtServletException.java:13:15:13:43 | getParameter(...) : String | UncaughtServletException.java:14:44:14:45 | ip |
| UncaughtServletException.java:16:19:16:41 | getRemoteUser(...) : String | UncaughtServletException.java:17:20:17:25 | userId |
| UncaughtServletException.java:75:21:75:43 | getRemoteUser(...) : String | UncaughtServletException.java:76:22:76:27 | userId |
nodes
| UncaughtServletException.java:13:15:13:43 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| UncaughtServletException.java:14:44:14:45 | ip | semmle.label | ip |
| UncaughtServletException.java:16:19:16:41 | getRemoteUser(...) : String | semmle.label | getRemoteUser(...) : String |
| UncaughtServletException.java:17:20:17:25 | userId | semmle.label | userId |
| UncaughtServletException.java:75:21:75:43 | getRemoteUser(...) : String | semmle.label | getRemoteUser(...) : String |
| UncaughtServletException.java:76:22:76:27 | userId | semmle.label | userId |
#select
| 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 |
| 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 |
| 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 |

81
java/ql/test/experimental/query-tests/security/CWE-600/UncaughtServletException.java
View File

@@ -8,28 +8,99 @@ import javax.servlet.http.HttpServletResponse;
import javax.servlet.ServletException;
class UncaughtServletException extends HttpServlet {
// BAD - Tests `doGet` without catching exceptions
// BAD - Tests `doGet` without catching exceptions.
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
String ip = request.getParameter("srcIP");
InetAddress addr = InetAddress.getByName(ip); // BAD: getByName(String) throws UnknownHostException
InetAddress addr = InetAddress.getByName(ip); // getByName(String) throws UnknownHostException
String userId = request.getRemoteUser();
Integer.parseInt(userId); //BAD: Integer.parse(String) throws RuntimeException
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
}
// GOOD - Tests `doPost` with catching exceptions
// GOOD - Tests `doPost` with catching exceptions.
public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
try {
String ip = request.getParameter("srcIP");
InetAddress addr = InetAddress.getByName(ip);
String userId = request.getRemoteUser();
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
} catch (UnknownHostException uhex) {
uhex.printStackTrace();
} catch (RuntimeException re) {
re.printStackTrace();
}
}
// GOOD - Tests `doPut` without user provided data and without catching exceptions.
public void doPut(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
String ip = "10.100.10.81";
InetAddress addr = InetAddress.getByName(ip); // GOOD: hard-coded variable value or system property not controlled by attacker
}
}
// GOOD - Tests rethrowing caught exceptions without stack trace, which the typical programming practice.
public void doDelete(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
try {
String ip = request.getParameter("srcIP");
InetAddress addr = InetAddress.getByName(ip);
} catch (UnknownHostException uhex) {
throw new IOException("Host not found "+uhex.getMessage());
}
}
// BAD - Tests rethrowing caught exceptions with stack trace.
// Note this case is not yet detected by this query.
public void doOptions(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
try {
String ip = request.getParameter("srcIP");
InetAddress addr = InetAddress.getByName(ip);
} catch (UnknownHostException uhex) {
throw new IOException(uhex);
}
}
// GOOD - Tests invoking another top-level method.
public void doHead(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
doGet(request, response);
}
// BAD - Tests nested try-blocks without catching runtime exceptions.
public void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
try {
String ip = request.getParameter("srcIP");
InetAddress addr = null;
try {
addr = InetAddress.getByName(ip);
String userId = request.getRemoteUser();
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
} catch (UnknownHostException uhex) {
throw new UnknownHostException("Got exception "+uhex.getMessage());
}
} catch (IOException ie) {
ie.printStackTrace();
}
}
// GOOD - Tests nested try-blocks with catching all exceptions.
public void doTrace(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
try {
try {
String ip = request.getParameter("srcIP");
InetAddress addr = null;
try {
addr = InetAddress.getByName(ip);
String userId = request.getRemoteUser();
Integer.parseInt(userId); // Integer.parse(String) throws RuntimeException
} catch (UnknownHostException uhex) {
throw new UnknownHostException("Got exception "+uhex.getMessage());
}
} catch (IOException ie) {
ie.printStackTrace();
}
} catch (RuntimeException re) {
re.printStackTrace();
}
}
}