Java: Add proper support for variable capture flow. #13478

aschackmull · 2023-06-16T12:28:19Z

This PR adds a new shared library to implement flow through captured variables that adhere to inter-procedural control flow and enables its use for Java.

I've named the shared qlpack dataflow, since I expect this qlpack to also eventually contain the data flow library, and I thought it made sense to put the variable-capture library in the same qlpack.

Not yet handled: Support for capture of this.

EDIT: Also not yet handled: Capture of loop variables from enhanced for statements.

EDIT: I recommend reviewing this in its entirety and not commit-by-commit. I've kept the commit history in order to save the first version based on a heap variable, in case we want to look back at that in the future.

shared/dataflow/codeql/dataflow/VariableCapture.qll

shared/dataflow/qlpack.yml

aschackmull · 2023-06-19T09:02:37Z

The couple of new missing flow cases in the ratpack qltest highlights that we need a model for Operation.of that merely states that the callback is called without any additional flow. We might be able to achieve this with an Argument[0] to Argument[0].Parameter[this] model, but we may want different syntax for this, since this is usually implicit for other callback models that actually propagate data from or to their callback parameters. In any case, it probably makes the most sense to follow up on this in a separate PR.

github-actions · 2023-06-19T11:09:27Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Others,"``antlr``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.mitchellbosecke.pebble``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.thoughtworks.xstream``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.hibernate``, ``org.influxdb``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.kohsuke.stapler``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``",102,4467,554,81,4,18,18,,197
+    Others,"``antlr``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.mitchellbosecke.pebble``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.thoughtworks.xstream``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.hibernate``, ``org.influxdb``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.kohsuke.stapler``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``",102,4468,554,81,4,18,18,,197
-    Totals,,259,12766,2023,278,14,122,33,1,387
+    Totals,,259,12767,2023,278,14,122,33,1,387

Changes to framework-coverage-java.csv:

- ratpack.exec,,,48,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,48
+ ratpack.exec,,,49,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,49

github-actions · 2023-06-20T07:08:12Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Others,"``antlr``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.mitchellbosecke.pebble``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.thoughtworks.xstream``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.hibernate``, ``org.influxdb``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.kohsuke.stapler``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``",126,4467,571,89,6,18,18,,200
+    Others,"``antlr``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.mitchellbosecke.pebble``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.thoughtworks.xstream``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.hibernate``, ``org.influxdb``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.kohsuke.stapler``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``",126,4468,571,89,6,18,18,,200
-    Totals,,283,12766,2040,286,16,122,33,1,390
+    Totals,,283,12767,2040,286,16,122,33,1,390

Changes to framework-coverage-java.csv:

- ratpack.exec,,,48,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,48
+ ratpack.exec,,,49,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,49

aschackmull · 2023-06-20T12:42:50Z

Hmm, performance doesn't look too good. We're very likely synthesising way too many nodes.

github-actions · 2023-07-26T10:03:33Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Others,"``antlr``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.mitchellbosecke.pebble``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.thoughtworks.xstream``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.exec``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.lang``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.influxdb``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.kohsuke.stapler``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``",126,5237,577,89,6,18,18,,200
+    Others,"``antlr``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.mitchellbosecke.pebble``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.thoughtworks.xstream``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.exec``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.lang``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.influxdb``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.kohsuke.stapler``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``",126,5238,577,89,6,18,18,,200
-    Totals,,283,13607,2067,290,16,122,33,1,391
+    Totals,,283,13608,2067,290,16,122,33,1,391

Changes to framework-coverage-java.csv:

- ratpack.exec,,,48,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,48
+ ratpack.exec,,,49,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,49

java/ql/src/Security/CWE/CWE-611/XXE.ql

Co-authored-by: Mathias Vorreiter Pedersen <[email protected]>

hvitved

Looks really great, awesome work Anders! Some (minor) comments; can't wait to try this out on Ruby.

hvitved · 2023-08-07T07:56:16Z

shared/dataflow/codeql/dataflow/VariableCapture.qll

