Merge pull request github#13413 from egregius313/egregius313/trust-boundary

egregius313 · web-flow · commit d109637e2d7a · 2023-08-18T10:33:32.000-04:00
Java: Trust Boundary Violation Query
diff --git a/docs/codeql/codeql-language-guides/customizing-library-models-for-java.rst b/docs/codeql/codeql-language-guides/customizing-library-models-for-java.rst
@@ -349,6 +349,7 @@ The following sink kinds are supported:
 - **response-splitting**: A sink that can be used for HTTP response splitting, such as in calls to **HttpServletResponse.setHeader**.
 - **sql-injection**: A sink that can be used for SQL injection, such as in a **Statement.executeQuery** call.
 - **template-injection**: A sink that can be used for server side template injection, such as in a **Velocity.evaluate** call.
+- **trust-boundary-violation**: A sink that can be used to cross a trust boundary, such as in a **HttpSession.setAttribute** call.
 - **url-redirection**: A sink that can be used to redirect the user to a malicious URL, such as in a **Response.temporaryRedirect** call.
 - **xpath-injection**: A sink that can be used for XPath injection, such as in a **XPath.evaluate** call.
 - **xslt-injection**: A sink that can be used for XSLT injection, such as in a **Transformer.transform** call.
diff --git a/java/ql/lib/ext/generated/org.apache.commons.lang.model.yml b/java/ql/lib/ext/generated/org.apache.commons.lang.model.yml
@@ -1604,8 +1604,6 @@ extensions:
       - ["org.apache.commons.lang", "SerializationUtils", "serialize", "(Serializable,OutputStream)", "summary", "df-generated"]
       - ["org.apache.commons.lang", "StringEscapeUtils", "escapeCsv", "(String)", "summary", "df-generated"]
       - ["org.apache.commons.lang", "StringEscapeUtils", "escapeCsv", "(Writer,String)", "summary", "df-generated"]
-      - ["org.apache.commons.lang", "StringEscapeUtils", "escapeHtml", "(String)", "summary", "df-generated"]
-      - ["org.apache.commons.lang", "StringEscapeUtils", "escapeHtml", "(Writer,String)", "summary", "df-generated"]
       - ["org.apache.commons.lang", "StringEscapeUtils", "escapeJava", "(String)", "summary", "df-generated"]
       - ["org.apache.commons.lang", "StringEscapeUtils", "escapeJava", "(Writer,String)", "summary", "df-generated"]
       - ["org.apache.commons.lang", "StringEscapeUtils", "escapeJavaScript", "(String)", "summary", "df-generated"]
diff --git a/java/ql/lib/ext/javax.servlet.http.model.yml b/java/ql/lib/ext/javax.servlet.http.model.yml
@@ -26,6 +26,8 @@ extensions:
       - ["javax.servlet.http", "HttpServletResponse", False, "addHeader", "", "", "Argument[0..1]", "response-splitting", "manual"]
       - ["javax.servlet.http", "HttpServletResponse", False, "sendError", "(int,String)", "", "Argument[1]", "information-leak", "manual"]
       - ["javax.servlet.http", "HttpServletResponse", False, "setHeader", "", "", "Argument[0..1]", "response-splitting", "manual"]
+      - ["javax.servlet.http", "HttpSession", True, "putValue", "", "", "Argument[0..1]", "trust-boundary-violation", "manual"]
+      - ["javax.servlet.http", "HttpSession", True, "setAttribute", "", "", "Argument[0..1]", "trust-boundary-violation", "manual"]
   - addsTo:
       pack: codeql/java-all
       extensible: summaryModel
