Support Java wait/notify pattern

[pwntester]

In some cases, CodeQL may not be able to follow a data flow where tainted data is stored in a class field on a synchronized block which then calls notify or notifyAll to awake a thread which in turn end up using the tainted field into a sink.

A good example is the code injection vulnerability described in this SonarSource Blog post.

I was able to get the issue reported by adding a new TaintStep which models the wait/notify pattern by connecting fields writes on a synchronized version calling notify with the same field reads on a synchronized version calling wait:

class NotifyWaitTaintStep extends TaintTracking::AdditionalTaintStep {
  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
    exists(MethodAccess notify, RefType t, MethodAccess wait, SynchronizedStmt notifySync, SynchronizedStmt waitSync |

      notify.getMethod().getName() = ["notify", "notifyAll"] and
      notify.getAnEnclosingStmt() = notifySync and
      notifySync.getExpr().getType() = t and

      wait.getMethod().getName() = "wait" and
      wait.getAnEnclosingStmt() = waitSync and
      waitSync.getExpr().getType() = t and

      exists(AssignExpr write, FieldAccess read, Field f |
        write.getAnEnclosingStmt() = notifySync and
        write.getDest().(FieldAccess).getField() = f and
        write = n1.asExpr() and

        read.getAnEnclosingStmt() = waitSync and
        read.getField() = f and
        read = n2.asExpr()
      )
    )
  }
}

The taint step may need improvements though, but I think is a good starting point.

[aschackmull]

I think we'll need more real code examples of TP and FP versions of the above flow step to allow us to start thinking about how it may be improved. As is, there's no indication that the qualifiers of the field read and write can refer to the same object, and that sounds like it may lead to lots of FP flow.
Also, this only accounts for SynchronizedStmts - we'd want to consider synchronized methods as well, I guess.

[pwntester]

👍 will dump more examples here if/when I find them

Also, this only accounts for SynchronizedStmts - we'd want to consider synchronized methods as well, I guess.

Totally, those should be accounted for as well

[smowton]

A restricted version of this that I suspect might be quite accurate would restrict the waiting sync-block and the notifying sync-block to occur in methods declared on the same type. If there are side-by-side methods

void writer() {
  synchronized(something) {
    field = x;
    something.notify();
  }
}

T read() {
  synchronized(something) {
    something.wait();
    return field; 
  }
}

If the synchronization types match and the fields do too... seems like pretty good odds information is flowing.

In any case it's easy to query LGTM for all the flow edges we would introduce this way and evaluate them. Action @pwntester? :)

[pwntester]

@smowton SGTM, I think it would make sense to test that version of the predicate on LGTM and see how accurate/noisy it is.

[smowton]

I've started an all-projects test identifying reader/writer pairs seen by your current draft, will send the run to you privately to check for accuracy