+private import codeql.ssa.Ssa as Ssa
+
+signature module InputSig {
+  class Location {


Alternatively, you can use https://github.com/github/codeql/blob/main/shared/util/codeql/util/Location.qll#L9.

Can I? I can't really import that signature into this one, so that would mean splitting the input into two and parameterising this signature over the location-signature, and then I prefer keeping it as-is with just one input to the module.

Yes, it would require using two module parameters.

hvitved · 2023-08-07T07:59:01Z

shared/dataflow/codeql/dataflow/VariableCapture.qll

+    /** Gets the variable that is the target of this write. */
+    CapturedVariable getVariable();
+
+    /** Gets the expression that is the source of this write. */


Hmm, what's a good example where there's no source? Currently I have put in a consistency check that there's supposed to be exactly one source of a write, but there may very well be cases I haven't thought of.

For example out/ref arguments in C#.

But supposedly those writes need incoming flow from the output of those arguments? And that incoming flow will have associated some DataFlow::Node. This is actually likely a good example where it isn't sufficient to simply give the C# class Expr as the input class Expr here - instead you'll possibly need to provide a newtype in order to include something for out/ref arguments, so that you'll actually be able to get that flow as well.
Another example is the enhanced for statements of Java, these also don't have Java Expr sources for the writes, so that's why the capture of loop variables from enhanced for statements currently doesn't work (documented with a test, and I've made and issue to track the followup, and the corresponding consistency check also highlights this problem). In order to fix this, I'll need to replace the Expr input to be a disjoint union of Java Expr and something for enhanced for statements. I'll likely also need to add an additional intermediate DataFlow::Node.

hvitved · 2023-08-07T08:08:46Z

shared/dataflow/codeql/dataflow/VariableCapture.qll

+  }
+
+  /** A write to a captured variable. */
+  class VariableWrite {


Can't this be a sub class of Expr?

It was in an earlier iteration, but I decided that it was cleaner to not have it be an Expr in order to not give any expectations about there being a data flow node for it (since the Exprs here get associated with ExprNodes).

shared/dataflow/codeql/dataflow/VariableCapture.qll

hvitved · 2023-08-08T09:43:42Z

shared/dataflow/codeql/dataflow/VariableCapture.qll

+    TThisParamNode(Callable c) { captureAccess(_, c) } or
+    TMallocNode(ClosureExpr ce) { hasConstructorCapture(ce, _) }
+
+  class ClosureNode extends TClosureNode {


I think we should consider using final classes in all new code going forward. I.e.

private abstract class ClosureNodeImpl extends TClosureNode { abstract string toString(); // ... } final class ClosureNode = ClosureNodeImpl;

I agree that that's probably preferable, although I'm leaving this as-is for now.

hvitved

Btw, you probably also want to hide CaptureNodes using nodeIsHidden.

shared/dataflow/codeql/dataflow/VariableCapture.qll

aschackmull · 2023-08-08T12:00:19Z

Btw, you probably also want to hide CaptureNodes using nodeIsHidden.

Are you sure? I'm probably biased because I generally want to see what's going on - i.e. see most nodes. Perhaps we should expose the functionality to some more people and gather opinions. I'd certainly be open to hiding some or all of them, but only if it doesn't hurt the explainability of the paths.

geoffw0 · 2023-08-15T13:39:05Z

shared/dataflow/codeql/dataflow/VariableCapture.qll

+            captureRead(_, bb, j, _, _) or
+            captureWrite(_, bb, j, _, _) or
+            synthRead(_, bb, j, _, _)
+          ) - 1


What does this part do? It looks as if i can be 0 or -1 or possibly further negative values?

@d10c FYI

It simply ensures that the implicit entry definition comes before all other references (or at index 0 when there are no references).

aschackmull requested a review from a team as a code owner June 16, 2023 12:28

github-actions bot added documentation Java labels Jun 16, 2023

aschackmull force-pushed the java/varcapture branch from d4e14c3 to 4e37b22 Compare June 16, 2023 12:32

github-advanced-security bot found potential problems Jun 16, 2023

View reviewed changes

shared/dataflow/codeql/dataflow/VariableCapture.qll Fixed Show fixed Hide fixed

shared/dataflow/codeql/dataflow/VariableCapture.qll Fixed Show fixed Hide fixed

hvitved reviewed Jun 19, 2023

View reviewed changes

shared/dataflow/qlpack.yml Outdated Show resolved Hide resolved

aschackmull requested review from a team as code owners June 19, 2023 08:58

github-actions bot added C# DataFlow Library Go Python Ruby Swift labels Jun 19, 2023

aschackmull force-pushed the java/varcapture branch from b73f9e0 to 2c575dc Compare June 20, 2023 07:05

aschackmull marked this pull request as draft June 20, 2023 12:44

aschackmull force-pushed the java/varcapture branch from 11d48fe to 942dfa9 Compare July 26, 2023 10:00

github-advanced-security bot found potential problems Jul 26, 2023

View reviewed changes

java/ql/src/Security/CWE/CWE-611/XXE.ql Fixed Show fixed Hide fixed

aschackmull force-pushed the java/varcapture branch from 942dfa9 to 79edfb2 Compare July 26, 2023 11:42

github-actions bot removed C# Python Go labels Jul 26, 2023

aschackmull and others added 13 commits August 3, 2023 10:04

Java: Add proper support for variable capture flow.

d1a616a

Java: Fix qltests.

9a4de20

Java: Disregard heap parameter in any-argument and any-parameter specs.

a23e77c

Shared: Address review comments and QL4QL alerts.

15daa51

Shared: Add some qldoc.

f4a05c1

Java: Fix ratpack flow.

70bef64

Java: Redesign and reimplement variable capture flow.

c599031

Java: Replace ratpack test fix with general heuristic summary.

37455ec

Java: Add more qldoc.

e2a0849

Java: Add proper types for capture nodes.

90052a3

Java: Add more qldoc.

84316c4

Apply suggestions from code review

7217197

Co-authored-by: Mathias Vorreiter Pedersen <[email protected]>

Java: update fixed test

0ae81ea

aschackmull dismissed MathiasVP’s stale review via 0ae81ea August 3, 2023 08:07

aschackmull force-pushed the java/varcapture branch from e5898e3 to 0ae81ea Compare August 3, 2023 08:07

aschackmull added 2 commits August 8, 2023 10:00

Java: Add another test case.

cd22bb3

Java: Always apply heuristic query regardless of existing models.

ab334f6

hvitved reviewed Aug 8, 2023

View reviewed changes

Java: Review fixes.

9d59f50

hvitved reviewed Aug 8, 2023

View reviewed changes

shared/dataflow/codeql/dataflow/VariableCapture.qll Outdated Show resolved Hide resolved

Java: More review fixes.

1cd3272

hvitved approved these changes Aug 8, 2023

View reviewed changes

aschackmull merged commit 0ca3f33 into github:main Aug 8, 2023

aschackmull deleted the java/varcapture branch August 8, 2023 14:23

aschackmull mentioned this pull request Aug 9, 2023

Java data flow: Identify side effects on captured variable in lambda callback. #6906

Closed

geoffw0 reviewed Aug 15, 2023

View reviewed changes

hvitved mentioned this pull request Aug 17, 2023

Ruby: Reimplement flow through captured variables using field flow #11725

Merged

d10c mentioned this pull request Aug 28, 2023

Swift: use shared capture flow library #14078

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add proper support for variable capture flow. #13478

Java: Add proper support for variable capture flow. #13478

aschackmull commented Jun 16, 2023 •

edited

Loading

aschackmull commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

java

Generated file changes for java

github-actions bot commented Jun 20, 2023

java

Generated file changes for java

aschackmull commented Jun 20, 2023

github-actions bot commented Jul 26, 2023

java

Generated file changes for java

hvitved left a comment

hvitved Aug 7, 2023

aschackmull Aug 8, 2023

hvitved Aug 8, 2023

hvitved Aug 7, 2023

aschackmull Aug 8, 2023

hvitved Aug 8, 2023

aschackmull Aug 8, 2023

hvitved Aug 7, 2023

aschackmull Aug 8, 2023

hvitved Aug 8, 2023

aschackmull Aug 8, 2023

hvitved left a comment

aschackmull commented Aug 8, 2023

geoffw0 Aug 15, 2023

hvitved Aug 15, 2023

Java: Add proper support for variable capture flow. #13478

Java: Add proper support for variable capture flow. #13478

Conversation

aschackmull commented Jun 16, 2023 • edited Loading

aschackmull commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

java

Generated file changes for java

github-actions bot commented Jun 20, 2023

java

Generated file changes for java

aschackmull commented Jun 20, 2023

github-actions bot commented Jul 26, 2023

java

Generated file changes for java

hvitved left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved left a comment

Choose a reason for hiding this comment

aschackmull commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschackmull commented Jun 16, 2023 •

edited

Loading