diff --git a/java/ql/lib/ext/org.apache.commons.lang.model.yml b/java/ql/lib/ext/org.apache.commons.lang.model.yml
@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["org.apache.commons.lang", "StringEscapeUtils", true, "escapeHtml", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["org.apache.commons.lang", "StringEscapeUtils", true, "escapeHtml", "(Writer,String)", "", "Argument[1]", "Argument[0]", "taint", "manual"]
diff --git a/java/ql/lib/ext/org.apache.struts2.dispatcher.model.yml b/java/ql/lib/ext/org.apache.struts2.dispatcher.model.yml
@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+    - ["org.apache.struts2.dispatcher", "SessionMap", False, "put", "", "", "Argument[0..1]", "trust-boundary-violation", "manual"]
diff --git a/java/ql/lib/ext/org.apache.struts2.interceptor.model.yml b/java/ql/lib/ext/org.apache.struts2.interceptor.model.yml
@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+    - ["org.apache.struts2.interceptor", "SessionAware", False, "setSession", "", "", "Argument[0]", "trust-boundary-violation", "manual"]
+    - ["org.apache.struts2.interceptor", "SessionAware", False, "withSession", "", "", "Argument[0]", "trust-boundary-violation", "manual"]
diff --git a/java/ql/lib/ext/org.owasp.esapi.model.yml b/java/ql/lib/ext/org.owasp.esapi.model.yml
@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
diff --git a/java/ql/lib/ext/play.mvc.model.yml b/java/ql/lib/ext/play.mvc.model.yml
@@ -16,6 +16,11 @@ extensions:
       - ["play.mvc", "Http$RequestHeader", True, "queryString", "", "", "ReturnValue", "remote", "manual"]
       - ["play.mvc", "Http$RequestHeader", True, "remoteAddress", "", "", "ReturnValue", "remote", "manual"]
       - ["play.mvc", "Http$RequestHeader", True, "uri", "", "", "ReturnValue", "remote", "manual"]
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["play.mvc", "Result", False, "addingToSession", "", "", "Argument[1..2]", "trust-boundary-violation", "manual"]
   - addsTo:
       pack: codeql/java-all
       extensible: summaryModel
diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll
@@ -397,3 +397,8 @@ class GetServletResourceAsStreamMethod extends Method {
     this.hasName("getResourceAsStream")
   }
 }
