Java: FileUpload Support MaD

[Kwstubbs]

This PR adds javax.servlet.http.Part and org.apache.commons.fileupload.FileItem/Stream support to RemoteFlow Sources.

[Kwstubbs]

I added basic tests and stubs to the library-tests folder.
I have two questions:

1. I manually added a stub for FileItemHeadersSupport. The java compiler seems to allow the import and private field, but errors out when I attempt to use the variable within the test method. I am not sure what is wrong with my code.
2. Should I add stubs and tests for jakarta.servlet.http.model.yml?

[github-actions]

⚠️ 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 extensions,"``javax.*``, ``jakarta.*``",87,4185,90,10,4,2,1,1,4+ Java extensions,"``javax.*``, ``jakarta.*``",101,4185,90,10,4,2,1,1,4 - Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.com.caucho.hessian.io``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``com.caucho.burlap.io``, ``com.caucho.hessian.io``, ``com.cedarsoftware.util.io``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.esotericsoftware.yamlbeans``, ``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.exolab.castor.xml``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.ho.yaml``, ``org.influxdb``, ``org.jabsorb``, ``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,927,140,6,22,18,,208 + Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.com.caucho.hessian.io``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``com.caucho.burlap.io``, ``com.caucho.hessian.io``, ``com.cedarsoftware.util.io``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.esotericsoftware.yamlbeans``, ``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.fileupload``, ``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.exolab.castor.xml``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.ho.yaml``, ``org.influxdb``, ``org.jabsorb``, ``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``",144,10529,927,140,6,22,18,,208 - Totals,,330,26361,2656,404,16,128,33,1,409+ Totals,,355,26365,2656,404,16,128,33,1,409

• Changes to framework-coverage-java.csv:

- jakarta.servlet,2,19,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,19,,+ jakarta.servlet,2,26,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,26,,- javax.servlet,10,22,3,,,,,,,,,,,,,,1,,,,,,,,,,2,,,,,,,,,,3,,,2,,2,,,,,,,,,22,3,+ javax.servlet,10,29,3,,,,,,,,,,,,,,1,,,,,,,,,,2,,,,,,,,,,3,,,2,,2,,,,,,,,,29,3,+ org.apache.commons.fileupload,,11,4,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,11,4,

[smowton]

If you push the stub that isn't working for you I could probably shed some light.

And yes it does seem wrong that the Jakarta version of the models has become out of sync with the Javax version. Syncing the two and adding appropriate tests would be welcome.

[Kwstubbs]

@smowton I have found the solution to my problem. This is ready for review. 🍨

[smowton]

Thanks for this! To check: can most/all of https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll be removed as a consequence of this work?

[Kwstubbs]

@smowton I have modeled everything except parseRequest which I assume is already modeled since it returns a java.util.List<FileItem>. If someone with more Java experience could take a look to see if that would need to be modeled that would be great. I just added the value preserving steps summaryModel. I'm not sure if tests are required for those, or where they would go. If they are required, could you please direct me. I'm happy to remove or edit FileAndFormRemoteSource if you would like.

[smowton]

Suggest giving it a test anyway -- it should work; it doesn't matter if it's due to changes here or if existing models are sufficient.

To reiterate, can we move QL models from https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll as mentioned above?

[Kwstubbs]

Correct, the models from https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll should be modeled here so they are no longer needed. Can you specify which file I should add the tests for the summaryModels to?

[smowton]

Hi, sorry for the slow reponse-- how about adding the summary model tests to https://github.com/github/codeql/tree/main/java/ql/test/library-tests/frameworks similar to the existing test directories there for commons-lang and so on

[owen-mc]

java/ql/test/library-tests/frameworks/apache-commons-fileupload-1.4/test.ql is failing because the line numbers are wrong.

[Kwstubbs]

@smowton@owen-mc Hi I think everything is good on this PR. Ready for official review.

[owen-mc]

@Kwstubbs Sorry this slipped through the cracks. If you update it then I will review it.

[Copilot]

Pull Request Overview

This PR adds support for file upload sources to Remote Data Flow Sources by adding model definitions for javax.servlet.http.Part, jakarta.servlet.http.Part, org.apache.commons.fileupload.FileItem, and org.apache.commons.fileupload.FileItemStream.

• Adds comprehensive taint source models for Java file upload APIs
• Includes taint flow models for Apache Commons FileUpload utility classes
• Adds extensive test stubs for Jakarta Servlet API and Apache Commons FileUpload

Reviewed Changes

Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
java/ql/lib/ext/javax.servlet.http.model.yml	Adds remote source models for javax.servlet.http.Part methods
java/ql/lib/ext/jakarta.servlet.http.model.yml	Adds remote source models for jakarta.servlet.http.Part methods
java/ql/lib/ext/org.apache.commons.fileupload.model.yml	Adds remote source models for FileItem and FileItemStream
java/ql/lib/ext/org.apache.commons.fileupload.util.model.yml	Adds taint flow models for Streams utility methods
java/ql/test/library-tests/dataflow/taintsources/FileUpload.java	Adds test cases for file upload taint sources
java/ql/test/library-tests/frameworks/apache-commons-fileupload-1.4/Test.java	Adds test cases for Apache Commons FileUpload taint flows
Multiple stub files	Adds comprehensive test stubs for Jakarta Servlet API and Apache Commons FileUpload

[owen-mc]

 WARNING: A terminally deprecated method in sun.misc.Unsafe has been called WARNING: sun.misc.Unsafe::objectFieldOffset has been called by lombok.permit.Permit WARNING: Please consider reporting this to the maintainers of class lombok.permit.Permit WARNING: sun.misc.Unsafe::objectFieldOffset will be removed in a future release Exception in thread "main" Compilation errors were reported by javac. at com.semmle.extractor.java.JavaExtractor.main(JavaExtractor.java:999) 

[Kwstubbs]

@owen-mc Not sure what I can do here. Have any other of your Java PRs had this problem? If you could, could you rerun the tests just to make sure this is not a one time fluke? Thanks

[owen-mc]

There are two sources of test failures. I've made #20714 to fix one of them, which is unrelated to this PR. The rest are caused by an extractor crash relating to Lombok. It does seem to be related to the changes in this PR - it's when extracting the test file that you added that it crashes. I've asked if anyone knows what might be causing it.

[owen-mc]

I've fixed the language test problems. We can now see the integration test problems that were previously hidden 😆. They seem to have also failed on main recently, so perhaps they're flaky.

[owen-mc]

Please delete the appropriate parts of https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll . If you add a test for parseRequest and it works then then I think you can delete the whole file.

[owen-mc]

Actually, before merging I should do a QA run to check how many extra alerts we get from this. Before doing that, I'd really like it if we could confirm that the last remaining model from FileAndFormRemoteSource.qll is covered, and then delete that file. I tried to write a test for it and got stuck at generating stubs. Please can you do this when you have time?