Skip to content

Conversation

artem-smotrakov
Copy link
Contributor

@artem-smotrakov artem-smotrakov commented Apr 17, 2022

This pull request was slpit from #8501 as suggested by @smowton.

In addition, I've added flow sources and steps for JMS API versions 1 and 2 as suggested by @pwntester. The model for the JMS API applis to all JMS implementations such as ActiveMQ and others. However, there may be implementation-specific flow sources and steps that are not covered here. I'd suggest to start with the API and then cover specific implementations if necessary.

Also, added a few flow steps for methods in DataInput and ObjectInput that read byte arrays.

This PR is related to github/securitylab#666

@artem-smotrakov artem-smotrakov requested a review from a team as a code owner April 17, 2022 11:37
@artem-smotrakov artem-smotrakov changed the title Add flow sources and steps for RabbitMQ and JMS Java: Add flow sources and steps for RabbitMQ and JMS Apr 17, 2022
@pwntester
Copy link
Contributor

thanks @artem-smotrakov would it make sense to also add deserialization sinks to ObjectMessage.getObject() API as per https://www.blackhat.com/docs/us-16/materials/us-16-Kaiser-Pwning-Your-Java-Messaging-With-Deserialization-Vulnerabilities.pdf

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the APIs in general to find additional methods that maybe ought to be modelled, but the changes here seem uncontroverisal

@artem-smotrakov
Copy link
Contributor Author

Hi @smowton Thanks for the comments! I've addressed them.

@artem-smotrakov
Copy link
Contributor Author

thanks @artem-smotrakov would it make sense to also add deserialization sinks to ObjectMessage.getObject() API as per https://www.blackhat.com/docs/us-16/materials/us-16-Kaiser-Pwning-Your-Java-Messaging-With-Deserialization-Vulnerabilities.pdf

@pwntester ObjectMessage.getObject() is an interface method. It depends on implementation if and how it does deserialization. By adding this sink, we'd assume that all implementations do some kind of unsafe deserialization here.

The author of the paper you mentioned says

All ObjectMessage implementations I looked at were deserializing from untrusted input without any validation

Maybe it is a safe assumption. Let me know if this assumption is okay. I don't mind adding this sink though.

@pwntester
Copy link
Contributor

Maybe it is a safe assumption. Let me know if this assumption is okay. I don't mind adding this sink though.

I think that after the talk some mitgations were put in place such as the ActiveMQ one so it may require a little testing

@artem-smotrakov
Copy link
Contributor Author

@smowton Unfortunately, I can't see why the checks failed. Could you please check what happened?

@atorralba
Copy link
Contributor

First error

File "ql/java/ql/test/stubs/rabbitmq-4.12.0/com/rabbitmq/client/DefaultConsumer.java" contains a non-ASCII character at the location marked with | in:

        
    }

    @Override
    public void handleDelivery|

File "ql/java/ql/test/stubs/rabbitmq-4.12.0/com/rabbitmq/client/Consumer.java" contains a non-ASCII character at the location marked with | in:

ption;;
  