+
+/** The interface `javax.servlet.http.HttpSession` */
+class HttpServletSession extends RefType {
+  HttpServletSession() { this.hasQualifiedName("javax.servlet.http", "HttpSession") }
+}
diff --git a/java/ql/lib/semmle/code/java/frameworks/owasp/Esapi.qll b/java/ql/lib/semmle/code/java/frameworks/owasp/Esapi.qll
@@ -0,0 +1,40 @@
+/** Classes and predicates for reasoning about the `owasp.easpi` package. */
+
+import java
+
+/**
+ * The `org.owasp.esapi.Validator` interface.
+ */
+class EsapiValidator extends RefType {
+  EsapiValidator() { this.hasQualifiedName("org.owasp.esapi", "Validator") }
+}
+
+/**
+ * The methods of `org.owasp.esapi.Validator` which validate data.
+ */
+class EsapiIsValidMethod extends Method {
+  EsapiIsValidMethod() {
+    this.getDeclaringType() instanceof EsapiValidator and
+    this.hasName([
+        "isValidCreditCard", "isValidDate", "isValidDirectoryPath", "isValidDouble",
+        "isValidFileContent", "isValidFileName", "isValidInput", "isValidInteger",
+        "isValidListItem", "isValidNumber", "isValidPrintable", "isValidRedirectLocation",
+        "isValidSafeHTML", "isValidURI"
+      ])
+  }
+}
+
+/**
+ * The methods of `org.owasp.esapi.Validator` which return validated data.
+ */
+class EsapiGetValidMethod extends Method {
+  EsapiGetValidMethod() {
+    this.getDeclaringType() instanceof EsapiValidator and
+    this.hasName([
+        "getValidCreditCard", "getValidDate", "getValidDirectoryPath", "getValidDouble",
+        "getValidFileContent", "getValidFileName", "getValidInput", "getValidInteger",
+        "getValidListItem", "getValidNumber", "getValidPrintable", "getValidRedirectLocation",
+        "getValidSafeHTML", "getValidURI"
+      ])
+  }
+}
diff --git a/java/ql/lib/semmle/code/java/security/TrustBoundaryViolationQuery.qll b/java/ql/lib/semmle/code/java/security/TrustBoundaryViolationQuery.qll
@@ -0,0 +1,70 @@
+/** Provides classes and predicates to reason about trust boundary violations */
+
+import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.controlflow.Guards
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.frameworks.owasp.Esapi
+
+/**
+ * A source of data that crosses a trust boundary.
+ */
+abstract class TrustBoundaryViolationSource extends DataFlow::Node { }
+
+private class RemoteSource extends TrustBoundaryViolationSource instanceof RemoteFlowSource { }
+
+/**
+ * A sink for data that crosses a trust boundary.
+ */
+class TrustBoundaryViolationSink extends DataFlow::Node {
+  TrustBoundaryViolationSink() { sinkNode(this, "trust-boundary-violation") }
+}
+
+/**
+ * A sanitizer for data that crosses a trust boundary.
+ */
+abstract class TrustBoundaryValidationSanitizer extends DataFlow::Node { }
+
+/**
+ * A node validated by an OWASP ESAPI validation method.
+ */
+private class EsapiValidatedInputSanitizer extends TrustBoundaryValidationSanitizer {
+  EsapiValidatedInputSanitizer() {
+    this = DataFlow::BarrierGuard<esapiIsValidData/3>::getABarrierNode() or
+    this.asExpr().(MethodAccess).getMethod() instanceof EsapiGetValidMethod
+  }
+}
+
+/**
+ * Holds if `g` is a guard that checks that `e` is valid data according to an OWASP ESAPI validation method.
+ */
+private predicate esapiIsValidData(Guard g, Expr e, boolean branch) {
+  branch = true and
+  exists(MethodAccess ma | ma.getMethod() instanceof EsapiIsValidMethod |
+    g = ma and
+    e = ma.getArgument(1)
+  )
+}
+
+/**
+ * Taint tracking for data that crosses a trust boundary.
+ */
+module TrustBoundaryConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof TrustBoundaryViolationSource }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof TrustBoundaryValidationSanitizer or
+    node.getType() instanceof HttpServletSession or
+    node.getType() instanceof NumberType or
+    node.getType() instanceof PrimitiveType or
+    node.getType() instanceof BoxedType
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof TrustBoundaryViolationSink }
+}
+
+/**
+ * Taint-tracking flow for values which cross a trust boundary.
+ */
+module TrustBoundaryFlow = TaintTracking::Global<TrustBoundaryConfig>;
diff --git a/java/ql/src/Security/CWE/CWE-501/TrustBoundaryFixed.java b/java/ql/src/Security/CWE/CWE-501/TrustBoundaryFixed.java
@@ -0,0 +1,8 @@
+public void doGet(HttpServletRequest request, HttpServletResponse response) {
+    String username = request.getParameter("username");
+
+    if (validator.isValidInput("HTTP parameter", username, "username", 20, false)) {
+        // GOOD: The input is sanitized before being written to the session.
+        request.getSession().setAttribute("username", username);
+    }
+}
diff --git a/java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.qhelp b/java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.qhelp
@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+ "-//Semmle//qhelp//EN"
+ "qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      A trust boundary violation occurs when a value is passed from a less trusted context to a more trusted context.
+    </p>
+    
+    <p>
+      For example, a value that is generated by a less trusted source, such as a user, may be passed to a more trusted
+      source, such as a system process. If the less trusted source is malicious, then the value may be crafted to
+      exploit the more trusted source.
+    </p>
+
+    <p>
+      Trust boundary violations are often caused by a failure to validate input. For example, if a web application
+      accepts a cookie from a user, then the application should validate the cookie before using it. If the cookie is
+      not validated, then the user may be able to craft a malicious cookie that exploits the application.
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      To maintain a trust boundary, validate data from less trusted sources before use.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      In the first (bad) example, the server accepts a parameter from the user, then uses it to set the username without validation.
+    </p>
+    <sample src="TrustBoundaryVulnerable.java" />
+
+    <p>
+      In the second (good) example, the server validates the parameter from the user, then uses it to set the username.
+    </p>
+    <sample src="TrustBoundaryFixed.java" />
+
+  </example>
+
+  <references>
+    <li>
+      Wikipedia: <a href="http://en.wikipedia.org/wiki/Trust_boundary">Trust boundary</a>.
+    </li>
+  </references>
+
+</qhelp>
diff --git a/java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.ql b/java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.ql
@@ -0,0 +1,20 @@
+/**
+ * @id java/trust-boundary-violation
+ * @name Trust boundary violation
+ * @description Modifying the HTTP session attributes based on data from an untrusted source may violate a trust boundary.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 8.8
+ * @precision medium
+ * @tags security
+ *      external/cwe/cwe-501
+ */
+
+import java
+import semmle.code.java.security.TrustBoundaryViolationQuery
+import TrustBoundaryFlow::PathGraph
+
+from TrustBoundaryFlow::PathNode source, TrustBoundaryFlow::PathNode sink
+where TrustBoundaryFlow::flowPath(source, sink)
+select sink.getNode(), sink, source,
+  "This servlet reads data from a remote source and writes it to a session variable."
diff --git a/java/ql/src/Security/CWE/CWE-501/TrustBoundaryVulnerable.java b/java/ql/src/Security/CWE/CWE-501/TrustBoundaryVulnerable.java
@@ -0,0 +1,6 @@
+public void doGet(HttpServletRequest request, HttpServletResponse response) {
+    String username = request.getParameter("username");
+
+    // BAD: The input is written to the session without being sanitized.
+    request.getSession().setAttribute("username", username);
+}
diff --git a/java/ql/src/change-notes/2023-07-25-trust-boundary-violation-query.md b/java/ql/src/change-notes/2023-07-25-trust-boundary-violation-query.md
@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+* Added the `java/trust-boundary-violation` query to detect trust boundary violations between HTTP requests and the HTTP session. Also added the `trust-boundary-violation` sink kind for sinks which may cross a trust boundary, such as calls to the `HttpSession#setAttribute` method.
+
diff --git a/java/ql/test/query-tests/security/CWE-501/TrustBoundaryViolations.expected b/java/ql/test/query-tests/security/CWE-501/TrustBoundaryViolations.expected
@@ -0,0 +1,2 @@
+failures
+testFailures
diff --git a/java/ql/test/query-tests/security/CWE-501/TrustBoundaryViolations.java b/java/ql/test/query-tests/security/CWE-501/TrustBoundaryViolations.java
@@ -0,0 +1,35 @@
+import java.io.IOException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.owasp.esapi.Validator;
+
+public class TrustBoundaryViolations extends HttpServlet {
+    Validator validator;
+
+    public void doGet(HttpServletRequest request, HttpServletResponse response) {
+        String input = request.getParameter("input");
+
+        // BAD: The input is written to the session without being sanitized.
+        request.getSession().setAttribute("input", input); // $ hasTaintFlow
+
+        String input2 = request.getParameter("input2");
+
+        try {
+            String sanitized = validator.getValidInput("HTTP parameter", input2, "HTTPParameterValue", 100, false);
+            // GOOD: The input is sanitized before being written to the session.
+            request.getSession().setAttribute("input2", sanitized);
+
+        } catch (Exception e) {
+        }
+
+        try {
+            String input3 = request.getParameter("input3");
+            if (validator.isValidInput("HTTP parameter", input3, "HTTPParameterValue", 100, false)) {
+                // GOOD: The input is sanitized before being written to the session.
+                request.getSession().setAttribute("input3", input3);
+            }
+        } catch (Exception e) {
+        }
+    }
+}
diff --git a/java/ql/test/query-tests/security/CWE-501/TrustBoundaryViolations.ql b/java/ql/test/query-tests/security/CWE-501/TrustBoundaryViolations.ql
@@ -0,0 +1,4 @@
+import java
+import semmle.code.java.security.TrustBoundaryViolationQuery
+import TestUtilities.InlineFlowTest
+import TaintFlowTest<TrustBoundaryConfig>
diff --git a/java/ql/test/query-tests/security/CWE-501/options b/java/ql/test/query-tests/security/CWE-501/options
@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/esapi-2.0.1:${testdir}/../../../stubs/javax-servlet-2.5
diff --git a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/ValidationErrorList.java b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/ValidationErrorList.java
diff --git a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Validator.java b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Validator.java
diff --git a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/errors/IntrusionException.java b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/errors/IntrusionException.java
diff --git a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/errors/ValidationException.java b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/errors/ValidationException.java
diff --git a/shared/mad/codeql/mad/ModelValidation.qll b/shared/mad/codeql/mad/ModelValidation.qll

Original file line number	Diff line number	Diff line change
`@@ -397,3 +397,8 @@ class GetServletResourceAsStreamMethod extends Method {`
`397`	`397`	`this.hasName("getResourceAsStream")`
`398`	`398`	`}`
`399`	`399`	`}`
	`400`	`+`
	`401`	+/** The interface `javax.servlet.http.HttpSession` */
	`402`	`+class HttpServletSession extends RefType {`
	`403`	`+ HttpServletSession() { this.hasQualifiedName("javax.servlet.http", "HttpSession") }`
	`404`	`+}`