Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: Update Java JDK 17 models. #17547

Merged
merged 18 commits into from
Oct 23, 2024
Merged

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Sep 23, 2024

TLDR

In this PR we re-generate the Java SDK 17 models using the mixed model generator. That is, we use the content based (field aware) model generation where applicable, otherwise we default to the old heuristic for detecting summaries and tainting entire objects. Some minor manual modelling was required after the re-generation. DCA shows

  • No performance regressions.
  • Removal of several false positives as they were flagged due to field conflation (with the non content based models).
  • The new results based on
    • Content based summaries look accurate.
    • Non content based summaries look reasonable.
    • Manual summaries look accurate.

Even though it is hard to say (as there are lot of manual models for the Java SDK), it is my impression, that the mixed model generation improves the precision (quality) or our results.

Details

The sections below contains more detailed information on test failures/updates and changes to alerts.

Some notes on the model and test updates:

  • The update of the request forgery expected output is because the generated models now include ["java.net.http", "HttpRequest$Builder", True, "build", "()", "", "Argument[this]", "ReturnValue", "taint", "df-generated"], which propagates the taint to the send sink as well.
  • TopJdkApisTest.ql: Due to the following neutrals, we no longer generate models for these callables (which is causing the change in the test output):
    • ["java.util.concurrent", "Executor", "execute", "(Runnable)", "summary", "manual"]
    • ["java.util.stream", "IntStream", "mapToObj", "(IntFunction)", "summary", "manual"]
  • ApplicationModeEndpoints.ql: This test reports reports model usages. The new set of models doesn't contain generated models for some of the method overloads for java.io.PrintStream.println. For these to be generated we should have a model for String.valueOf(Object), which is explicitly added as a summary neutral (so this change in the generated models would also have happened even, if we didn't generate mixed models). In any case, the summary models for PrintStream are worthless as it is a wrapper class and there is not any models that retrieve any value from this (unless returning an entire PrintStream object). That is, IMO we can accept these changes.
  • LdapInjection.ql. It turns out that the more precise models for BasicAttributes (which are correct) doesn't work properly in conjunction with the manual sink definition see below. Less precise models that works with the search sink has been added.
    • ["javax.naming.directory", "DirContext", True, "search", "", "", "Argument[0..1]", "ldap-injection", "manual"]
  • The expected test output for the remaining tests have been accepted without any deeper investigation as it didn't effect reported results (only reported edges and nodes).

Some notes on DCA results:

Removed results:

  • java/exec-tainted-environment on apache/hive was a false positive. Before we had the models seen below. However, the constructor only taints the command field, which is not used in the environment method and thus the combination of these models leads to false positive flow.
    • ["java.lang", "ProcessBuilder", False, "ProcessBuilder", "(String[])", "", "Argument[0].ArrayElement", "Argument[this]", "taint", "df-generated"]
    • ["java.lang", "ProcessBuilder", False, "environment", "()", "", "Argument[this]", "ReturnValue", "taint", "ai-manual"]
  • java/sensitive-log on apache/hive were false positives. Same reason as for java/exec-tainted-environment on apache-hive.
  • java/sensitive-log on apache/geode was a false positive. Before we had the models seen below. However, it is only Argument[0] that leads to tainting the returnvalue of getType.
    • ["javax.management", "Notification", True, "Notification", "(String,Object,long)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"]
    • ["javax.management", "Notification", True, "Notification", "(String,Object,long)", "", "Argument[1]", "Argument[this]", "taint", "df-generated"]
    • ["javax.management", "Notification", True, "getType", "()", "", "Argument[this]", "ReturnValue", "taint", "df-generated"]

Added results

  • java/path-injection on WebGoat/WebGoat: Due to the manual ZipFile models (this looks sound).
  • java/sensitive-log on apache/hadoop-common: Due to non content based summaries for javax.security.sasl.SaslServer (this might be a it speculative).
  • java/sensitive-log on apache/solr: Due to non content based summaries for java.xml.stream.XMLStreamReader (these look reasonable).
  • java/zipslip on apache/flink: Due to content based summaries for java.io.File (these summaries looks good).
  • java/zipslip on apache/geode: Same as above

Copy link
Contributor

github-actions bot commented Sep 23, 2024