public interface Consumer {
    void handleDelivery|

Second error

--- expected
+++ actual
@@ -1,1 +1,2 @@
-
+| TestIO.java:97:14:97:58 | readLine(...) | Fixed missing result:numTaintFlow=1 |
+| TestIO.java:139:14:139:64 | readUTF(...) | Fixed missing result:numTaintFlow=1 |
Error: [7/8] [114/134 comp 13s eval 6.2s] FAILED(RESULT) ql/java/ql/test/library-tests/frameworks/guava/handwritten/flow.ql

Third error

Error: The following CodeQL elements are lacking documentation:
semmle/code/java/frameworks/JMS.qll  JMS  file

@artem-smotrakov
Copy link
Contributor Author

artem-smotrakov commented Apr 25, 2022

Thanks for the info @atorralba ! I hope the errors are now fixed. Could you please rerun the checks?

@github-actions
Copy link
Contributor

⚠️ 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.*``,3,541,115,28,,,7,,,10
+    Java Standard Library,``java.*``,3,545,115,28,,,7,,,10
-    Java extensions,"``javax.*``, ``jakarta.*``",54,552,32,,,4,,1,1,2
+    Java extensions,"``javax.*``, ``jakarta.*``",63,609,32,,,4,,1,1,2
-    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",44,283,929,,,,14,18,,
+    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",65,290,929,,,,14,18,,
-    Totals,,183,6233,1441,106,6,10,107,33,1,81
+    Totals,,213,6301,1441,106,6,10,107,33,1,81
  • Changes to framework-coverage-java.csv:
+ com.rabbitmq.client,,21,7,,,,,,,,,,,,,,,,,,,,,,,,,,21,7,
- java.io,37,,31,,15,,,,,,,,,,,,,,,,,,22,,,,,,,31,
+ java.io,37,,35,,15,,,,,,,,,,,,,,,,,,22,,,,,,,35,
+ javax.jms,,9,57,,,,,,,,,,,,,,,,,,,,,,,,,,9,57,

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

smowton
smowton previously approved these changes Apr 26, 2022
@github-actions
Copy link
Contributor

⚠️ 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.*``,3,541,115,28,,,7,,,10
+    Java Standard Library,``java.*``,3,545,115,28,,,7,,,10
-    Java extensions,"``javax.*``, ``jakarta.*``",54,552,32,,,4,,1,1,2
+    Java extensions,"``javax.*``, ``jakarta.*``",63,609,32,,,4,,1,1,2
-    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",44,283,929,,,,14,18,,
+    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",65,290,929,,,,14,18,,
-    Totals,,183,6233,1441,106,6,10,107,33,1,81
+    Totals,,213,6301,1441,106,6,10,107,33,1,81
  • Changes to framework-coverage-java.csv:
+ com.rabbitmq.client,,21,7,,,,,,,,,,,,,,,,,,,,,,,,,,21,7,
- java.io,37,,31,,15,,,,,,,,,,,,,,,,,,22,,,,,,,31,
+ java.io,37,,35,,15,,,,,,,,,,,,,,,,,,22,,,,,,,35,
+ javax.jms,,9,57,,,,,,,,,,,,,,,,,,,,,,,,,,9,57,

smowton
smowton previously approved these changes Apr 26, 2022
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

@aschackmull is this ok with you with the import fixed as asked?

@github-actions
Copy link
Contributor

⚠️ 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.*``,3,541,115,28,,,7,,,10
+    Java Standard Library,``java.*``,3,545,115,28,,,7,,,10
-    Java extensions,"``javax.*``, ``jakarta.*``",54,552,32,,,4,,1,1,2
+    Java extensions,"``javax.*``, ``jakarta.*``",63,609,32,,,4,,1,1,2
-    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",44,283,929,,,,14,18,,
+    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",65,290,929,,,,14,18,,
-    Totals,,183,6233,1441,106,6,10,107,33,1,81
+    Totals,,213,6301,1441,106,6,10,107,33,1,81
  • Changes to framework-coverage-java.csv:
+ com.rabbitmq.client,,21,7,,,,,,,,,,,,,,,,,,,,,,,,,,21,7,
- java.io,37,,31,,15,,,,,,,,,,,,,,,,,,22,,,,,,,31,
+ java.io,37,,35,,15,,,,,,,,,,,,,,,,,,22,,,,,,,35,
+ javax.jms,,9,57,,,,,,,,,,,,,,,,,,,,,,,,,,9,57,

@aschackmull
Copy link
Contributor

@aschackmull is this ok with you with the import fixed as asked?

Change note possibly needs to be in lib rather than src. Otherwise yes, no further comments from me.

@github-actions
Copy link
Contributor

⚠️ 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.*``,3,541,115,28,,,7,,,10
+    Java Standard Library,``java.*``,3,545,115,28,,,7,,,10
-    Java extensions,"``javax.*``, ``jakarta.*``",54,552,32,,,4,,1,1,2
+    Java extensions,"``javax.*``, ``jakarta.*``",63,609,32,,,4,,1,1,2
-    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",44,283,929,,,,14,18,,
+    Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.rabbitmq.client``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.logging``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.log4j``, ``org.apache.logging.log4j``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jboss.logging``, ``org.jdbi.v3.core``, ``org.jooq``, ``org.mvel2``, ``org.scijava.log``, ``org.slf4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",65,290,929,,,,14,18,,
-    Totals,,183,6233,1441,106,6,10,107,33,1,81
+    Totals,,213,6301,1441,106,6,10,107,33,1,81
  • Changes to framework-coverage-java.csv:
+ com.rabbitmq.client,,21,7,,,,,,,,,,,,,,,,,,,,,,,,,,21,7,
- java.io,37,,31,,15,,,,,,,,,,,,,,,,,,22,,,,,,,31,
+ java.io,37,,35,,15,,,,,,,,,,,,,,,,,,22,,,,,,,35,
+ javax.jms,,9,57,,,,,,,,,,,,,,,,,,,,,,,,,,9,57,

@smowton smowton merged commit bb049bf into github:main Apr 27, 2022
@artem-smotrakov artem-smotrakov deleted the cover-jms branch April 27, 2022 20:46
@artem-smotrakov
Copy link
Contributor Author

Thanks for the review and addressing the comments @smowton !

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.

5 participants