Conversation
|
I am not asking for a full review necessarily, this is machine-generated and I have looked at the rows myself individually. Obviously my CodeQL knowledge is still limited so I may very well have made mistakes. I am open for discussing how to approach getting this reviewed/merged. |
owen-mc
left a comment
There was a problem hiding this comment.
I think the output format needs to be updated to correctly deal with nested classes.
| pack: codeql/java-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["org.apache.avro.data", "ObjectReader", True, "read", "(Object,Decoder)", "", "Argument[1]", "unsafe-deserialization", "ai-generated"] |
There was a problem hiding this comment.
I tried to look up the docs for this and found that it's actually a nested class. I believe we would need the below syntax to correctly specify it. (You can see this is used for java.io.ObjectInputFilter.Config, for example.)
| - ["org.apache.avro.data", "ObjectReader", True, "read", "(Object,Decoder)", "", "Argument[1]", "unsafe-deserialization", "ai-generated"] | |
| - ["org.apache.avro.data", "Json$ObjectReader", True, "read", "(Object,Decoder)", "", "Argument[1]", "unsafe-deserialization", "ai-generated"] |
| pack: codeql/java-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["org.apache.avro.data", "ObjectReader", True, "read", "(Object,Decoder)", "", "Argument[1]", "unsafe-deserialization", "ai-generated"] |
There was a problem hiding this comment.
Also, Gemini 3.1 Pro doesn't think this should be a sink.
Reasoning: Unsafe deserialization vulnerabilities occur when an application deserializes untrusted data into arbitrary Java objects (allowing an attacker to trigger malicious gadget chains). However, Json.ObjectReader is designed to strictly read Avro-encoded data matching the specific Json.SCHEMA internal to Apache Avro.
If you examine its implementation, it maps incoming primitive tokens directly to basic, safe Jackson JsonNode types (like LongNode, DoubleNode, TextNode, ArrayNode, and ObjectNode) and then unwraps them into basic Java structures (Map, List, String, Long, etc.). Since it does not perform polymorphic deserialization or resolve arbitrary class names from the data stream, it is structurally immune to unsafe class instantiation and does not act as a deserialization sink.
5d78705 to
ec10873
Compare
|
In the past, when we have generated too many models to manually check, we have validated them by running QA and checking the alert changes are reasonable (not too many, not too many FPs). It turns out that if you have 100 incorrect models, most of them won't cause any problems, because they just don't lead to any extra data flow paths, but there are some which do cause a lot of extra data flow paths, which will cause lots of FPs, which are pretty obvious when you look at the QA results. This PR doesn't introduce that many models, so it might be worth reviewing them manually. Also, the above procedure won't work for sources that below to non-default threat models. It might well be possible to run QA with a particular threat model. I haven't done this before so I've asked around. If so we should definitely make sure to add |
Add Models as Data for the Java version of Apache Avro. This is based on this subfolder/commit.
This is entirely LLM-generated and the output has undergone a voting procedure. It is not meant to fully cover the library. I am curious for any feedback on this. We also need to decide if the provenance is OK or if there is a better name.
This PR contains #21751 for the
qlpack.ymlwildcard, needed for a DCA run.