⚠️ 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:
-    Java Standard Library,``java.*``,10,4284,259,99,,9,,,26
+    Java Standard Library,``java.*``,10,4618,259,99,,9,,,26
-    Java extensions,"``javax.*``, ``jakarta.*``",69,3260,90,10,4,2,1,1,4
+    Java extensions,"``javax.*``, ``jakarta.*``",69,4159,90,10,4,2,1,1,4
-    Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``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.microsoft.sqlserver.jdbc``, ``com.mitchellbosecke.pebble``, ``com.mongodb``, ``com.opensymphony.xwork2``, ``com.rabbitmq.client``, ``com.sshtools.j2ssh.authentication``, ``com.sun.crypto.provider``, ``com.sun.jndi.ldap``, ``com.sun.net.httpserver``, ``com.sun.net.ssl``, ``com.sun.rowset``, ``com.sun.security.auth.module``, ``com.sun.security.ntlm``, ``com.sun.security.sasl.digest``, ``com.thoughtworks.xstream``, ``com.trilead.ssh2``, ``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``, ``io.undertow.server.handlers.resource``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``liquibase.database.jvm``, ``liquibase.statement.core``, ``net.lingala.zip4j``, ``net.schmizz.sshj``, ``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.cxf.catalog``, ``org.apache.cxf.common.classloader``, ``org.apache.cxf.common.jaxb``, ``org.apache.cxf.common.logging``, ``org.apache.cxf.configuration.jsse``, ``org.apache.cxf.helpers``, ``org.apache.cxf.resource``, ``org.apache.cxf.staxutils``, ``org.apache.cxf.tools.corba.utils``, ``org.apache.cxf.tools.util``, ``org.apache.cxf.transform``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hadoop.hive.ql.exec``, ``org.apache.hadoop.hive.ql.metadata``, ``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.ibatis.mapping``, ``org.apache.log4j``, ``org.apache.shiro.authc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.shiro.mgt``, ``org.apache.sshd.client.session``, ``org.apache.struts.beanvalidation.validation.interceptor``, ``org.apache.struts2``, ``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.jboss.vfs``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.keycloak.models.map.storage``, ``org.kohsuke.stapler``, ``org.lastaflute.web``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.owasp.esapi``, ``org.pac4j.jwt.config.encryption``, ``org.pac4j.jwt.config.signature``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.w3c.dom``, ``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``, ``software.amazon.awssdk.transfer.s3.model``, ``sun.awt``, ``sun.jvmstat.perfdata.monitor.protocol.local``, ``sun.jvmstat.perfdata.monitor.protocol.rmi``, ``sun.management.spi``, ``sun.misc``, ``sun.net.ftp``, ``sun.net.www.protocol.http``, ``sun.nio.ch``, ``sun.security.acl``, ``sun.security.jgss.krb5``, ``sun.security.krb5``, ``sun.security.pkcs``, ``sun.security.pkcs11``, ``sun.security.provider``, ``sun.security.ssl``, ``sun.security.x509``, ``sun.tools.jconsole``, ``sun.util.logging.internal``",133,10603,908,140,6,22,18,,208
+    Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``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.microsoft.sqlserver.jdbc``, ``com.mitchellbosecke.pebble``, ``com.mongodb``, ``com.opensymphony.xwork2``, ``com.rabbitmq.client``, ``com.sshtools.j2ssh.authentication``, ``com.sun.crypto.provider``, ``com.sun.jndi.ldap``, ``com.sun.net.httpserver``, ``com.sun.net.ssl``, ``com.sun.rowset``, ``com.sun.security.auth.module``, ``com.sun.security.ntlm``, ``com.sun.security.sasl.digest``, ``com.thoughtworks.xstream``, ``com.trilead.ssh2``, ``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``, ``io.undertow.server.handlers.resource``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``liquibase.database.jvm``, ``liquibase.statement.core``, ``net.lingala.zip4j``, ``net.schmizz.sshj``, ``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.cxf.catalog``, ``org.apache.cxf.common.classloader``, ``org.apache.cxf.common.jaxb``, ``org.apache.cxf.common.logging``, ``org.apache.cxf.configuration.jsse``, ``org.apache.cxf.helpers``, ``org.apache.cxf.resource``, ``org.apache.cxf.staxutils``, ``org.apache.cxf.tools.corba.utils``, ``org.apache.cxf.tools.util``, ``org.apache.cxf.transform``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hadoop.hive.ql.exec``, ``org.apache.hadoop.hive.ql.metadata``, ``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.ibatis.mapping``, ``org.apache.log4j``, ``org.apache.shiro.authc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.shiro.mgt``, ``org.apache.sshd.client.session``, ``org.apache.struts.beanvalidation.validation.interceptor``, ``org.apache.struts2``, ``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.jboss.vfs``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.keycloak.models.map.storage``, ``org.kohsuke.stapler``, ``org.lastaflute.web``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.owasp.esapi``, ``org.pac4j.jwt.config.encryption``, ``org.pac4j.jwt.config.signature``, ``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``, ``software.amazon.awssdk.transfer.s3.model``, ``sun.jvmstat.perfdata.monitor.protocol.local``, ``sun.jvmstat.perfdata.monitor.protocol.rmi``, ``sun.misc``, ``sun.net.ftp``, ``sun.net.www.protocol.http``, ``sun.security.acl``, ``sun.security.jgss.krb5``, ``sun.security.krb5``, ``sun.security.pkcs``, ``sun.security.pkcs11``, ``sun.security.provider``, ``sun.security.ssl``, ``sun.security.x509``, ``sun.tools.jconsole``",133,10525,908,140,6,22,18,,208
-    Totals,,312,25170,2635,404,16,128,33,1,409
+    Totals,,312,26325,2635,404,16,128,33,1,409
  • Changes to framework-coverage-java.csv:
- java.applet,,,14,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,14,
+ java.applet,,,11,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,11,
- java.awt,1,,5,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,2,3
+ java.awt,1,,3,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,3
- java.beans,,,193,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,193,
+ java.beans,,,177,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,82,95
- java.io,66,1,256,,,,,,,,,22,,,,,,,,,,,,,,,44,,,,,,,,,,,,,,,,,,,,,,1,,249,7
+ java.io,66,1,225,,,,,,,,,22,,,,,,,,,,,,,,,44,,,,,,,,,,,,,,,,,,,,,,1,,202,23
- java.lang,38,3,756,,13,,,,,,1,,,,,,,,,,,,8,,,,11,,,4,,,1,,,,,,,,,,,,,,,3,,,681,75
+ java.lang,38,3,783,,13,,,,,,1,,,,,,,,,,,,8,,,,11,,,4,,,1,,,,,,,,,,,,,,,3,,,506,277
- java.net,23,3,279,,,,1,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,21,,,,,,,,,,,,,,3,275,4
+ java.net,23,3,347,,,,1,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,21,,,,,,,,,,,,,,3,248,99
- java.nio,47,,373,,,,,,,,,5,,,,,,,,,,,,,,,41,,,,,,,,,1,,,,,,,,,,,,,,,267,106
+ java.nio,47,,499,,,,,,,,,5,,,,,,,,,,,,,,,41,,,,,,,,,1,,,,,,,,,,,,,,,302,197
- java.rmi,,,71,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,71,
+ java.rmi,,,68,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,45,23
- java.security,21,,547,,,11,10,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,543,4
+ java.security,21,,583,,,11,10,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,285,298
- java.sql,15,1,303,,,,1,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,4,,9,,,,,,,,,1,,,,303,
+ java.sql,15,1,292,,,,1,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,4,,9,,,,,,,,,1,,,,274,18
- java.text,,,134,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,134,
+ java.text,,,154,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,72,82
- java.time,,,123,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,35,88
+ java.time,,,131,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,27,104
- java.util,48,2,1221,,,,,,,,,1,,,,,,,,,,,34,,,,3,,,,5,2,,1,2,,,,,,,,,,,,,2,,,705,516
+ java.util,48,2,1336,,,,,,,,,1,,,,,,,,,,,34,,,,3,,,,5,2,,1,2,,,,,,,,,,,,,2,,,558,778
- javax.accessibility,,,31,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,31,
+ javax.accessibility,,,63,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,28,35
- javax.annotation.processing,,,9,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,9,
+ javax.annotation.processing,,,28,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,25,3
- javax.crypto,19,,128,,,12,3,,2,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,128,
+ javax.crypto,19,,114,,,12,3,,2,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,61,53
- javax.imageio,1,,261,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,,,,,,,,,261,
+ javax.imageio,1,,304,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,,,,,,,,,138,166
- javax.lang.model.element,,,17,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,17,
+ javax.lang.model,,,277,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,217,60
- javax.lang.model.type,,,9,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,9,
- javax.lang.model.util,,,68,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,68,
- javax.management,2,,802,,,,,,,,,,,,,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,801,1
+ javax.management,2,,766,,,,,,,,,,,,,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,363,403
- javax.naming,7,,324,,,,,,,,,,,,,,,,,6,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,318,6
+ javax.naming,7,,341,,,,,,,,,,,,,,,,,6,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,191,150
- javax.net,4,,86,,,,2,,,,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,86,
+ javax.net,4,,136,,,,2,,,,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,87,49
- javax.portlet,1,,61,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,61,
+ javax.portlet,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,
- javax.print,2,,100,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,100,
+ javax.print,2,,133,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,102,31
- javax.rmi.ssl,,,7,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,7,
+ javax.rmi.ssl,,,6,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,6
- javax.script,1,,42,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,42,
+ javax.script,1,,50,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,14,36
- javax.security.auth,7,,137,,,4,3,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,137,
+ javax.security.auth,7,,147,,,4,3,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,50,97
- javax.security.sasl,,,28,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,28,
+ javax.security.sasl,,,49,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,42,7
- javax.smartcardio,,,30,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,30,
+ javax.smartcardio,,,34,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,24,10
- javax.sound.midi,,,29,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,29,
+ javax.sound.midi,,,60,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,51,9
- javax.sound.sampled,,,66,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,66,
+ javax.sound.sampled,,,90,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,53,37
- javax.sql,7,,63,,,,4,3,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,63,
+ javax.sql,7,,126,,,,4,3,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,68,58
- javax.tools,,,31,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,31,
+ javax.tools,,,66,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,62,4
- javax.xml.catalog,,,11,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,10,1
+ javax.xml.catalog,,,12,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,11,1
- javax.xml.crypto,,,126,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,126,
+ javax.xml.crypto,,,269,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,172,97
- javax.xml.datatype,,,4,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,4,
+ javax.xml.datatype,,,6,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,5,1
- javax.xml.namespace,,,10,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,10,
+ javax.xml.namespace,,,15,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,5,10
- javax.xml.parsers,,,18,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,18,
+ javax.xml.parsers,,,37,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,35,2
- javax.xml.stream,,,36,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,36,
+ javax.xml.stream,,,221,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,201,20
- javax.xml.transform,2,,89,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,,1,,,,,,,89,
+ javax.xml.transform,2,,134,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,,1,,,,,,,72,62
- javax.xml.validation,,,5,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,5,
+ javax.xml.validation,,,29,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,29,
- javax.xml.xpath,3,,12,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,3,,,,,,,,12,
+ javax.xml.xpath,3,,26,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,3,,,,,,,,26,
- org.w3c.dom,,,57,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,56,1
- org.xml.sax,,,4,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,4,
+ org.xml.sax,,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,
- sun.awt,,,5,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,5,
- sun.management.spi,,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,
- sun.nio.ch,,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,
- sun.security.krb5,9,,7,,,3,6,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,7,
+ sun.security.krb5,9,,,,,3,6,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
- sun.util.logging.internal,,,3,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,3,

