From 0c6925399d76ad479fb71c8c3fe0cb47595661fc Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 16 Dec 2024 18:44:55 -0500 Subject: [PATCH 01/38] Java: add qhelp --- .../CWE-352/CsrfUnprotectedRequestType.qhelp | 43 +++++++++++++++++++ .../CsrfUnprotectedRequestTypeBad.java | 5 +++ .../CsrfUnprotectedRequestTypeGood.java | 5 +++ 3 files changed, 53 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeBad.java create mode 100644 java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeGood.java diff --git a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.qhelp b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.qhelp new file mode 100644 index 000000000000..7517fd5c7346 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.qhelp @@ -0,0 +1,43 @@ + + + + +

When you set up a web server to receive a request from a client without any mechanism +for verifying that it was intentionally sent, then it is vulnerable to attack. An attacker can +trick a client into making an unintended request to the web server that will be treated as +an authentic request. This can be done via a URL, image load, XMLHttpRequest, etc. and can +result in exposure of data or unintended code execution.

+ + + +

When handling requests, make sure any requests that change application state are protected from +Cross Site Request Forgery (CSRF). Some application frameworks, such as Spring, provide default CSRF +protection for HTTP request types that may change application state, such as POST. Other HTTP request +types, such as GET, should not be used for actions that change the state of the application, since these +request types are not default-protected from CSRF by the framework.

+ + + +

The following example shows a Spring request handler using a GET request for a state-changing action. +Since a GET request does not have default CSRF protection in Spring, this type of request should +not be used when modifying application state. Instead use one of Spring's default-protected request +types, such as POST.

+ + + + + + + +
  • +OWASP: +Cross-Site Request Forgery (CSRF). +
    • +
  • +Spring Security Reference: + + Cross Site Request Forgery (CSRF) +. +
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeBad.java b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeBad.java new file mode 100644 index 000000000000..c79d15cd828c --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeBad.java @@ -0,0 +1,5 @@ +// BAD - a GET request should not be used for a state-changing action like transfer +@RequestMapping(value="transfer", method=RequestMethod.GET) +public boolean transfer(HttpServletRequest request, HttpServletResponse response){ + return doTransfer(request, response); +} diff --git a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeGood.java b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeGood.java new file mode 100644 index 000000000000..40b54a800e12 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeGood.java @@ -0,0 +1,5 @@ +// GOOD - use a POST request for a state-changing action +@RequestMapping(value="transfer", method=RequestMethod.POST) +public boolean transfer(HttpServletRequest request, HttpServletResponse response){ + return doTransfer(request, response); +} From 506d6682895e9e1c3b519435d91520e45206602d Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 26 Nov 2024 20:58:49 -0500 Subject: [PATCH 02/38] Java: add class for Spring request mapping methods that are not default-protected from CSRF --- .../CsrfUnprotectedRequestTypeQuery.qll | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll diff --git a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll new file mode 100644 index 000000000000..697461a30f1b --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll @@ -0,0 +1,29 @@ +/** Provides classes and predicates to reason about CSRF vulnerabilities due to use of unprotected HTTP request types. */ + +import java +private import semmle.code.java.frameworks.spring.SpringController + +/** A method that is not protected from CSRF by default. */ +abstract class CsrfUnprotectedMethod extends Method { } + +/** + * A Spring request mapping method that is not protected from CSRF by default. + * + * https://docs.spring.io/spring-security/reference/features/exploits/csrf.html#csrf-protection-read-only + */ +private class SpringCsrfUnprotectedMethod extends CsrfUnprotectedMethod instanceof SpringRequestMappingMethod +{ + SpringCsrfUnprotectedMethod() { + this.hasAnnotation("org.springframework.web.bind.annotation", "GetMapping") + or + this.hasAnnotation("org.springframework.web.bind.annotation", "RequestMapping") and + ( + this.getAnAnnotation().getAnEnumConstantArrayValue("method").getName() = + ["GET", "HEAD", "OPTIONS", "TRACE"] + or + // If no request type is specified with `@RequestMapping`, then all request types + // are possible, so we treat this as unsafe; example: @RequestMapping(value = "test"). + not exists(this.getAnAnnotation().getAnArrayValue("method")) + ) + } +} From 8e9f21dc52228965019ae66863f7345903cf5324 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 26 Nov 2024 21:14:32 -0500 Subject: [PATCH 03/38] Java: add a class for MyBatis Mapper methods that update a database --- .../CsrfUnprotectedRequestTypeQuery.qll | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll index 697461a30f1b..e33502174821 100644 --- a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll @@ -27,3 +27,20 @@ private class SpringCsrfUnprotectedMethod extends CsrfUnprotectedMethod instance ) } } + +/** A method that updates a database. */ +abstract class DatabaseUpdateMethod extends Method { } + +/** A MyBatis Mapper method that updates a database. */ +private class MyBatisMapperDatabaseUpdateMethod extends DatabaseUpdateMethod { + MyBatisMapperDatabaseUpdateMethod() { + exists(MyBatisMapperSqlOperation mapperXml | + ( + mapperXml instanceof MyBatisMapperInsert or + mapperXml instanceof MyBatisMapperUpdate or + mapperXml instanceof MyBatisMapperDelete + ) and + this = mapperXml.getMapperMethod() + ) + } +} From b88731df8004d42c0ebe996aa1d6bc8445c0123d Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 26 Nov 2024 21:18:40 -0500 Subject: [PATCH 04/38] Java: move contents of MyBatisMapperXML.qll in src to MyBatis.qll in lib so importable, and fix experimental files broken by the move --- .../semmle/code/java/frameworks/MyBatis.qll | 111 ++++++++++++++++++ .../CsrfUnprotectedRequestTypeQuery.qll | 1 + .../Security/CWE/CWE-089/MyBatisCommonLib.qll | 1 - .../MyBatisMapperXmlSqlInjectionLib.qll | 2 +- 4 files changed, 113 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll index 01c8b829de6b..c7fc09a33b4d 100644 --- a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll +++ b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll @@ -128,3 +128,114 @@ private class MyBatisProviderStep extends TaintTracking::AdditionalValueStep { ) } } + +/** + * A MyBatis Mapper XML file. + */ +class MyBatisMapperXmlFile extends XmlFile { + MyBatisMapperXmlFile() { + count(XmlElement e | e = this.getAChild()) = 1 and + this.getAChild().getName() = "mapper" + } +} + +/** + * An XML element in a `MyBatisMapperXMLFile`. + */ +class MyBatisMapperXmlElement extends XmlElement { + MyBatisMapperXmlElement() { this.getFile() instanceof MyBatisMapperXmlFile } + + /** + * Gets the value for this element, with leading and trailing whitespace trimmed. + */ + string getValue() { result = this.allCharactersString().trim() } + + /** + * Gets the reference type bound to MyBatis Mapper XML File. + */ + RefType getNamespaceRefType() { + result.getQualifiedName() = this.getAttribute("namespace").getValue() + } +} + +/** + * An MyBatis Mapper sql operation element. + */ +abstract class MyBatisMapperSqlOperation extends MyBatisMapperXmlElement { + /** + * Gets the value of the `id` attribute of MyBatis Mapper sql operation element. + */ + string getId() { result = this.getAttribute("id").getValue() } + + /** + * Gets the `` element in a `MyBatisMapperSqlOperation`. + */ + MyBatisMapperInclude getInclude() { result = this.getAChild*() } + + /** + * Gets the method bound to MyBatis Mapper XML File. + */ + Method getMapperMethod() { + result.getName() = this.getId() and + result.getDeclaringType() = this.getParent().(MyBatisMapperXmlElement).getNamespaceRefType() + } +} + +/** + * A `` element in a `MyBatisMapperSqlOperation`. + */ +class MyBatisMapperInsert extends MyBatisMapperSqlOperation { + MyBatisMapperInsert() { this.getName() = "insert" } +} + +/** + * A `` element in a `MyBatisMapperSqlOperation`. + */ +class MyBatisMapperUpdate extends MyBatisMapperSqlOperation { + MyBatisMapperUpdate() { this.getName() = "update" } +} + +/** + * A `` element in a `MyBatisMapperSqlOperation`. + */ +class MyBatisMapperDelete extends MyBatisMapperSqlOperation { + MyBatisMapperDelete() { this.getName() = "delete" } +} + +/** + * A `