From 39ccde0c9dffe6d493d5b7962c2e9e4abe6f47f8 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Sun, 15 Dec 2024 20:53:27 -0500 Subject: [PATCH] Java: add name-based heuristic --- .../CsrfUnprotectedRequestTypeQuery.qll | 67 ++++++++++++++----- .../CWE/CWE-352/CsrfUnprotectedRequestType.ql | 8 +-- .../CsrfUnprotectedRequestTypeTest.java | 6 ++ .../CWE-352/CsrfUnprotectedRequestTypeTest.ql | 4 +- 4 files changed, 62 insertions(+), 23 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll index a4633ae451d..a5bd49bc62c 100644 --- a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll @@ -49,6 +49,15 @@ private class StaplerCsrfUnprotectedMethod extends CsrfUnprotectedMethod instanc } } +/** A method that appears to change application state based on its name. */ +private class NameStateChangeMethod extends Method { + NameStateChangeMethod() { + this.getName() + .regexpMatch("(^|\\w+(?=[A-Z]))((?i)post|put|patch|delete|remove|create|add|update|edit|(un|)publish|fill|move|transfer|log(out|in)|access|connect(|ion)|register|submit)($|(?![a-z])\\w+)") and + not this.getName().regexpMatch("^(get|show|view|list|query|find)(?![a-z])\\w*") + } +} + /** A method that updates a database. */ abstract class DatabaseUpdateMethod extends Method { } @@ -163,20 +172,46 @@ module CallGraph { import CallGraph -/** Holds if `src` is an unprotected request handler that reaches a state-changing `sink`. */ -predicate unprotectedStateChange(CallPathNode src, CallPathNode sink, CallPathNode sinkPred) { - src.asMethod() instanceof CsrfUnprotectedMethod and - sink.asMethod() instanceof DatabaseUpdateMethod and - sinkPred.getASuccessor() = sink and - src.getASuccessor+() = sinkPred and - if - sink.asMethod() instanceof SqlInjectionMethod and - sink.asMethod().hasName("execute") - then - exists(SqlExecuteFlow::PathNode executeSrc, SqlExecuteFlow::PathNode executeSink | - SqlExecuteFlow::flowPath(executeSrc, executeSink) - | - sinkPred.asCall() = executeSink.getNode().asExpr().(Argument).getCall() - ) - else any() +/** + * Holds if `src` is an unprotected request handler that reaches a + * `sink` that updates a database. + */ +predicate unprotectedDatabaseUpdate(CallPathNode sourceMethod, CallPathNode sinkMethodCall) { + sourceMethod.asMethod() instanceof CsrfUnprotectedMethod and + exists(CallPathNode sinkMethod | + sinkMethod.asMethod() instanceof DatabaseUpdateMethod and + sinkMethodCall.getASuccessor() = sinkMethod and + sourceMethod.getASuccessor+() = sinkMethodCall and + if + sinkMethod.asMethod() instanceof SqlInjectionMethod and + sinkMethod.asMethod().hasName("execute") + then + exists(SqlExecuteFlow::PathNode executeSrc, SqlExecuteFlow::PathNode executeSink | + SqlExecuteFlow::flowPath(executeSrc, executeSink) + | + sinkMethodCall.asCall() = executeSink.getNode().asExpr().(Argument).getCall() + ) + else any() + ) +} + +/** + * Holds if `src` is an unprotected request handler that appears to + * change application state based on its name. + */ +private predicate unprotectedHeuristicStateChange(CallPathNode sourceMethod, CallPathNode sinkMethod) { + sourceMethod.asMethod() instanceof CsrfUnprotectedMethod and + sinkMethod.asMethod() instanceof NameStateChangeMethod and + sinkMethod = sourceMethod and + // exclude any alerts that update a database + not unprotectedDatabaseUpdate(sourceMethod, _) +} + +/** + * Holds if `src` is an unprotected request handler that may + * change an application's state. + */ +predicate unprotectedStateChange(CallPathNode source, CallPathNode sink) { + unprotectedDatabaseUpdate(source, sink) or + unprotectedHeuristicStateChange(source, sink) } diff --git a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql index db19c6efc85..9caf2903007 100644 --- a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql +++ b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql @@ -17,8 +17,8 @@ import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery query predicate edges(CallPathNode pred, CallPathNode succ) { CallGraph::edges(pred, succ) } -from CallPathNode source, CallPathNode reachable, CallPathNode callsReachable -where unprotectedStateChange(source, reachable, callsReachable) -select source.asMethod(), source, callsReachable, +from CallPathNode source, CallPathNode sink +where unprotectedStateChange(source, sink) +select source.asMethod(), source, sink, "Potential CSRF vulnerability due to using an HTTP request type which is not default-protected from CSRF for an apparent $@.", - callsReachable, "state-changing action" + sink, "state-changing action" diff --git a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java index ec2732d6aa0..cc581cfb222 100644 --- a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java +++ b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java @@ -211,4 +211,10 @@ public class CsrfUnprotectedRequestTypeTest { public void bad10(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType myBatisService.bad10(user); } + + // BAD: method name implies a state-change + @GetMapping(value = "delete") + public String delete(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType + return "delete"; + } } diff --git a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql index 94ecf55516e..6da418767ee 100644 --- a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql +++ b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql @@ -7,9 +7,7 @@ module CsrfUnprotectedRequestTypeTest implements TestSig { predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasCsrfUnprotectedRequestType" and - exists(CallPathNode src, CallPathNode sink, CallPathNode sinkPred | - unprotectedStateChange(src, sink, sinkPred) - | + exists(CallPathNode src, CallPathNode sink | unprotectedStateChange(src, sink) | src.getLocation() = location and element = src.toString() and value = ""