owen-mc
owen-mc previously approved these changes Oct 10, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. I don't totally follow your reasoning on PrintStream, but I'm happy to accept your decision.

I note that we make quite a lot of models for methods that override Object.clone(). There is special treatment for this here and here, so we don't need to model these methods. Is it worth putting an exclusion in the model generator for this? (We do have various manual models for it.)

owen-mc
owen-mc previously approved these changes Oct 11, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for dealing with the clone methods

@michaelnebel
Copy link
Contributor Author

It looks good. I don't totally follow your reasoning on PrintStream, but I'm happy to accept your decision.

I note that we make quite a lot of models for methods that override Object.clone(). There is special treatment for this here and here, so we don't need to model these methods. Is it worth putting an exclusion in the model generator for this? (We do have various manual models for it.)

Thank you very much for the review and thank you for proactively asking on slack!!!

  • Yes, it is indeed a good idea to exclude clone methods from the model generation. Thank you!
  • After re-reading the description of why PrintStream models doesn't provide much value, I see that it is a bit convoluted: The PrintStream class is a wrapper like class which doesn't expose its internals after being created. Having summaries for such a class is of limited (or no value), so we can accept the removal (however, for sink modelling it would be a different story).

@michaelnebel
Copy link
Contributor Author

@owen-mc : Needed to update one more test for the CI to pass. I intend to also run QA job before merging this PR (when I get back from PTO).

@aschackmull
Copy link
Contributor

The detailed summary sounds good to me. I noticed one thing, though: "java.util.stream", "IntStream", "mapToObj" should not be neutral - it ought to have a manual value model from "Argument[0].ReturnValue" to "ReturnValue.Element". The same goes for mapToObj on the other specialised stream classes, DoubleStream and LongStream.

@michaelnebel
Copy link
Contributor Author

The detailed summary sounds good to me. I noticed one thing, though: "java.util.stream", "IntStream", "mapToObj" should not be neutral - it ought to have a manual value model from "Argument[0].ReturnValue" to "ReturnValue.Element". The same goes for mapToObj on the other specialised stream classes, DoubleStream and LongStream.

That makes sense. I think that should be considered potential follow up work.
Will start a QA run.

@michaelnebel
Copy link
Contributor Author

The alert counts diff of a QA run of this branch (after a rebase) vs the latest nightly bundle shows
image

The QA run shows variations on a few repositories for path-injection, sensitive-log and log-injection, which are also the queries that produces the most results (by far). Since we don't see any particular overall decrease or increase in the alert count, I think we should accept these results without further investigation (the discrepancies were thoroughly investigated for the DCA execution and the conclusion was that we now produce more precise results with the new models).

@aschackmull @owen-mc : Do you have any objections? Otherwise, I think we should go ahead and merge the new models.

@michaelnebel michaelnebel merged commit 197642c into github:main Oct 23, 2024
15 checks passed
@michaelnebel michaelnebel deleted the java/jdk17update branch October 23, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants