Java: Delayed unsafe deserialization #8501
Conversation
| exists(FieldAccess access, Field field | field = access.getField() | | ||
| field.getType().hasName("byte[]") and | ||
| access = fromNode.asExpr() and | ||
| field.getAnAccess() = toNode.asExpr() |
There was a problem hiding this comment.
Probably not an issue, but should this cast the access to FieldRead to explicitly exclude FieldWrite?
| field.getAnAccess() = toNode.asExpr() | |
| field.getAnAccess().(FieldRead) = toNode.asExpr() |
There was a problem hiding this comment.
Maybe. I'm going to move this to a separate PR, let's discuss it there. Thanks for the suggestion!
| ObjectInputStream ois = new ObjectInputStream(bais); | ||
| return ois.readObject(); // $unsafeDeserialization | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe it would be good to also add a test where an assignment to a byte[] field transfers taint, e.g.:
(assuming that is the reason why you chose FieldAccess instead of the more specific FieldRead for your byteArrayFieldFlowStep predicate above)
...
class ObjectHolder {
private byte[] buffer;
public void init(byte[] tainted) throws Exception {
buffer = tainted;
}
public Object getObject() throws Exception {
ByteArrayInputStream bais = new ByteArrayInputStream(buffer);
ObjectInputStream ois = new ObjectInputStream(bais);
return ois.readObject(); // $unsafeDeserialization
}
}There was a problem hiding this comment.
Thanks for the idea!
|
I suggest splitting this PR into two parts: the uncontroversial addition of new sources, sinks and library-specific propagation steps, but then let's consider separately the difficult issue of when it's reasonable to treat a field write as a jump step (one that discards calling context, and supposes that probably at some point a read step takes place) |
|
@artem-smotrakov can you show us in which specific case you need the |
Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>
|
Thanks for the review and suggestions @Marcono1234 ! |
Hi @smowton Sure, let's split it. I'm going to add a model for JMS API as well. If you're not sure about |
|
hey @pwntester
I got this idea while analysing a fix for CVE-2016-6194 in RabbitMQ. Here is the details.
Untrusted data can be written to But actuall deserialization of untrusted data happens later when Honestly, it doesn't look framework-specific to me. Untrusted data flows via byte-arrays and standard Java I/O objects. That is why I thought we can cover this case in the query for unsafe deserialization. In CVE-2016-6194, untrusted data comes from an RabbitMQ message, therfore I think it makes sense to add a source for that. As I suggested above, we could probably create an experimental query for this kind of delayed deserialization instaead of updating the existing one. What do you think? |
Hi @smowton I've moved this part to #8765 . Also, I've added a model for JMS API as @pwntester suggested. |
@smowton I've moved this to a new experimental query so that this wouldn't affect the standard one.
@Marcono1234 I've applied your suggestions in the new pull request. Please have a look. Your commenta are as usual very useful, I am always happy to see them! |
|
Hi @pwntester
As an example, please have a look at the test from the new PR |
Deserialization can sometimes be implemented in two steps. An untrusted serialized object can be stored in a field but actual deserialization happens only when the object is necessary. CVE-2016-6194 in RabbitMQ is an example of such scenario (GitHub issue). Untrusted data that comes from a response is stored in
RMQObjectMessage.buffield. Then, deserialization happens whengetObject()method is called. Currently,java/unsafe-deserializationquery doesn't catch this.I'd like to propose the following updates to the query:
java/unsafe-deserializationquery because unsafe deserialization is a severe issue.DataInputandObjectInputthat read byte arrays.With these updates, the query detects CVE-2016-6194. Let me know what you think.