}
+ * * @return {!Array<string>}
* */
* Object.keys = function(obj) {};
*
@@ -214,7 +214,7 @@ class ExternalGlobalVarDecl extends ExternalGlobalDecl, VariableDeclarator {
*
* /**
* * @param {!Object} obj
- * * @return {!Array}
+ * * @return {!Array<string>}
* */
* Object.keys = function(obj) {};
*
@@ -273,7 +273,7 @@ class ExternalMemberDecl extends ExternalVarDecl, ExprStmt {
*
* /**
* * @param {!Object} obj
- * * @return {!Array}
+ * * @return {!Array<string>}
* */
* Object.keys = function(obj) {};
*
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index f46570b7434..676d4a41f10 100644
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -273,7 +273,7 @@ module UnsafeShellCommandConstruction {
}
/**
- * A guard of the form `typeof x === ""`, where is "number", or "boolean",
+ * A guard of the form `typeof x === ""`, where `` is "number", or "boolean",
* which sanitizes `x` in its "then" branch.
*/
class TypeOfSanitizer extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
From 834f2e845df193e16d9e4ca134157684a61432a5 Mon Sep 17 00:00:00 2001
From: Jorge <46056498+jorgectf@users.noreply.github.com>
Date: Thu, 28 Apr 2022 21:55:15 +0200
Subject: [PATCH 0196/1618] Delete `MyBatisAbstractSql` and inline
`MyBatisAbstractSqlMethodsStep`
---
.../semmle/code/java/frameworks/MyBatis.qll | 100 +++++++++++-------
1 file changed, 61 insertions(+), 39 deletions(-)
diff --git a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
index e2fb5b3707f..691fa0d5382 100644
--- a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
@@ -105,10 +105,6 @@ class TypeParam extends Interface {
TypeParam() { this.hasQualifiedName("org.apache.ibatis.annotations", "Param") }
}
-private class MyBatisAbstractSql extends RefType {
- MyBatisAbstractSql() { this.hasQualifiedName("org.apache.ibatis.jdbc", "AbstractSQL") }
-}
-
private class MyBatisProvider extends RefType {
MyBatisProvider() {
this.hasQualifiedName("org.apache.ibatis.annotations",
@@ -129,7 +125,7 @@ class MyBatisInjectionSink extends DataFlow::Node {
a.getType() instanceof MyBatisProvider and
m.getDeclaringType() = a.getValue(["type", "value"]).(TypeLiteral).getTypeName().getType() and
m.hasName(a.getValue("method").(StringLiteral).getValue()) and
- this.asExpr() = m.getBody().getAStmt().(ReturnStmt).getResult()
+ this.asExpr() = m.getBody().getAStmt().(ReturnStmt).getEnclosingCallable()
)
}
}
@@ -157,41 +153,67 @@ private class MyBatisAbstractSqlToStringStep extends SummaryModelCsv {
}
}
-private class MyBatisAbstractSqlMethod extends string {
- string taintedArgs;
- string signature;
-
- MyBatisAbstractSqlMethod() {
- this in [
- "UPDATE", "SET", "INSERT_INTO", "SELECT", "OFFSET_ROWS", "LIMIT", "OFFSET",
- "FETCH_FIRST_ROWS_ONLY", "DELETE_FROM", "INNER_JOIN", "ORDER_BY", "WHERE", "HAVING",
- "OUTER_JOIN", "LEFT_OUTER_JOIN", "RIGHT_OUTER_JOIN", "GROUP_BY", "FROM", "SELECT_DISTINCT"
- ] and
- taintedArgs = "Argument[0]" and
- signature = "String"
- or
- this in [
- "SET", "INTO_COLUMNS", "INTO_VALUES", "SELECT_DISTINCT", "FROM", "JOIN", "INNER_JOIN",
- "LEFT_OUTER_JOIN", "RIGHT_OUTER_JOIN", "OUTER_JOIN", "WHERE", "GROUP_BY", "HAVING",
- "ORDER_BY"
- ] and
- taintedArgs = "Argument[0].ArrayElement" and
- signature = "String[]"
- or
- this = "VALUES" and taintedArgs = "Argument[0..1]" and signature = "String,String"
- }
-
- string getTaintedArgs() { result = taintedArgs }
-
- string getCsvSignature() { result = signature }
-}
-
private class MyBatisAbstractSqlMethodsStep extends SummaryModelCsv {
override predicate row(string row) {
- exists(MyBatisAbstractSqlMethod m |
- row =
- "org.apache.ibatis.jdbc;AbstractSQL;true;" + m + ";(" + m.getCsvSignature() + ");;" +
- m.getTaintedArgs() + ";Argument[-1];taint"
- )
+ row =
+ [
+ "org.apache.ibatis.jdbc;AbstractSQL;true;toString;;;Argument[-1];ReturnValue;taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;WHERE;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;WHERE;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;WHERE;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;WHERE;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;VALUES;(String,String);;Argument[0..1];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;UPDATE;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SET;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SET;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SET;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SET;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SELECT_DISTINCT;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SELECT_DISTINCT;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SELECT_DISTINCT;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SELECT_DISTINCT;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;SELECT;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;RIGHT_OUTER_JOIN;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;RIGHT_OUTER_JOIN;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;RIGHT_OUTER_JOIN;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;RIGHT_OUTER_JOIN;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;OUTER_JOIN;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;OUTER_JOIN;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;OUTER_JOIN;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;OUTER_JOIN;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;ORDER_BY;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;ORDER_BY;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;ORDER_BY;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;ORDER_BY;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;OFFSET_ROWS;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;OFFSET;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;LIMIT;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;LEFT_OUTER_JOIN;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;LEFT_OUTER_JOIN;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;LEFT_OUTER_JOIN;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;LEFT_OUTER_JOIN;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;JOIN;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INTO_VALUES;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INTO_COLUMNS;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INSERT_INTO;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INNER_JOIN;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INNER_JOIN;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INNER_JOIN;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;INNER_JOIN;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;HAVING;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;HAVING;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;HAVING;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;HAVING;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;GROUP_BY;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;GROUP_BY;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;GROUP_BY;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;GROUP_BY;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;FROM;(String[]);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;FROM;(String[]);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;FROM;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;FROM;(String);;Argument[0].ArrayElement;Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;FETCH_FIRST_ROWS_ONLY;(String);;Argument[0];Argument[-1];taint",
+ "org.apache.ibatis.jdbc;AbstractSQL;true;DELETE_FROM;(String);;Argument[0];Argument[-1];taint"
+ ]
}
}
From 50e95b5aadc5a679dc85ca2c35bae019c6cfdee8 Mon Sep 17 00:00:00 2001
From: Jorge <46056498+jorgectf@users.noreply.github.com>
Date: Thu, 28 Apr 2022 21:56:20 +0200
Subject: [PATCH 0197/1618] Apply suggestions from code review
Co-authored-by: Anders Schack-Mulligen
---
java/ql/lib/semmle/code/java/frameworks/MyBatis.qll | 4 ++--
1 file changed, 2 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 691fa0d5382..b48508a0b3e 100644
--- a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
@@ -134,8 +134,8 @@ private class MyBatisProviderStep extends TaintTracking::AdditionalValueStep {
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
exists(MethodAccess ma, Annotation a, Method providerMethod |
exists(int i |
- ma.getArgument(i) = n1.asExpr() and
- providerMethod.getParameter(i) = n2.asParameter()
+ ma.getArgument(pragma[only_bind_into](i)) = n1.asExpr() and
+ providerMethod.getParameter(pragma[only_bind_into](i)) = n2.asParameter()
)
|
a.getType() instanceof MyBatisProvider and
From 548721a8cfe004d06a1482da7ede2162ab802083 Mon Sep 17 00:00:00 2001
From: jorgectf
Date: Thu, 28 Apr 2022 23:36:51 +0200
Subject: [PATCH 0198/1618] Fix `MyBatisInjectionSink`
---
java/ql/lib/semmle/code/java/frameworks/MyBatis.qll | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
index 2e08ee9ca26..86384362a3b 100644
--- a/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/MyBatis.qll
@@ -125,7 +125,7 @@ class MyBatisInjectionSink extends DataFlow::Node {
a.getType() instanceof MyBatisProvider and
m.getDeclaringType() = a.getValue(["type", "value"]).(TypeLiteral).getTypeName().getType() and
m.hasName(a.getValue("method").(StringLiteral).getValue()) and
- this.asExpr() = m.getBody().getAStmt().(ReturnStmt).getEnclosingCallable()
+ this.getEnclosingCallable() = m.getBody().getAStmt().(ReturnStmt).getEnclosingCallable()
)
}
}
From 0aa1251ffe47a396224db32e8d72a5d7beaf42ad Mon Sep 17 00:00:00 2001
From: luchua-bc
Date: Fri, 29 Apr 2022 02:31:43 +0000
Subject: [PATCH 0199/1618] Add more test cases
---
.../Security/CWE/CWE-552/UnsafeUrlForward.qll | 6 +-
.../semmle/code/java/frameworks/Jsf.qll | 2 +-
.../security/CWE-552/UnsafeResourceGet.java | 64 ++++++++++++++++-
.../security/CWE-552/UnsafeResourceGet2.java | 58 +++++++++++++++
.../CWE-552/UnsafeUrlForward.expected | 70 +++++++++++--------
.../query-tests/security/CWE-552/options | 2 +-
.../jboss-vfs-3.2/org/jboss/vfs/VFS.java | 19 +++++
.../org/jboss/vfs/VirtualFile.java | 29 ++++++++
.../resource/FileResourceManager.java | 31 ++++++++
.../server/handlers/resource/Resource.java | 33 +++++++++
10 files changed, 281 insertions(+), 33 deletions(-)
create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java
create mode 100644 java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VFS.java
create mode 100644 java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VirtualFile.java
create mode 100644 java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/FileResourceManager.java
create mode 100644 java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/Resource.java
diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
index 2ee7188f313..c741eabcaa5 100644
--- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
+++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
@@ -160,7 +160,7 @@ private class ServletGetPathSource extends SourceModelCsv {
}
}
-/** Taint model related to `java.nio.file.Path`. */
+/** Taint model related to `java.nio.file.Path` and `io.undertow.server.handlers.resource.Resource`. */
private class FilePathFlowStep extends SummaryModelCsv {
override predicate row(string row) {
row =
@@ -168,7 +168,9 @@ private class FilePathFlowStep extends SummaryModelCsv {
"java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint",
"java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint",
"java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint",
- "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint"
+ "io.undertow.server.handlers.resource;Resource;true;getFile;;;Argument[-1];ReturnValue;taint",
+ "io.undertow.server.handlers.resource;Resource;true;getFilePath;;;Argument[-1];ReturnValue;taint",
+ "io.undertow.server.handlers.resource;Resource;true;getPath;;;Argument[-1];ReturnValue;taint"
]
}
}
diff --git a/java/ql/src/experimental/semmle/code/java/frameworks/Jsf.qll b/java/ql/src/experimental/semmle/code/java/frameworks/Jsf.qll
index 4701d6ca565..a013c341c67 100644
--- a/java/ql/src/experimental/semmle/code/java/frameworks/Jsf.qll
+++ b/java/ql/src/experimental/semmle/code/java/frameworks/Jsf.qll
@@ -2,7 +2,7 @@
* Provides classes and predicates for working with the Java Server Faces (JSF).
*/
-import semmle.code.java.Type
+import java
/**
* The JSF class `ExternalContext` for processing HTTP requests.
diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java
index ae22a7e7d0f..64c23334f18 100644
--- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java
+++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java
@@ -5,7 +5,9 @@ import java.io.IOException;
import java.io.PrintWriter;
import java.nio.file.Path;
import java.nio.file.Paths;
+import java.net.URI;
import java.net.URL;
+import java.net.URISyntaxException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
@@ -15,6 +17,11 @@ import javax.servlet.ServletException;
import javax.servlet.ServletConfig;
import javax.servlet.ServletContext;
+import io.undertow.server.handlers.resource.FileResourceManager;
+import io.undertow.server.handlers.resource.Resource;
+import org.jboss.vfs.VFS;
+import org.jboss.vfs.VirtualFile;
+
public class UnsafeResourceGet extends HttpServlet {
private static final String BASE_PATH = "/pages";
@@ -137,7 +144,7 @@ public class UnsafeResourceGet extends HttpServlet {
ServletOutputStream out = response.getOutputStream();
// A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file
- // Note the class is in two levels of subpackages and `Class.loadResource` starts from its own directory
+ // Note the class is in two levels of subpackages and `Class.getResource` starts from its own directory
URL url = getClass().getResource(requestUrl);
InputStream in = url.openStream();
@@ -205,4 +212,59 @@ public class UnsafeResourceGet extends HttpServlet {
}
}
}
+
+ // BAD: getResource constructed from `ClassLoader` without input validation
+ protected void doPutBad(HttpServletRequest request, HttpServletResponse response)
+ throws ServletException, IOException {
+ String requestUrl = request.getParameter("requestURL");
+ ServletOutputStream out = response.getOutputStream();
+
+ // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file
+ // Note the class is in two levels of subpackages and `ClassLoader.getResource` starts from its own directory
+ URL url = getClass().getClassLoader().getResource(requestUrl);
+
+ InputStream in = url.openStream();
+ byte[] buf = new byte[4 * 1024]; // 4K buffer
+ int bytesRead;
+ while ((bytesRead = in.read(buf)) != -1) {
+ out.write(buf, 0, bytesRead);
+ }
+ }
+
+ // BAD: getResource constructed using Undertow IO without input validation
+ protected void doPutBad2(HttpServletRequest request, HttpServletResponse response)
+ throws ServletException, IOException {
+ String requestPath = request.getParameter("requestPath");
+
+ try {
+ FileResourceManager rm = new FileResourceManager(VFS.getChild(new URI("/usr/share")).getPhysicalFile());
+ Resource rs = rm.getResource(requestPath);
+
+ VirtualFile overlay = VFS.getChild(new URI("EAP_HOME/modules/"));
+ // Do file operations
+ overlay.getChild(rs.getPath());
+ } catch (URISyntaxException ue) {
+ throw new IOException("Cannot parse the URI");
+ }
+ }
+
+ // GOOD: getResource constructed using Undertow IO with input validation
+ protected void doPutGood2(HttpServletRequest request, HttpServletResponse response)
+ throws ServletException, IOException {
+ String requestPath = request.getParameter("requestPath");
+
+ try {
+ FileResourceManager rm = new FileResourceManager(VFS.getChild(new URI("/usr/share")).getPhysicalFile());
+ Resource rs = rm.getResource(requestPath);
+
+ VirtualFile overlay = VFS.getChild(new URI("EAP_HOME/modules/"));
+ String path = rs.getPath();
+ if (path.startsWith("/trusted_path") && !path.contains("..")) {
+ // Do file operations
+ overlay.getChild(path);
+ }
+ } catch (URISyntaxException ue) {
+ throw new IOException("Cannot parse the URI");
+ }
+ }
}
diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java
new file mode 100644
index 00000000000..b3d041d024c
--- /dev/null
+++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java
@@ -0,0 +1,58 @@
+package com.example;
+
+import javax.faces.context.FacesContext;
+import java.io.BufferedReader;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.IOException;
+import java.net.URL;
+import java.util.Map;
+
+/** Sample class of JSF managed bean */
+public class UnsafeResourceGet2 {
+ // BAD: getResourceAsStream constructed from `ExternalContext` without input validation
+ public String parameterActionBad1() throws IOException {
+ FacesContext fc = FacesContext.getCurrentInstance();
+ Map params = fc.getExternalContext().getRequestParameterMap();
+ String loadUrl = params.get("loadUrl");
+
+ InputStreamReader isr = new InputStreamReader(fc.getExternalContext().getResourceAsStream(loadUrl));
+ BufferedReader br = new BufferedReader(isr);
+ if(br.ready()) {
+ //Do Stuff
+ return "result";
+ }
+
+ return "home";
+ }
+
+ // BAD: getResource constructed from `ExternalContext` without input validation
+ public String parameterActionBad2() throws IOException {
+ FacesContext fc = FacesContext.getCurrentInstance();
+ Map params = fc.getExternalContext().getRequestParameterMap();
+ String loadUrl = params.get("loadUrl");
+
+ URL url = fc.getExternalContext().getResource(loadUrl);
+
+ InputStream in = url.openStream();
+ //Do Stuff
+ return "result";
+ }
+
+ // GOOD: getResource constructed from `ExternalContext` with input validation
+ public String parameterActionGood1() throws IOException {
+ FacesContext fc = FacesContext.getCurrentInstance();
+ Map params = fc.getExternalContext().getRequestParameterMap();
+ String loadUrl = params.get("loadUrl");
+
+ if (loadUrl.equals("/public/crossdomain.xml")) {
+ URL url = fc.getExternalContext().getResource(loadUrl);
+
+ InputStream in = url.openStream();
+ //Do Stuff
+ return "result";
+ }
+
+ return "home";
+ }
+}
diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected
index 2f136732fdc..202ce9d0cd8 100644
--- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected
+++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected
@@ -1,17 +1,21 @@
edges
| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path |
-| UnsafeResourceGet.java:25:23:25:56 | getParameter(...) : String | UnsafeResourceGet.java:34:20:34:22 | url |
-| UnsafeResourceGet.java:104:24:104:58 | getParameter(...) : String | UnsafeResourceGet.java:108:68:108:78 | requestPath |
-| UnsafeResourceGet.java:136:23:136:56 | getParameter(...) : String | UnsafeResourceGet.java:143:20:143:22 | url |
-| UnsafeResourceGet.java:174:24:174:58 | getParameter(...) : String | UnsafeResourceGet.java:182:68:182:78 | requestPath |
+| UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:17:20:17:25 | params : Map |
+| UnsafeResourceGet2.java:17:20:17:25 | params : Map | UnsafeResourceGet2.java:17:20:17:40 | get(...) : Object |
+| UnsafeResourceGet2.java:17:20:17:40 | get(...) : Object | UnsafeResourceGet2.java:19:93:19:99 | loadUrl |
+| UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:33:20:33:25 | params : Map |
+| UnsafeResourceGet2.java:33:20:33:25 | params : Map | UnsafeResourceGet2.java:33:20:33:40 | get(...) : Object |
+| UnsafeResourceGet2.java:33:20:33:40 | get(...) : Object | UnsafeResourceGet2.java:37:20:37:22 | url |
+| UnsafeResourceGet.java:32:23:32:56 | getParameter(...) : String | UnsafeResourceGet.java:41:20:41:22 | url |
+| UnsafeResourceGet.java:111:24:111:58 | getParameter(...) : String | UnsafeResourceGet.java:115:68:115:78 | requestPath |
+| UnsafeResourceGet.java:143:23:143:56 | getParameter(...) : String | UnsafeResourceGet.java:150:20:150:22 | url |
+| UnsafeResourceGet.java:181:24:181:58 | getParameter(...) : String | UnsafeResourceGet.java:189:68:189:78 | requestPath |
+| UnsafeResourceGet.java:219:23:219:56 | getParameter(...) : String | UnsafeResourceGet.java:226:20:226:22 | url |
+| UnsafeResourceGet.java:237:24:237:58 | getParameter(...) : String | UnsafeResourceGet.java:245:21:245:22 | rs : Resource |
+| UnsafeResourceGet.java:245:21:245:22 | rs : Resource | UnsafeResourceGet.java:245:21:245:32 | getPath(...) |
| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL |
| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL |
| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path |
-| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:107:53:107:56 | path : String |
-| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path |
-| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path |
-| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path |
-| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) |
| UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url |
| UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url |
| UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url |
@@ -23,26 +27,33 @@ edges
nodes
| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | semmle.label | getServletPath(...) : String |
| UnsafeRequestPath.java:23:33:23:36 | path | semmle.label | path |
-| UnsafeResourceGet.java:25:23:25:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UnsafeResourceGet.java:34:20:34:22 | url | semmle.label | url |
-| UnsafeResourceGet.java:104:24:104:58 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UnsafeResourceGet.java:108:68:108:78 | requestPath | semmle.label | requestPath |
-| UnsafeResourceGet.java:136:23:136:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UnsafeResourceGet.java:143:20:143:22 | url | semmle.label | url |
-| UnsafeResourceGet.java:174:24:174:58 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UnsafeResourceGet.java:182:68:182:78 | requestPath | semmle.label | requestPath |
+| UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | semmle.label | getRequestParameterMap(...) : Map |
+| UnsafeResourceGet2.java:17:20:17:25 | params : Map | semmle.label | params : Map |
+| UnsafeResourceGet2.java:17:20:17:40 | get(...) : Object | semmle.label | get(...) : Object |
+| UnsafeResourceGet2.java:19:93:19:99 | loadUrl | semmle.label | loadUrl |
+| UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | semmle.label | getRequestParameterMap(...) : Map |
+| UnsafeResourceGet2.java:33:20:33:25 | params : Map | semmle.label | params : Map |
+| UnsafeResourceGet2.java:33:20:33:40 | get(...) : Object | semmle.label | get(...) : Object |
+| UnsafeResourceGet2.java:37:20:37:22 | url | semmle.label | url |
+| UnsafeResourceGet.java:32:23:32:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceGet.java:41:20:41:22 | url | semmle.label | url |
+| UnsafeResourceGet.java:111:24:111:58 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceGet.java:115:68:115:78 | requestPath | semmle.label | requestPath |
+| UnsafeResourceGet.java:143:23:143:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceGet.java:150:20:150:22 | url | semmle.label | url |
+| UnsafeResourceGet.java:181:24:181:58 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceGet.java:189:68:189:78 | requestPath | semmle.label | requestPath |
+| UnsafeResourceGet.java:219:23:219:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceGet.java:226:20:226:22 | url | semmle.label | url |
+| UnsafeResourceGet.java:237:24:237:58 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceGet.java:245:21:245:22 | rs : Resource | semmle.label | rs : Resource |
+| UnsafeResourceGet.java:245:21:245:32 | getPath(...) | semmle.label | getPath(...) |
| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | semmle.label | returnURL |
| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | semmle.label | returnURL |
| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| UnsafeServletRequestDispatch.java:76:53:76:56 | path | semmle.label | path |
-| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | semmle.label | resolve(...) : Path |
-| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | semmle.label | normalize(...) : Path |
-| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | semmle.label | path : String |
-| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | semmle.label | requestedPath : Path |
-| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | semmle.label | toString(...) |
| UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String |
| UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url |
| UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String |
@@ -61,14 +72,17 @@ nodes
subpaths
#select
| UnsafeRequestPath.java:23:33:23:36 | path | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path | Potentially untrusted URL forward due to $@. | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) | user-provided value |
-| UnsafeResourceGet.java:34:20:34:22 | url | UnsafeResourceGet.java:25:23:25:56 | getParameter(...) : String | UnsafeResourceGet.java:34:20:34:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:25:23:25:56 | getParameter(...) | user-provided value |
-| UnsafeResourceGet.java:108:68:108:78 | requestPath | UnsafeResourceGet.java:104:24:104:58 | getParameter(...) : String | UnsafeResourceGet.java:108:68:108:78 | requestPath | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:104:24:104:58 | getParameter(...) | user-provided value |
-| UnsafeResourceGet.java:143:20:143:22 | url | UnsafeResourceGet.java:136:23:136:56 | getParameter(...) : String | UnsafeResourceGet.java:143:20:143:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:136:23:136:56 | getParameter(...) | user-provided value |
-| UnsafeResourceGet.java:182:68:182:78 | requestPath | UnsafeResourceGet.java:174:24:174:58 | getParameter(...) : String | UnsafeResourceGet.java:182:68:182:78 | requestPath | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:174:24:174:58 | getParameter(...) | user-provided value |
+| UnsafeResourceGet2.java:19:93:19:99 | loadUrl | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:19:93:19:99 | loadUrl | Potentially untrusted URL forward due to $@. | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) | user-provided value |
+| UnsafeResourceGet2.java:37:20:37:22 | url | UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:37:20:37:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) | user-provided value |
+| UnsafeResourceGet.java:41:20:41:22 | url | UnsafeResourceGet.java:32:23:32:56 | getParameter(...) : String | UnsafeResourceGet.java:41:20:41:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:32:23:32:56 | getParameter(...) | user-provided value |
+| UnsafeResourceGet.java:115:68:115:78 | requestPath | UnsafeResourceGet.java:111:24:111:58 | getParameter(...) : String | UnsafeResourceGet.java:115:68:115:78 | requestPath | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:111:24:111:58 | getParameter(...) | user-provided value |
+| UnsafeResourceGet.java:150:20:150:22 | url | UnsafeResourceGet.java:143:23:143:56 | getParameter(...) : String | UnsafeResourceGet.java:150:20:150:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:143:23:143:56 | getParameter(...) | user-provided value |
+| UnsafeResourceGet.java:189:68:189:78 | requestPath | UnsafeResourceGet.java:181:24:181:58 | getParameter(...) : String | UnsafeResourceGet.java:189:68:189:78 | requestPath | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:181:24:181:58 | getParameter(...) | user-provided value |
+| UnsafeResourceGet.java:226:20:226:22 | url | UnsafeResourceGet.java:219:23:219:56 | getParameter(...) : String | UnsafeResourceGet.java:226:20:226:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:219:23:219:56 | getParameter(...) | user-provided value |
+| UnsafeResourceGet.java:245:21:245:32 | getPath(...) | UnsafeResourceGet.java:237:24:237:58 | getParameter(...) : String | UnsafeResourceGet.java:245:21:245:32 | getPath(...) | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:237:24:237:58 | getParameter(...) | user-provided value |
| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) | user-provided value |
| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) | user-provided value |
| UnsafeServletRequestDispatch.java:76:53:76:56 | path | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) | user-provided value |
-| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) | user-provided value |
| UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value |
| UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value |
| UnsafeUrlForward.java:26:23:26:25 | url | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:25:21:25:30 | url | user-provided value |
diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/options b/java/ql/test/experimental/query-tests/security/CWE-552/options
index ba166b547a0..095b86c3e9a 100644
--- a/java/ql/test/experimental/query-tests/security/CWE-552/options
+++ b/java/ql/test/experimental/query-tests/security/CWE-552/options
@@ -1 +1 @@
-//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/
\ No newline at end of file
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/:${testdir}/../../../../stubs/javax-faces-2.3/:${testdir}/../../../../stubs/undertow-io-2.2/:${testdir}/../../../../stubs/jboss-vfs-3.2/
diff --git a/java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VFS.java b/java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VFS.java
new file mode 100644
index 00000000000..e3528239558
--- /dev/null
+++ b/java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VFS.java
@@ -0,0 +1,19 @@
+package org.jboss.vfs;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URL;
+
+public class VFS {
+ public static VirtualFile getChild(URL url) throws URISyntaxException {
+ return null;
+ }
+
+ public static VirtualFile getChild(URI uri) {
+ return null;
+ }
+
+ public static VirtualFile getChild(String path) {
+ return null;
+ }
+}
diff --git a/java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VirtualFile.java b/java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VirtualFile.java
new file mode 100644
index 00000000000..ff6c17caff6
--- /dev/null
+++ b/java/ql/test/stubs/jboss-vfs-3.2/org/jboss/vfs/VirtualFile.java
@@ -0,0 +1,29 @@
+package org.jboss.vfs;
+
+import java.io.File;
+import java.io.IOException;
+
+public class VirtualFile {
+ VirtualFile(String name, VirtualFile parent) {
+ }
+
+ public String getName() {
+ return null;
+ }
+
+ public String getPathName() {
+ return null;
+ }
+
+ String getPathName(boolean url) {
+ return null;
+ }
+
+ public File getPhysicalFile() throws IOException {
+ return null;
+ }
+
+ public VirtualFile getChild(String path) {
+ return null;
+ }
+}
diff --git a/java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/FileResourceManager.java b/java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/FileResourceManager.java
new file mode 100644
index 00000000000..815222f2f9f
--- /dev/null
+++ b/java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/FileResourceManager.java
@@ -0,0 +1,31 @@
+package io.undertow.server.handlers.resource;
+
+import java.io.File;
+
+public class FileResourceManager {
+ public FileResourceManager(final File base) {
+ }
+
+ public FileResourceManager(final File base, long transferMinSize) {
+ }
+
+ public FileResourceManager(final File base, long transferMinSize, boolean caseSensitive) {
+ }
+
+ public FileResourceManager(final File base, long transferMinSize, boolean followLinks, final String... safePaths) {
+ }
+
+ protected FileResourceManager(long transferMinSize, boolean caseSensitive, boolean followLinks, final String... safePaths) {
+ }
+
+ public FileResourceManager(final File base, long transferMinSize, boolean caseSensitive, boolean followLinks, final String... safePaths) {
+ }
+
+ public File getBase() {
+ return null;
+ }
+
+ public Resource getResource(final String p) {
+ return null;
+ }
+}
diff --git a/java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/Resource.java b/java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/Resource.java
new file mode 100644
index 00000000000..579e08107c6
--- /dev/null
+++ b/java/ql/test/stubs/undertow-io-2.2/io/undertow/server/handlers/resource/Resource.java
@@ -0,0 +1,33 @@
+package io.undertow.server.handlers.resource;
+
+import java.io.File;
+import java.net.URL;
+import java.nio.file.Path;
+import java.util.Date;
+import java.util.List;
+
+public interface Resource {
+ String getPath();
+
+ Date getLastModified();
+
+ String getLastModifiedString();
+
+ String getName();
+
+ boolean isDirectory();
+
+ List list();
+
+ Long getContentLength();
+
+ File getFile();
+
+ Path getFilePath();
+
+ File getResourceManagerRoot();
+
+ Path getResourceManagerRootPath();
+
+ URL getUrl();
+}
From 33d499c12db1c7b2c19eff664beb72677c048d51 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 29 Apr 2022 08:50:11 +0100
Subject: [PATCH 0200/1618] C++: Address review comments.
---
cpp/ql/src/Security/CWE/CWE-611/XXE.ql | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
index 20d979fd8c8..be626795db4 100644
--- a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
+++ b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
@@ -59,8 +59,8 @@ class XercesDOMParserClass extends Class {
/**
* The `SAXParser` class.
*/
-class SAXParser extends Class {
- SAXParser() { this.hasName("SAXParser") }
+class SAXParserClass extends Class {
+ SAXParserClass() { this.hasName("SAXParser") }
}
/**
@@ -112,7 +112,7 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
call.getTarget() = f and
(
f.getDeclaringType() instanceof AbstractDOMParserClass or
- f.getDeclaringType() instanceof SAXParser
+ f.getDeclaringType() instanceof SAXParserClass
) and
f.hasName("setDisableDefaultEntityResolution") and
this = call.getQualifier() and
@@ -172,7 +172,7 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
class ParseFunction extends Function {
ParseFunction() {
this.getClassAndName("parse") instanceof AbstractDOMParserClass or
- this.getClassAndName("parse") instanceof SAXParser
+ this.getClassAndName("parse") instanceof SAXParserClass
}
}
@@ -213,9 +213,9 @@ class XXEConfiguration extends DataFlow::Configuration {
// source is the write on `this` of a call to the `SAXParser`
// constructor.
exists(CallInstruction call |
+ call.getStaticCallTarget() = any(SAXParserClass c).getAConstructor() and
node.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
call.getThisArgument() and
- call.getStaticCallTarget().(Constructor).getDeclaringType() instanceof SAXParser and
encodeXercesFlowState(flowstate, 0, 1) // default configuration
)
}
From 215453e4db951b8d6e858cac8e5494d5ac1756ed Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 29 Apr 2022 09:07:25 +0100
Subject: [PATCH 0201/1618] Update cpp/ql/src/Security/CWE/CWE-611/XXE.ql
Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
---
cpp/ql/src/Security/CWE/CWE-611/XXE.ql | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
index be626795db4..76247cf7c49 100644
--- a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
+++ b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
@@ -91,7 +91,7 @@ predicate encodeXercesFlowState(
}
/**
- * A flow state representing the configuration of a `AbstractDOMParser` or
+ * A flow state representing the configuration of an `AbstractDOMParser` or
* `SAXParser` object.
*/
class XercesFlowState extends XXEFlowState {
From 7fb1069d69b0cff278250c7d05de52f6c13f2e2a Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 29 Apr 2022 09:54:47 +0100
Subject: [PATCH 0202/1618] C++: Use GVN on the values passed into set*
functions.
---
cpp/ql/src/Security/CWE/CWE-611/XXE.ql | 9 +++++----
.../test/query-tests/Security/CWE/CWE-611/XXE.expected | 4 ----
cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp | 2 +-
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
index 76247cf7c49..805f3a61277 100644
--- a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
+++ b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
@@ -16,6 +16,7 @@ import cpp
import semmle.code.cpp.ir.dataflow.DataFlow
import DataFlow::PathGraph
import semmle.code.cpp.ir.IR
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
/**
* A flow state representing a possible configuration of an XML object.
@@ -124,10 +125,10 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
exists(int createEntityReferenceNodes |
encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and
(
- newValue.getValue().toInt() = 1 and // true
+ globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
encodeXercesFlowState(result, 1, createEntityReferenceNodes)
or
- not newValue.getValue().toInt() = 1 and // false or unknown
+ not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
encodeXercesFlowState(result, 0, createEntityReferenceNodes)
)
)
@@ -156,10 +157,10 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
exists(int disabledDefaultEntityResolution |
encodeXercesFlowState(flowstate, disabledDefaultEntityResolution, _) and
(
- newValue.getValue().toInt() = 1 and // true
+ globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
encodeXercesFlowState(result, disabledDefaultEntityResolution, 1)
or
- not newValue.getValue().toInt() = 1 and // false or unknown
+ not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
encodeXercesFlowState(result, disabledDefaultEntityResolution, 0)
)
)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
index f6360956a5f..25f1ad8e1ab 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
@@ -1,7 +1,6 @@
edges
| tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p |
| tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p |
-| tests2.cpp:41:17:41:31 | SAXParser output argument | tests2.cpp:45:2:45:2 | p |
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p |
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p |
| tests.cpp:53:19:53:19 | VariableAddress [post update] | tests.cpp:55:2:55:2 | p |
@@ -34,8 +33,6 @@ nodes
| tests2.cpp:22:2:22:2 | p | semmle.label | p |
| tests2.cpp:33:17:33:31 | SAXParser output argument | semmle.label | SAXParser output argument |
| tests2.cpp:37:2:37:2 | p | semmle.label | p |
-| tests2.cpp:41:17:41:31 | SAXParser output argument | semmle.label | SAXParser output argument |
-| tests2.cpp:45:2:45:2 | p | semmle.label | p |
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
| tests.cpp:35:2:35:2 | p | semmle.label | p |
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
@@ -77,7 +74,6 @@ subpaths
#select
| tests2.cpp:22:2:22:2 | p | tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:20:17:20:31 | SAXParser output argument | XML parser |
| tests2.cpp:37:2:37:2 | p | tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:33:17:33:31 | SAXParser output argument | XML parser |
-| tests2.cpp:45:2:45:2 | p | tests2.cpp:41:17:41:31 | SAXParser output argument | tests2.cpp:45:2:45:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:41:17:41:31 | SAXParser output argument | XML parser |
| tests.cpp:35:2:35:2 | p | tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:33:23:33:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:49:2:49:2 | p | tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:46:23:46:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:57:2:57:2 | p | tests.cpp:53:23:53:43 | XercesDOMParser output argument | tests.cpp:57:2:57:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:53:23:53:43 | XercesDOMParser output argument | XML parser |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
index d5a495b29aa..758cb57b26e 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
@@ -42,5 +42,5 @@ void test2_4(InputSource &data) {
bool v = true;
p->setDisableDefaultEntityResolution(v);
- p->parse(data); // GOOD [FALSE POSITIVE]
+ p->parse(data); // GOOD
}
From dfe21409021c9ad6b7cab8f6a959df7ea140b1a6 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen
Date: Fri, 29 Apr 2022 11:22:12 +0200
Subject: [PATCH 0203/1618] slight simplification
---
ql/ql/src/queries/performance/UnusedField.ql | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/ql/ql/src/queries/performance/UnusedField.ql b/ql/ql/src/queries/performance/UnusedField.ql
index 5e58bb7f769..37b32f61a2a 100644
--- a/ql/ql/src/queries/performance/UnusedField.ql
+++ b/ql/ql/src/queries/performance/UnusedField.ql
@@ -16,8 +16,7 @@ where
implClz.getASuperType*() = clz and
// The field is not accessed in the charpred (of any of the classes)
not exists(FieldAccess access |
- access.getEnclosingPredicate() =
- [clz.getDeclaration().getCharPred(), implClz.getDeclaration().getCharPred()]
+ access.getEnclosingPredicate() = [clz, implClz].getDeclaration().getCharPred()
) and
// The implementation class is not abstract, and the field is not an override
not implClz.getDeclaration().isAbstract() and
From a1542322e279225a326313cb936e150f91fbe04a Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 09:39:33 +0100
Subject: [PATCH 0204/1618] C++: Add test cases for SAX2XMLReader.
---
.../Security/CWE/CWE-611/tests3.cpp | 65 +++++++++++++++++++
1 file changed, 65 insertions(+)
create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
new file mode 100644
index 00000000000..bcece961d56
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
@@ -0,0 +1,65 @@
+// test cases for rule CWE-611
+
+#include "tests.h"
+
+// ---
+
+typedef unsigned int XMLCh;
+
+class XMLUni
+{
+public:
+ static const XMLCh fgXercesDisableDefaultEntityResolution[];
+};
+
+class SAX2XMLReader
+{
+public:
+ void setFeature(const XMLCh *feature, bool value);
+ void parse(const InputSource &data);
+};
+
+class XMLReaderFactory
+{
+public:
+ static SAX2XMLReader *createXMLReader();
+};
+
+// ---
+
+void test3_1(InputSource &data) {
+ SAX2XMLReader *p = XMLReaderFactory::createXMLReader();
+
+ p->parse(data); // BAD (parser not correctly configured) [NOT DETECTED]
+}
+
+void test3_2(InputSource &data) {
+ SAX2XMLReader *p = XMLReaderFactory::createXMLReader();
+
+ p->setFeature(XMLUni::fgXercesDisableDefaultEntityResolution, true);
+ p->parse(data); // GOOD
+}
+
+SAX2XMLReader *p_3_3 = XMLReaderFactory::createXMLReader();
+
+void test3_3(InputSource &data) {
+ p_3_3->parse(data); // BAD (parser not correctly configured) [NOT DETECTED]
+}
+
+SAX2XMLReader *p_3_4 = XMLReaderFactory::createXMLReader();
+
+void test3_4(InputSource &data) {
+ p_3_4->setFeature(XMLUni::fgXercesDisableDefaultEntityResolution, true);
+ p_3_4->parse(data); // GOOD
+}
+
+SAX2XMLReader *p_3_5 = XMLReaderFactory::createXMLReader();
+
+void test3_5_init() {
+ p_3_5->setFeature(XMLUni::fgXercesDisableDefaultEntityResolution, true);
+}
+
+void test3_5(InputSource &data) {
+ test3_5_init();
+ p_3_5->parse(data); // GOOD
+}
From b02519bf0bb9c6d79ed01387466673c559a84293 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 16:35:31 +0100
Subject: [PATCH 0205/1618] C++: Make the createLSParser test a bit closer to
real life.
---
.../Security/CWE/CWE-611/XXE.expected | 6 ++--
.../Security/CWE/CWE-611/tests.cpp | 30 ++++++++++++-------
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
index 25f1ad8e1ab..083fa5f673e 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
@@ -27,7 +27,7 @@ edges
| tests.cpp:140:23:140:43 | XercesDOMParser output argument | tests.cpp:146:18:146:18 | q |
| tests.cpp:144:18:144:18 | q | tests.cpp:130:39:130:39 | p |
| tests.cpp:146:18:146:18 | q | tests.cpp:134:39:134:39 | p |
-| tests.cpp:150:19:150:32 | call to createLSParser | tests.cpp:152:2:152:2 | p |
+| tests.cpp:150:25:150:38 | call to createLSParser | tests.cpp:152:2:152:2 | p |
nodes
| tests2.cpp:20:17:20:31 | SAXParser output argument | semmle.label | SAXParser output argument |
| tests2.cpp:22:2:22:2 | p | semmle.label | p |
@@ -68,7 +68,7 @@ nodes
| tests.cpp:140:23:140:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
| tests.cpp:144:18:144:18 | q | semmle.label | q |
| tests.cpp:146:18:146:18 | q | semmle.label | q |
-| tests.cpp:150:19:150:32 | call to createLSParser | semmle.label | call to createLSParser |
+| tests.cpp:150:25:150:38 | call to createLSParser | semmle.label | call to createLSParser |
| tests.cpp:152:2:152:2 | p | semmle.label | p |
subpaths
#select
@@ -85,4 +85,4 @@ subpaths
| tests.cpp:122:3:122:3 | q | tests.cpp:118:24:118:44 | XercesDOMParser output argument | tests.cpp:122:3:122:3 | q | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:118:24:118:44 | XercesDOMParser output argument | XML parser |
| tests.cpp:131:2:131:2 | p | tests.cpp:140:23:140:43 | XercesDOMParser output argument | tests.cpp:131:2:131:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:140:23:140:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:135:2:135:2 | p | tests.cpp:140:23:140:43 | XercesDOMParser output argument | tests.cpp:135:2:135:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:140:23:140:43 | XercesDOMParser output argument | XML parser |
-| tests.cpp:152:2:152:2 | p | tests.cpp:150:19:150:32 | call to createLSParser | tests.cpp:152:2:152:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:150:19:150:32 | call to createLSParser | XML parser |
+| tests.cpp:152:2:152:2 | p | tests.cpp:150:25:150:38 | call to createLSParser | tests.cpp:152:2:152:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:150:25:150:38 | call to createLSParser | XML parser |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp
index 76ceb7b7556..ec822b44a20 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp
@@ -4,9 +4,6 @@
// ---
-
-
-
class AbstractDOMParser {
public:
AbstractDOMParser();
@@ -25,7 +22,10 @@ public:
class DOMLSParser : public AbstractDOMParser {
};
-DOMLSParser *createLSParser();
+class DOMImplementationLS {
+public:
+ DOMLSParser *createLSParser();
+};
// ---
@@ -146,25 +146,33 @@ void test10(InputSource &data) {
test10_doParseC(q, data);
}
-void test11(InputSource &data) {
- DOMLSParser *p = createLSParser();
+void test11(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
p->parse(data); // BAD (parser not correctly configured)
}
-void test12(InputSource &data) {
- DOMLSParser *p = createLSParser();
+void test12(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
p->setDisableDefaultEntityResolution(true);
p->parse(data); // GOOD
}
-DOMLSParser *g_p1 = createLSParser();
-DOMLSParser *g_p2 = createLSParser();
+DOMImplementationLS *g_impl;
+DOMLSParser *g_p1, *g_p2;
InputSource *g_data;
-void test13() {
+void test13_init() {
+ g_p1 = g_impl->createLSParser();
g_p1->setDisableDefaultEntityResolution(true);
+
+ g_p2 = g_impl->createLSParser();
+}
+
+void test13() {
+ test13_init();
+
g_p1->parse(*g_data); // GOOD
g_p2->parse(*g_data); // BAD (parser not correctly configured) [NOT DETECTED]
}
From 397efd1648e66342b3accacddbdc1448893ee3b1 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 16:56:00 +0100
Subject: [PATCH 0206/1618] C++: Split off the createLSParser tests into their
own file.
---
.../Security/CWE/CWE-611/XXE.expected | 8 +--
.../Security/CWE/CWE-611/tests.cpp | 65 +++++--------------
.../query-tests/Security/CWE/CWE-611/tests.h | 10 +++
.../Security/CWE/CWE-611/tests5.cpp | 46 +++++++++++++
4 files changed, 77 insertions(+), 52 deletions(-)
create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
index 083fa5f673e..d5353ff057a 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
@@ -1,6 +1,7 @@
edges
| tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p |
| tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p |
+| tests5.cpp:18:25:18:38 | call to createLSParser | tests5.cpp:20:2:20:2 | p |
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p |
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p |
| tests.cpp:53:19:53:19 | VariableAddress [post update] | tests.cpp:55:2:55:2 | p |
@@ -27,12 +28,13 @@ edges
| tests.cpp:140:23:140:43 | XercesDOMParser output argument | tests.cpp:146:18:146:18 | q |
| tests.cpp:144:18:144:18 | q | tests.cpp:130:39:130:39 | p |
| tests.cpp:146:18:146:18 | q | tests.cpp:134:39:134:39 | p |
-| tests.cpp:150:25:150:38 | call to createLSParser | tests.cpp:152:2:152:2 | p |
nodes
| tests2.cpp:20:17:20:31 | SAXParser output argument | semmle.label | SAXParser output argument |
| tests2.cpp:22:2:22:2 | p | semmle.label | p |
| tests2.cpp:33:17:33:31 | SAXParser output argument | semmle.label | SAXParser output argument |
| tests2.cpp:37:2:37:2 | p | semmle.label | p |
+| tests5.cpp:18:25:18:38 | call to createLSParser | semmle.label | call to createLSParser |
+| tests5.cpp:20:2:20:2 | p | semmle.label | p |
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
| tests.cpp:35:2:35:2 | p | semmle.label | p |
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
@@ -68,12 +70,11 @@ nodes
| tests.cpp:140:23:140:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
| tests.cpp:144:18:144:18 | q | semmle.label | q |
| tests.cpp:146:18:146:18 | q | semmle.label | q |
-| tests.cpp:150:25:150:38 | call to createLSParser | semmle.label | call to createLSParser |
-| tests.cpp:152:2:152:2 | p | semmle.label | p |
subpaths
#select
| tests2.cpp:22:2:22:2 | p | tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:20:17:20:31 | SAXParser output argument | XML parser |
| tests2.cpp:37:2:37:2 | p | tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:33:17:33:31 | SAXParser output argument | XML parser |
+| tests5.cpp:20:2:20:2 | p | tests5.cpp:18:25:18:38 | call to createLSParser | tests5.cpp:20:2:20:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests5.cpp:18:25:18:38 | call to createLSParser | XML parser |
| tests.cpp:35:2:35:2 | p | tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:33:23:33:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:49:2:49:2 | p | tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:46:23:46:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:57:2:57:2 | p | tests.cpp:53:23:53:43 | XercesDOMParser output argument | tests.cpp:57:2:57:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:53:23:53:43 | XercesDOMParser output argument | XML parser |
@@ -85,4 +86,3 @@ subpaths
| tests.cpp:122:3:122:3 | q | tests.cpp:118:24:118:44 | XercesDOMParser output argument | tests.cpp:122:3:122:3 | q | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:118:24:118:44 | XercesDOMParser output argument | XML parser |
| tests.cpp:131:2:131:2 | p | tests.cpp:140:23:140:43 | XercesDOMParser output argument | tests.cpp:131:2:131:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:140:23:140:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:135:2:135:2 | p | tests.cpp:140:23:140:43 | XercesDOMParser output argument | tests.cpp:135:2:135:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:140:23:140:43 | XercesDOMParser output argument | XML parser |
-| tests.cpp:152:2:152:2 | p | tests.cpp:150:25:150:38 | call to createLSParser | tests.cpp:152:2:152:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:150:25:150:38 | call to createLSParser | XML parser |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp
index ec822b44a20..a2d767e19bd 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp
@@ -1,31 +1,31 @@
-// test cases for rule CWE-611
+// test cases for rule CWE-611 (XercesDOMParser)
#include "tests.h"
// ---
-class AbstractDOMParser {
-public:
- AbstractDOMParser();
-
- void setDisableDefaultEntityResolution(bool); // default is false
- void setCreateEntityReferenceNodes(bool); // default is true
- void setSecurityManager(SecurityManager *const manager);
- void parse(const InputSource &data);
-};
-
class XercesDOMParser: public AbstractDOMParser {
public:
XercesDOMParser();
};
-class DOMLSParser : public AbstractDOMParser {
-};
-class DOMImplementationLS {
-public:
- DOMLSParser *createLSParser();
-};
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
// ---
@@ -145,34 +145,3 @@ void test10(InputSource &data) {
test10_doParseC(p, data);
test10_doParseC(q, data);
}
-
-void test11(DOMImplementationLS *impl, InputSource &data) {
- DOMLSParser *p = impl->createLSParser();
-
- p->parse(data); // BAD (parser not correctly configured)
-}
-
-void test12(DOMImplementationLS *impl, InputSource &data) {
- DOMLSParser *p = impl->createLSParser();
-
- p->setDisableDefaultEntityResolution(true);
- p->parse(data); // GOOD
-}
-
-DOMImplementationLS *g_impl;
-DOMLSParser *g_p1, *g_p2;
-InputSource *g_data;
-
-void test13_init() {
- g_p1 = g_impl->createLSParser();
- g_p1->setDisableDefaultEntityResolution(true);
-
- g_p2 = g_impl->createLSParser();
-}
-
-void test13() {
- test13_init();
-
- g_p1->parse(*g_data); // GOOD
- g_p2->parse(*g_data); // BAD (parser not correctly configured) [NOT DETECTED]
-}
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
index aa4c539bbd9..4e5c5b4e6fb 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
@@ -2,3 +2,13 @@
class SecurityManager;
class InputSource;
+
+class AbstractDOMParser {
+public:
+ AbstractDOMParser();
+
+ void setDisableDefaultEntityResolution(bool); // default is false
+ void setCreateEntityReferenceNodes(bool); // default is true
+ void setSecurityManager(SecurityManager *const manager);
+ void parse(const InputSource &data);
+};
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
new file mode 100644
index 00000000000..3f3bfad92df
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
@@ -0,0 +1,46 @@
+// test cases for rule CWE-611 (createLSParser)
+
+#include "tests.h"
+
+// ---
+
+class DOMLSParser : public AbstractDOMParser {
+};
+
+class DOMImplementationLS {
+public:
+ DOMLSParser *createLSParser();
+};
+
+// ---
+
+void test5_1(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
+
+ p->parse(data); // BAD (parser not correctly configured)
+}
+
+void test5_2(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
+
+ p->setDisableDefaultEntityResolution(true);
+ p->parse(data); // GOOD
+}
+
+DOMImplementationLS *g_impl;
+DOMLSParser *g_p1, *g_p2;
+InputSource *g_data;
+
+void test5_3_init() {
+ g_p1 = g_impl->createLSParser();
+ g_p1->setDisableDefaultEntityResolution(true);
+
+ g_p2 = g_impl->createLSParser();
+}
+
+void test5_3() {
+ test5_3_init();
+
+ g_p1->parse(*g_data); // GOOD
+ g_p2->parse(*g_data); // BAD (parser not correctly configured) [NOT DETECTED]
+}
From 4be31618917f67716e32c12ed51c80e82a46f172 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 17:02:26 +0100
Subject: [PATCH 0207/1618] C++: Move some stuff from tests3.cpp to common
tests.h
---
.../query-tests/Security/CWE/CWE-611/tests.h | 8 ++++++++
.../Security/CWE/CWE-611/tests3.cpp | 18 +++++++++---------
2 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
index 4e5c5b4e6fb..0e0d1c9d8c5 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
@@ -12,3 +12,11 @@ public:
void setSecurityManager(SecurityManager *const manager);
void parse(const InputSource &data);
};
+
+typedef unsigned int XMLCh;
+
+class XMLUni
+{
+public:
+ static const XMLCh fgXercesDisableDefaultEntityResolution[];
+};
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
index bcece961d56..3bff5d77866 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
@@ -1,17 +1,9 @@
-// test cases for rule CWE-611
+// test cases for rule CWE-611 (SAX2XMLReader)
#include "tests.h"
// ---
-typedef unsigned int XMLCh;
-
-class XMLUni
-{
-public:
- static const XMLCh fgXercesDisableDefaultEntityResolution[];
-};
-
class SAX2XMLReader
{
public:
@@ -25,6 +17,14 @@ public:
static SAX2XMLReader *createXMLReader();
};
+
+
+
+
+
+
+
+
// ---
void test3_1(InputSource &data) {
From c6deddb2904f72a713d7f5d172c39fad70800188 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 17:09:32 +0100
Subject: [PATCH 0208/1618] C++: For consistency.
---
cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
index 758cb57b26e..147be557844 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
@@ -1,4 +1,4 @@
-// test cases for rule CWE-611
+// test cases for rule CWE-611 (SAXParser)
#include "tests.h"
From 1d71f042db20cc0e59e9c5d95f1c52079b74c337 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 17:14:39 +0100
Subject: [PATCH 0209/1618] C++: Turns out DOMLSParser is not an
AbstractDOMParser and works a little differently than I'd thought.
---
.../Security/CWE/CWE-611/XXE.expected | 4 ----
.../query-tests/Security/CWE/CWE-611/tests5.cpp | 17 +++++++++++++----
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
index d5353ff057a..dcf334d5bfe 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
@@ -1,7 +1,6 @@
edges
| tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p |
| tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p |
-| tests5.cpp:18:25:18:38 | call to createLSParser | tests5.cpp:20:2:20:2 | p |
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p |
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p |
| tests.cpp:53:19:53:19 | VariableAddress [post update] | tests.cpp:55:2:55:2 | p |
@@ -33,8 +32,6 @@ nodes
| tests2.cpp:22:2:22:2 | p | semmle.label | p |
| tests2.cpp:33:17:33:31 | SAXParser output argument | semmle.label | SAXParser output argument |
| tests2.cpp:37:2:37:2 | p | semmle.label | p |
-| tests5.cpp:18:25:18:38 | call to createLSParser | semmle.label | call to createLSParser |
-| tests5.cpp:20:2:20:2 | p | semmle.label | p |
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
| tests.cpp:35:2:35:2 | p | semmle.label | p |
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
@@ -74,7 +71,6 @@ subpaths
#select
| tests2.cpp:22:2:22:2 | p | tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:20:17:20:31 | SAXParser output argument | XML parser |
| tests2.cpp:37:2:37:2 | p | tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:33:17:33:31 | SAXParser output argument | XML parser |
-| tests5.cpp:20:2:20:2 | p | tests5.cpp:18:25:18:38 | call to createLSParser | tests5.cpp:20:2:20:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests5.cpp:18:25:18:38 | call to createLSParser | XML parser |
| tests.cpp:35:2:35:2 | p | tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:33:23:33:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:49:2:49:2 | p | tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:46:23:46:43 | XercesDOMParser output argument | XML parser |
| tests.cpp:57:2:57:2 | p | tests.cpp:53:23:53:43 | XercesDOMParser output argument | tests.cpp:57:2:57:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:53:23:53:43 | XercesDOMParser output argument | XML parser |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
index 3f3bfad92df..477107d97fe 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
@@ -4,7 +4,16 @@
// ---
-class DOMLSParser : public AbstractDOMParser {
+class DOMConfiguration {
+public:
+ void setParameter(const XMLCh *parameter, bool value);
+};
+
+class DOMLSParser {
+public:
+ DOMConfiguration *getDomConfig();
+
+ void parse(const InputSource &data);
};
class DOMImplementationLS {
@@ -17,13 +26,13 @@ public:
void test5_1(DOMImplementationLS *impl, InputSource &data) {
DOMLSParser *p = impl->createLSParser();
- p->parse(data); // BAD (parser not correctly configured)
+ p->parse(data); // BAD (parser not correctly configured) [NOT DETECTED]
}
void test5_2(DOMImplementationLS *impl, InputSource &data) {
DOMLSParser *p = impl->createLSParser();
- p->setDisableDefaultEntityResolution(true);
+ p->getDomConfig()->setParameter(XMLUni::fgXercesDisableDefaultEntityResolution, true);
p->parse(data); // GOOD
}
@@ -33,7 +42,7 @@ InputSource *g_data;
void test5_3_init() {
g_p1 = g_impl->createLSParser();
- g_p1->setDisableDefaultEntityResolution(true);
+ g_p1->getDomConfig()->setParameter(XMLUni::fgXercesDisableDefaultEntityResolution, true);
g_p2 = g_impl->createLSParser();
}
From dd258781ed9c4eaa6dd452a913cdc640a9aad470 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 18:14:39 +0100
Subject: [PATCH 0210/1618] C++: More test cases.
---
.../Security/CWE/CWE-611/tests5.cpp | 29 +++++++++++++++++--
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
index 477107d97fe..e98d5a99e60 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests5.cpp
@@ -36,19 +36,42 @@ void test5_2(DOMImplementationLS *impl, InputSource &data) {
p->parse(data); // GOOD
}
+void test5_3(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
+
+ p->getDomConfig()->setParameter(XMLUni::fgXercesDisableDefaultEntityResolution, false);
+ p->parse(data); // BAD (parser not correctly configured) [NOT DETECTED]
+}
+
+void test5_4(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
+ DOMConfiguration *cfg = p->getDomConfig();
+
+ cfg->setParameter(XMLUni::fgXercesDisableDefaultEntityResolution, true);
+ p->parse(data); // GOOD
+}
+
+void test5_5(DOMImplementationLS *impl, InputSource &data) {
+ DOMLSParser *p = impl->createLSParser();
+ DOMConfiguration *cfg = p->getDomConfig();
+
+ cfg->setParameter(XMLUni::fgXercesDisableDefaultEntityResolution, false);
+ p->parse(data); // BAD (parser not correctly configured) [NOT DETECTED]
+}
+
DOMImplementationLS *g_impl;
DOMLSParser *g_p1, *g_p2;
InputSource *g_data;
-void test5_3_init() {
+void test5_6_init() {
g_p1 = g_impl->createLSParser();
g_p1->getDomConfig()->setParameter(XMLUni::fgXercesDisableDefaultEntityResolution, true);
g_p2 = g_impl->createLSParser();
}
-void test5_3() {
- test5_3_init();
+void test5_6() {
+ test5_6_init();
g_p1->parse(*g_data); // GOOD
g_p2->parse(*g_data); // BAD (parser not correctly configured) [NOT DETECTED]
From a0e003e33ce836f5eeb7b0e88e26b2a15e4762e1 Mon Sep 17 00:00:00 2001
From: Tom Hvitved
Date: Fri, 29 Apr 2022 11:59:51 +0200
Subject: [PATCH 0211/1618] C#: Add FP test for `cs/useless-cast-to-self`
---
.../Language Abuse/UselessCastToSelf/UselessCastToSelf.cs | 2 ++
.../UselessCastToSelf/UselessCastToSelf.expected | 5 +++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.cs b/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.cs
index 2c6ea6bafbf..e3aa1ad3067 100644
--- a/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.cs
+++ b/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.cs
@@ -22,6 +22,7 @@ class Test
var good7 = (Action)((int x) => { });
func = x => x;
exprFunc = x => x;
+ exprFuncUntyped = (Expression>)(x => x); // FP
}
enum Enum
@@ -35,4 +36,5 @@ class Test
private Func func;
private Expression> exprFunc;
+ private LambdaExpression exprFuncUntyped;
}
diff --git a/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.expected b/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.expected
index 7310437e1ef..11ac047b255 100644
--- a/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.expected
+++ b/csharp/ql/test/query-tests/Language Abuse/UselessCastToSelf/UselessCastToSelf.expected
@@ -2,5 +2,6 @@
| UselessCastToSelf.cs:10:20:10:29 | (...) ... | This cast is redundant because the expression already has type Test. |
| UselessCastToSelf.cs:11:20:11:31 | ... as ... | This cast is redundant because the expression already has type Test. |
| UselessCastToSelf.cs:13:20:13:56 | (...) ... | This cast is redundant because the expression already has type Expression>>. |
-| UselessCastToSelf.cs:31:17:31:22 | (...) ... | This cast is redundant because the expression already has type Int32. |
-| UselessCastToSelf.cs:33:24:33:29 | (...) ... | This cast is redundant because the expression already has type Int32. |
+| UselessCastToSelf.cs:25:27:25:63 | (...) ... | This cast is redundant because the expression already has type Expression>>. |
+| UselessCastToSelf.cs:32:17:32:22 | (...) ... | This cast is redundant because the expression already has type Int32. |
+| UselessCastToSelf.cs:34:24:34:29 | (...) ... | This cast is redundant because the expression already has type Int32. |
From 12320aa5d28ee7af8a82d51b9e5c82fdb8373d4b Mon Sep 17 00:00:00 2001
From: Tony Torralba
Date: Fri, 29 Apr 2022 11:29:44 +0200
Subject: [PATCH 0212/1618] Fix Intent Redirection sanitizer
---
...22-04-29-intent-redirection-sanitizer-fix.md | 5 +++++
.../java/security/AndroidIntentRedirection.qll | 14 +++++++++++---
.../CWE-940/AndroidIntentRedirectionTest.java | 17 +++++++++++++----
3 files changed, 29 insertions(+), 7 deletions(-)
create mode 100644 java/ql/lib/change-notes/2022-04-29-intent-redirection-sanitizer-fix.md
diff --git a/java/ql/lib/change-notes/2022-04-29-intent-redirection-sanitizer-fix.md b/java/ql/lib/change-notes/2022-04-29-intent-redirection-sanitizer-fix.md
new file mode 100644
index 00000000000..66fa93ec4db
--- /dev/null
+++ b/java/ql/lib/change-notes/2022-04-29-intent-redirection-sanitizer-fix.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+Fixed a sanitizer of the query `java/android/intent-redirection`. Now, for an intent to be considered
+safe against intent redirection, both its package name and class name must be checked.
\ No newline at end of file
diff --git a/java/ql/lib/semmle/code/java/security/AndroidIntentRedirection.qll b/java/ql/lib/semmle/code/java/security/AndroidIntentRedirection.qll
index 4a89b59f8c9..c549784ccbf 100644
--- a/java/ql/lib/semmle/code/java/security/AndroidIntentRedirection.qll
+++ b/java/ql/lib/semmle/code/java/security/AndroidIntentRedirection.qll
@@ -65,16 +65,24 @@ private class DefaultIntentRedirectionSink extends IntentRedirectionSink {
}
/**
- * A default sanitizer for nodes dominated by calls to `ComponentName.getPackageName`
- * or `ComponentName.getClassName`. These are used to check whether the origin or destination
+ * A default sanitizer for `Intent` nodes dominated by calls to `ComponentName.getPackageName`
+ * and `ComponentName.getClassName`. These are used to check whether the origin or destination
* components are trusted.
*/
private class DefaultIntentRedirectionSanitizer extends IntentRedirectionSanitizer {
DefaultIntentRedirectionSanitizer() {
+ this.getType() instanceof TypeIntent and
exists(MethodAccess ma, Method m, Guard g, boolean branch |
ma.getMethod() = m and
m.getDeclaringType() instanceof TypeComponentName and
- m.hasName(["getPackageName", "getClassName"]) and
+ m.hasName("getPackageName") and
+ g.isEquality(ma, _, branch) and
+ g.controls(this.asExpr().getBasicBlock(), branch)
+ ) and
+ exists(MethodAccess ma, Method m, Guard g, boolean branch |
+ ma.getMethod() = m and
+ m.getDeclaringType() instanceof TypeComponentName and
+ m.hasName("getClassName") and
g.isEquality(ma, _, branch) and
g.controls(this.asExpr().getBasicBlock(), branch)
)
diff --git a/java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.java b/java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.java
index c577ca96620..2ce945461b6 100644
--- a/java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.java
+++ b/java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.java
@@ -40,13 +40,23 @@ public class AndroidIntentRedirectionTest extends Activity {
sendStickyOrderedBroadcastAsUser(intent, null, null, null, 0, null, null); // $ hasAndroidIntentRedirection
// @formatter:on
+ // Sanitizing only the package or the class still allows redirecting
+ // to non-exported activities in the same package
+ // or activities with the same name in other packages, respectively.
if (intent.getComponent().getPackageName().equals("something")) {
- startActivity(intent); // Safe - sanitized
+ startActivity(intent); // $ hasAndroidIntentRedirection
} else {
startActivity(intent); // $ hasAndroidIntentRedirection
}
if (intent.getComponent().getClassName().equals("something")) {
- startActivity(intent); // Safe - sanitized
+ startActivity(intent); // $ hasAndroidIntentRedirection
+ } else {
+ startActivity(intent); // $ hasAndroidIntentRedirection
+ }
+
+ if (intent.getComponent().getPackageName().equals("something")
+ && intent.getComponent().getClassName().equals("something")) {
+ startActivity(intent); // Safe
} else {
startActivity(intent); // $ hasAndroidIntentRedirection
}
@@ -94,8 +104,7 @@ public class AndroidIntentRedirectionTest extends Activity {
}
{
Intent fwdIntent = new Intent();
- ComponentName component =
- new ComponentName("", intent.getStringExtra("className"));
+ ComponentName component = new ComponentName("", intent.getStringExtra("className"));
fwdIntent.setComponent(component);
startActivity(fwdIntent); // $ hasAndroidIntentRedirection
}
From 08b6b1d2094603802c14ef9213ff8ca2086f8c65 Mon Sep 17 00:00:00 2001
From: Henry Mercer
Date: Fri, 29 Apr 2022 11:26:32 +0100
Subject: [PATCH 0213/1618] Use codeql-action/upload-sarif@main in CSV coverage
metrics workflow
---
.github/workflows/csv-coverage-metrics.yml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/csv-coverage-metrics.yml b/.github/workflows/csv-coverage-metrics.yml
index a48bbc4def6..7778221dc2f 100644
--- a/.github/workflows/csv-coverage-metrics.yml
+++ b/.github/workflows/csv-coverage-metrics.yml
@@ -38,7 +38,7 @@ jobs:
path: metrics-java.sarif
retention-days: 20
- name: Upload SARIF file
- uses: github/codeql-action/upload-sarif@v1
+ uses: github/codeql-action/upload-sarif@main
with:
sarif_file: metrics-java.sarif
@@ -65,6 +65,6 @@ jobs:
path: metrics-csharp.sarif
retention-days: 20
- name: Upload SARIF file
- uses: github/codeql-action/upload-sarif@v1
+ uses: github/codeql-action/upload-sarif@main
with:
sarif_file: metrics-csharp.sarif
From 812a24fc1846af1247abc7e2710a9b0cb7a871c9 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Apr 2022 12:53:00 +0100
Subject: [PATCH 0214/1618] C++: Add test cases for libxml2.
---
.../query-tests/Security/CWE/CWE-611/tests.h | 2 +
.../Security/CWE/CWE-611/tests4.cpp | 135 ++++++++++++++++++
2 files changed, 137 insertions(+)
create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-611/tests4.cpp
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
index aa4c539bbd9..fa67239a8bc 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
@@ -2,3 +2,5 @@
class SecurityManager;
class InputSource;
+
+#define NULL (0)
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests4.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests4.cpp
new file mode 100644
index 00000000000..40197d2c0ee
--- /dev/null
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests4.cpp
@@ -0,0 +1,135 @@
+// test cases for rule CWE-611 (libxml2)
+
+#include "tests.h"
+
+// ---
+
+enum xmlParserOption
+{
+ XML_PARSE_NOENT = 2,
+ XML_PARSE_DTDLOAD = 4,
+ XML_PARSE_OPTION_HARMLESS = 8
+};
+
+class xmlDoc;
+
+xmlDoc *xmlReadFile(const char *fileName, const char *encoding, int flags);
+xmlDoc *xmlReadMemory(const char *ptr, int sz, const char *url, const char *encoding, int flags);
+
+void xmlFreeDoc(xmlDoc *ptr);
+
+// ---
+
+void test4_1(const char *fileName) {
+ xmlDoc *p;
+
+ p = xmlReadFile(fileName, NULL, XML_PARSE_NOENT); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_2(const char *fileName) {
+ xmlDoc *p;
+
+ p = xmlReadFile(fileName, NULL, XML_PARSE_DTDLOAD); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_3(const char *fileName) {
+ xmlDoc *p;
+
+ p = xmlReadFile(fileName, NULL, XML_PARSE_NOENT | XML_PARSE_DTDLOAD); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_4(const char *fileName) {
+ xmlDoc *p;
+
+ p = xmlReadFile(fileName, NULL, 0); // GOOD
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_5(const char *fileName) {
+ xmlDoc *p;
+
+ p = xmlReadFile(fileName, NULL, XML_PARSE_OPTION_HARMLESS); // GOOD
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_6(const char *fileName) {
+ xmlDoc *p;
+ int flags = XML_PARSE_NOENT;
+
+ p = xmlReadFile(fileName, NULL, flags); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_7(const char *fileName) {
+ xmlDoc *p;
+ int flags = 0;
+
+ p = xmlReadFile(fileName, NULL, flags); // GOOD
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_8(const char *fileName) {
+ xmlDoc *p;
+ int flags = XML_PARSE_OPTION_HARMLESS;
+
+ p = xmlReadFile(fileName, NULL, flags | XML_PARSE_NOENT); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_9(const char *fileName) {
+ xmlDoc *p;
+ int flags = XML_PARSE_NOENT;
+
+ p = xmlReadFile(fileName, NULL, flags | XML_PARSE_OPTION_HARMLESS); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_10(const char *ptr, int sz) {
+ xmlDoc *p;
+
+ p = xmlReadMemory(ptr, sz, "", NULL, 0); // GOOD
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
+
+void test4_11(const char *ptr, int sz) {
+ xmlDoc *p;
+
+ p = xmlReadMemory(ptr, sz, "", NULL, XML_PARSE_DTDLOAD); // BAD (parser not correctly configured) [NOT DETECTED]
+ if (p != NULL)
+ {
+ xmlFreeDoc(p);
+ }
+}
From 37b051a851b8a4a736ff4111b9be4e937c1ac482 Mon Sep 17 00:00:00 2001
From: Jorge <46056498+jorgectf@users.noreply.github.com>
Date: Fri, 29 Apr 2022 14:44:17 +0200
Subject: [PATCH 0215/1618] Apply suggestions from code review
Co-authored-by: Anders Schack-Mulligen