From 195755d687ae9324b1acfc45e7eb0459bc18dba7 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 5 Jan 2021 00:04:08 +0000 Subject: [PATCH] Revamp the query to be more selective --- .../Security/CWE/CWE-598/SensitiveGetQuery.ql | 20 +++++------ .../semmle/code/java/frameworks/Servlets.qll | 23 ++++++++---- .../CWE-598/SensitiveGetQuery.expected | 36 ++++++++++++++++--- .../security/CWE-598/SensitiveGetQuery.java | 8 ++++- .../security/CWE-598/SensitiveGetQuery2.java | 29 +++++++++++++++ 5 files changed, 93 insertions(+), 23 deletions(-) create mode 100644 java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery2.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-598/SensitiveGetQuery.ql b/java/ql/src/experimental/Security/CWE/CWE-598/SensitiveGetQuery.ql index 39f6b3c8b52..85464084135 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-598/SensitiveGetQuery.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-598/SensitiveGetQuery.ql @@ -23,15 +23,15 @@ class SensitiveInfoExpr extends Expr { } } -/** Holds if `c` is a call to some override of `HttpServlet.doGet`. */ -private predicate isGetServletMethod(Callable c) { isServletMethod(c, "doGet") } +/** Holds if `ma` is a method access to some override of `HttpServlet.doGet`. */ +private predicate isGetServletMethod(MethodAccess ma) { isServletMethod(ma, "doGet") } -/** Sink of GET servlet requests. */ -class GetServletMethodSink extends DataFlow::ExprNode { - GetServletMethodSink() { +/** Source of GET servlet requests. */ +class GetServletMethodSource extends DataFlow::ExprNode { + GetServletMethodSource() { exists(MethodAccess ma | - isGetServletMethod(ma.getEnclosingCallable()) and - ma.getAnArgument() = this.getExpr() + isGetServletMethod(ma) and + ma = this.getExpr() ) } } @@ -40,11 +40,9 @@ class GetServletMethodSink extends DataFlow::ExprNode { class SensitiveGetQueryConfiguration extends TaintTracking::Configuration { SensitiveGetQueryConfiguration() { this = "SensitiveGetQueryConfiguration" } - override predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof SensitiveInfoExpr - } + override predicate isSource(DataFlow::Node source) { source instanceof GetServletMethodSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof GetServletMethodSink } + override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof SensitiveExpr } } from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveGetQueryConfiguration c diff --git a/java/ql/src/semmle/code/java/frameworks/Servlets.qll b/java/ql/src/semmle/code/java/frameworks/Servlets.qll index b9a71f00838..a7325608a3e 100644 --- a/java/ql/src/semmle/code/java/frameworks/Servlets.qll +++ b/java/ql/src/semmle/code/java/frameworks/Servlets.qll @@ -323,11 +323,20 @@ class ServletWebXMLListenerType extends RefType { } } -/** Holds if `c` is a call to some override of methods of `HttpServlet`, for example `doGet` or `doPost`. */ -predicate isServletMethod(Callable c, string methodName) { - c.getDeclaringType() instanceof ServletClass and - c.getNumberOfParameters() = 2 and - c.getParameter(0).getType() instanceof ServletRequest and - c.getParameter(1).getType() instanceof ServletResponse and - c.getName() = methodName +/** Holds if `ma` is a method access to some override of methods of `HttpServlet`, for example `doGet` or `doPost`. */ +predicate isServletMethod(MethodAccess ma, string methodName) { + exists(Method m | + m = ma.getEnclosingCallable() and + m.getDeclaringType() instanceof ServletClass and + m.getNumberOfParameters() = 2 and + m.getParameter(0).getType() instanceof ServletRequest and + m.getParameter(1).getType() instanceof ServletResponse and + m.getName() = methodName and + ma.getQualifier() = m.getParameter(0).getAnAccess() and + ( + ma.getMethod() instanceof ServletRequestGetParameterMethod or + ma.getMethod() instanceof ServletRequestGetParameterMapMethod or + ma.getMethod() instanceof HttpServletRequestGetQueryStringMethod + ) + ) } diff --git a/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.expected b/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.expected index 238927ac348..41394e6ca72 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.expected @@ -1,7 +1,35 @@ edges -| SensitiveGetQuery.java:12:38:12:45 | password : String | SensitiveGetQuery.java:12:22:12:45 | ... + ... | +| SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | SensitiveGetQuery2.java:14:21:14:48 | (...)... : Object | +| SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | SensitiveGetQuery2.java:14:30:14:48 | get(...) | +| SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | SensitiveGetQuery2.java:14:30:14:48 | get(...) : Object | +| SensitiveGetQuery2.java:14:21:14:48 | (...)... : Object | SensitiveGetQuery2.java:15:29:15:36 | password | +| SensitiveGetQuery2.java:14:21:14:48 | (...)... : Object | SensitiveGetQuery2.java:15:29:15:36 | password : Object | +| SensitiveGetQuery2.java:14:30:14:48 | get(...) : Object | SensitiveGetQuery2.java:14:21:14:48 | (...)... : Object | +| SensitiveGetQuery2.java:15:29:15:36 | password : Object | SensitiveGetQuery2.java:18:40:18:54 | password : Object | +| SensitiveGetQuery2.java:18:40:18:54 | password : Object | SensitiveGetQuery2.java:19:61:19:68 | password | +| SensitiveGetQuery.java:12:21:12:52 | getParameter(...) : String | SensitiveGetQuery.java:14:29:14:36 | password | +| SensitiveGetQuery.java:12:21:12:52 | getParameter(...) : String | SensitiveGetQuery.java:14:29:14:36 | password : String | +| SensitiveGetQuery.java:14:29:14:36 | password : String | SensitiveGetQuery.java:17:40:17:54 | password : String | +| SensitiveGetQuery.java:17:40:17:54 | password : String | SensitiveGetQuery.java:18:61:18:68 | password | nodes -| SensitiveGetQuery.java:12:22:12:45 | ... + ... | semmle.label | ... + ... | -| SensitiveGetQuery.java:12:38:12:45 | password : String | semmle.label | password : String | +| SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | semmle.label | getParameterMap(...) : Map | +| SensitiveGetQuery2.java:14:21:14:48 | (...)... : Object | semmle.label | (...)... : Object | +| SensitiveGetQuery2.java:14:30:14:48 | get(...) | semmle.label | get(...) | +| SensitiveGetQuery2.java:14:30:14:48 | get(...) : Object | semmle.label | get(...) : Object | +| SensitiveGetQuery2.java:15:29:15:36 | password | semmle.label | password | +| SensitiveGetQuery2.java:15:29:15:36 | password : Object | semmle.label | password : Object | +| SensitiveGetQuery2.java:18:40:18:54 | password : Object | semmle.label | password : Object | +| SensitiveGetQuery2.java:19:61:19:68 | password | semmle.label | password | +| SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | semmle.label | getParameter(...) | +| SensitiveGetQuery.java:12:21:12:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| SensitiveGetQuery.java:14:29:14:36 | password | semmle.label | password | +| SensitiveGetQuery.java:14:29:14:36 | password : String | semmle.label | password : String | +| SensitiveGetQuery.java:17:40:17:54 | password : String | semmle.label | password : String | +| SensitiveGetQuery.java:18:61:18:68 | password | semmle.label | password | #select -| SensitiveGetQuery.java:12:22:12:45 | ... + ... | SensitiveGetQuery.java:12:38:12:45 | password : String | SensitiveGetQuery.java:12:22:12:45 | ... + ... | $@ uses GET request method with sensitive information. | SensitiveGetQuery.java:12:38:12:45 | password | sensitive query string | +| SensitiveGetQuery2.java:14:30:14:48 | get(...) | SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | SensitiveGetQuery2.java:14:30:14:48 | get(...) | $@ uses GET request method with sensitive information. | SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) | sensitive query string | +| SensitiveGetQuery2.java:15:29:15:36 | password | SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | SensitiveGetQuery2.java:15:29:15:36 | password | $@ uses GET request method with sensitive information. | SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) | sensitive query string | +| SensitiveGetQuery2.java:19:61:19:68 | password | SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) : Map | SensitiveGetQuery2.java:19:61:19:68 | password | $@ uses GET request method with sensitive information. | SensitiveGetQuery2.java:12:13:12:37 | getParameterMap(...) | sensitive query string | +| SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | $@ uses GET request method with sensitive information. | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | sensitive query string | +| SensitiveGetQuery.java:14:29:14:36 | password | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) : String | SensitiveGetQuery.java:14:29:14:36 | password | $@ uses GET request method with sensitive information. | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | sensitive query string | +| SensitiveGetQuery.java:18:61:18:68 | password | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) : String | SensitiveGetQuery.java:18:61:18:68 | password | $@ uses GET request method with sensitive information. | SensitiveGetQuery.java:12:21:12:52 | getParameter(...) | sensitive query string | diff --git a/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.java b/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.java index 3e5ce6ef3bd..5443d44c19c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.java +++ b/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery.java @@ -8,8 +8,14 @@ import javax.servlet.ServletException; public class SensitiveGetQuery extends HttpServlet { // BAD - Tests sending sensitive information in a GET request. public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { + String username = request.getParameter("username"); String password = request.getParameter("password"); - System.out.println("password = " + password); + + processUserInfo(username, password); + } + + void processUserInfo(String username, String password) { + System.out.println("username = " + username+"; password "+password); } // GOOD - Tests sending sensitive information in a POST request. diff --git a/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery2.java b/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery2.java new file mode 100644 index 00000000000..b7dc8d8976f --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-598/SensitiveGetQuery2.java @@ -0,0 +1,29 @@ +import java.io.IOException; +import java.util.Map; + +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.ServletException; + +public class SensitiveGetQuery2 extends HttpServlet { + // BAD - Tests sending sensitive information in a GET request. + public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { + Map map = request.getParameterMap(); + String username = (String) map.get("username"); + String password = (String) map.get("password"); + processUserInfo(username, password); + } + + void processUserInfo(String username, String password) { + System.out.println("username = " + username+"; password "+password); + } + + // GOOD - Tests sending sensitive information in a POST request. + public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { + Map map = request.getParameterMap(); + String username = (String) map.get("username"); + String password = (String) map.get("password"); + processUserInfo(username, password); + } +}