diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index fbeb7ef9a2c..bf6adf9408f 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -185,6 +185,44 @@ predicate summaryModel( ) } +/** + * Holds if the given extension tuple `madId` should pretty-print as `model`. + * + * This predicate should only be used in tests. + */ +predicate interpretModelForTest(QlBuiltins::ExtensionId madId, string model) { + exists( + string package, string type, boolean subtypes, string name, string signature, string ext, + string output, string kind, string provenance + | + sourceModel(package, type, subtypes, name, signature, ext, output, kind, provenance, madId) and + model = + "Source: " + package + "; " + type + "; " + subtypes + "; " + name + "; " + signature + "; " + + ext + "; " + output + "; " + kind + "; " + provenance + ) + or + exists( + string package, string type, boolean subtypes, string name, string signature, string ext, + string input, string kind, string provenance + | + sinkModel(package, type, subtypes, name, signature, ext, input, kind, provenance, madId) and + model = + "Sink: " + package + "; " + type + "; " + subtypes + "; " + name + "; " + signature + "; " + + ext + "; " + input + "; " + kind + "; " + provenance + ) + or + exists( + string package, string type, boolean subtypes, string name, string signature, string ext, + string input, string output, string kind, string provenance + | + summaryModel(package, type, subtypes, name, signature, ext, input, output, kind, provenance, + madId) and + model = + "Summary: " + package + "; " + type + "; " + subtypes + "; " + name + "; " + signature + "; " + + ext + "; " + input + "; " + output + "; " + kind + "; " + provenance + ) +} + /** Holds if a neutral model exists for the given parameters. */ predicate neutralModel = Extensions::neutralModel/6; diff --git a/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.expected b/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.expected index 45acb1c6c6f..5d1117393e6 100644 --- a/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.expected +++ b/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.expected @@ -1,17 +1,21 @@ +models +| 1 | Sink: java.net; URL; false; openConnection; ; ; Argument[this]; request-forgery; manual | +| 2 | Summary: java.net; URL; false; URL; (String); ; Argument[0]; Argument[this]; taint; manual | +| 3 | Summary: java.net; URL; false; URL; (URL,String); ; Argument[1]; Argument[this]; taint; ai-manual | edges | HttpsUrlsTest.java:23:23:23:31 | "http://" : String | HttpsUrlsTest.java:24:21:24:56 | ... + ... : String | provenance | | -| HttpsUrlsTest.java:24:13:24:57 | new URL(...) : URL | HttpsUrlsTest.java:28:50:28:50 | u | provenance | Sink:MaD:42944 | +| HttpsUrlsTest.java:24:13:24:57 | new URL(...) : URL | HttpsUrlsTest.java:28:50:28:50 | u | provenance | Sink:MaD:1 | | HttpsUrlsTest.java:24:21:24:56 | ... + ... : String | HttpsUrlsTest.java:24:13:24:57 | new URL(...) : URL | provenance | Config | -| HttpsUrlsTest.java:24:21:24:56 | ... + ... : String | HttpsUrlsTest.java:24:13:24:57 | new URL(...) : URL | provenance | MaD:42977 | +| HttpsUrlsTest.java:24:21:24:56 | ... + ... : String | HttpsUrlsTest.java:24:13:24:57 | new URL(...) : URL | provenance | MaD:2 | | HttpsUrlsTest.java:36:23:36:28 | "http" : String | HttpsUrlsTest.java:37:21:37:28 | protocol : String | provenance | | -| HttpsUrlsTest.java:37:13:37:62 | new URL(...) : URL | HttpsUrlsTest.java:41:50:41:50 | u | provenance | Sink:MaD:42944 | +| HttpsUrlsTest.java:37:13:37:62 | new URL(...) : URL | HttpsUrlsTest.java:41:50:41:50 | u | provenance | Sink:MaD:1 | | HttpsUrlsTest.java:37:21:37:28 | protocol : String | HttpsUrlsTest.java:37:13:37:62 | new URL(...) : URL | provenance | Config | | HttpsUrlsTest.java:49:23:49:31 | "http://" : String | HttpsUrlsTest.java:51:64:51:98 | ... + ... : String | provenance | | -| HttpsUrlsTest.java:51:13:51:99 | new URL(...) : URL | HttpsUrlsTest.java:55:50:55:50 | u | provenance | Sink:MaD:42944 | +| HttpsUrlsTest.java:51:13:51:99 | new URL(...) : URL | HttpsUrlsTest.java:55:50:55:50 | u | provenance | Sink:MaD:1 | | HttpsUrlsTest.java:51:64:51:98 | ... + ... : String | HttpsUrlsTest.java:51:13:51:99 | new URL(...) : URL | provenance | Config | -| HttpsUrlsTest.java:51:64:51:98 | ... + ... : String | HttpsUrlsTest.java:51:13:51:99 | new URL(...) : URL | provenance | MaD:42985 | +| HttpsUrlsTest.java:51:64:51:98 | ... + ... : String | HttpsUrlsTest.java:51:13:51:99 | new URL(...) : URL | provenance | MaD:3 | | HttpsUrlsTest.java:87:23:87:28 | "http" : String | HttpsUrlsTest.java:88:21:88:28 | protocol : String | provenance | | -| HttpsUrlsTest.java:88:13:88:52 | new URL(...) : URL | HttpsUrlsTest.java:92:50:92:50 | u | provenance | Sink:MaD:42944 | +| HttpsUrlsTest.java:88:13:88:52 | new URL(...) : URL | HttpsUrlsTest.java:92:50:92:50 | u | provenance | Sink:MaD:1 | | HttpsUrlsTest.java:88:21:88:28 | protocol : String | HttpsUrlsTest.java:88:13:88:52 | new URL(...) : URL | provenance | Config | nodes | HttpsUrlsTest.java:23:23:23:31 | "http://" : String | semmle.label | "http://" : String | diff --git a/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.ql b/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.ql new file mode 100644 index 00000000000..1175b676939 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.ql @@ -0,0 +1,14 @@ +/** + * @kind path-problem + */ + +import java +import semmle.code.java.security.HttpsUrlsQuery +import codeql.dataflow.test.ProvenancePathGraph +import semmle.code.java.dataflow.ExternalFlow +import ShowProvenance + +from HttpStringToUrlOpenMethodFlow::PathNode source, HttpStringToUrlOpenMethodFlow::PathNode sink +where HttpStringToUrlOpenMethodFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "URL may have been constructed with HTTP protocol, using $@.", + source.getNode(), "this HTTP URL" diff --git a/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.qlref b/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.qlref deleted file mode 100644 index bd936a400c0..00000000000 --- a/java/ql/test/query-tests/security/CWE-311/CWE-319/HttpsUrls.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-319/HttpsUrls.ql \ No newline at end of file diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index 67a067e659c..3b1a34d0f21 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -3905,10 +3905,12 @@ module MakeImpl Lang> { final predicate isSinkGroup(string group) { this = TPathNodeSinkGroup(group) } } + private import codeql.dataflow.test.ProvenancePathGraph as ProvenancePathGraph + /** * Provides the query predicates needed to include a graph in a path-problem query. */ - module PathGraph implements PathGraphSig { + module PathGraph implements PathGraphSig, ProvenancePathGraph::PathGraphSig { /** Holds if `(a,b)` is an edge in the graph of data flow path explanations. */ query predicate edges(PathNode a, PathNode b, string key, string val) { a.(PathNodeImpl).getANonHiddenSuccessor(val) = b and diff --git a/shared/dataflow/codeql/dataflow/test/ProvenancePathGraph.qll b/shared/dataflow/codeql/dataflow/test/ProvenancePathGraph.qll new file mode 100644 index 00000000000..b02f8277b7b --- /dev/null +++ b/shared/dataflow/codeql/dataflow/test/ProvenancePathGraph.qll @@ -0,0 +1,82 @@ +/** + * Provides a module for renumbering MaD IDs in data flow path explanations in + * order to produce more stable test output. + * + * In addition to the `PathGraph`, a `query predicate models` is provided to + * list the contents of the referenced MaD rows. + */ +signature predicate interpretModelForTestSig(QlBuiltins::ExtensionId madId, string model); + +signature class PathNodeSig { + string toString(); +} + +signature module PathGraphSig { + /** Holds if `(a,b)` is an edge in the graph of data flow path explanations. */ + predicate edges(PathNode a, PathNode b, string key, string val); + + /** Holds if `n` is a node in the graph of data flow path explanations. */ + predicate nodes(PathNode n, string key, string val); + + /** + * Holds if `(arg, par, ret, out)` forms a subpath-tuple, that is, flow through + * a subpath between `par` and `ret` with the connecting edges `arg -> par` and + * `ret -> out` is summarized as the edge `arg -> out`. + */ + predicate subpaths(PathNode arg, PathNode par, PathNode ret, PathNode out); +} + +module ShowProvenance< + interpretModelForTestSig/2 interpretModelForTest, PathNodeSig PathNode, + PathGraphSig PathGraph> +{ + private predicate madIds(string madId) { + exists(string model | + PathGraph::edges(_, _, _, model) and + model.regexpFind("(?<=MaD:)[0-9]*", _, _) = madId + ) + } + + private predicate rankedMadIds(string madId, int r) { + madId = rank[r](string madId0 | madIds(madId0) | madId0 order by madId0.toInt()) + } + + query predicate models(int r, string model) { + exists(QlBuiltins::ExtensionId madId | + rankedMadIds(madId.toString(), r) and interpretModelForTest(madId, model) + ) + } + + private predicate translateModelsPart(string model1, string model2, int i) { + PathGraph::edges(_, _, _, model1) and + exists(string s | model1.splitAt("MaD:", i) = s | + model2 = s and i = 0 + or + exists(string part, string madId, string rest, int r | + translateModelsPart(model1, part, i - 1) and + madId = s.regexpCapture("([0-9]*)(.*)", 1) and + rest = s.regexpCapture("([0-9]*)(.*)", 2) and + rankedMadIds(madId, r) and + model2 = part + "MaD:" + r + rest + ) + ) + } + + private predicate translateModels(string model1, string model2) { + exists(int i | + translateModelsPart(model1, model2, i) and + not translateModelsPart(model1, _, i + 1) + ) + } + + query predicate edges(PathNode a, PathNode b, string key, string val) { + exists(string model | + PathGraph::edges(a, b, key, model) and + translateModels(model, val) + ) + } + + query predicate nodes = PathGraph::nodes/3; + + query predicate subpaths = PathGraph::subpaths/4; +}