Some time ago, I read a blog post about the review that SonarSource security researchers performed of the Emissary application, which is maintained by the National Security Agency (NSA). According to the NSA, Emissary is a “P2P based data-driven workflow engine that runs in a heterogeneous possibly widely dispersed, multi-tiered P2P network of compute resources.” I read that as “this Java code probably handles a lot of attacker controlled input,” so naturally my interest was piqued.

The SonarSource blog post describes some of the vulnerabilities that researchers uncovered in Emissary, including:

Having worked on a CodeQL query to detect similar code injection patterns, I wanted to check if the query could find these issues automatically. In this blog post, I describe how CodeQL detects some of the above-mentioned CVEs using its default rule set, how we were able to find an entirely new set of additional critical issues, and how the NSA leveraged GitHub code scanning and security advisories to ultimately address the issues.

CodeQL findings

Here’s a quick summary of my CodeQL findings, all of which I cover in detail below.

By running the standard set of CodeQL queries on the Emissary project, I found the previously reported arbitrary file disclosure (CVE-2021-32093) but also uncovered new issues:

The original code injection CVE (CVE-2021-32096) was flagged by a community-contributed CodeQL query.

As of today, the reflected cross-site scripting vulnerability (CVE-2021-32092) is also found by a default CodeQL query.

Code injection (CVE-2021-32096)

Initially, when I tried the CodeQL script injection query on the Emissary 5.9.0 codebase I got no results.

After reading the source code for the vulnerability details, I was sure that my query was correctly modeling the javax.script.ScriptEngine.eval() sink and the source was already modelled by the default CodeQL JAX-RS libraries. However, I realized that the flow from the untrusted data to the script injection sink was not a “direct” one. You can take a look at how the code flows to understand why.

The JAX-RS endpoint where user data enters the application is:

web entry point

The getOrCreateConsole(request) will call RubyConsole.getConsole() which takes us to:

getConsole

This code starts a new thread running the RubyConsole.run() method (since it implements the Java Runnable interface):

run

However, since at this point, stringToEval is null, this method will almost immediately suspend the thread with the wait() method.

Later on, in the rubyConsolePost, we find the following code:

eval command

Here is where the untrusted data (request.getParameter(CONSOLE_COMMAND_STRING)) enters the application and flows into the RubyConsole.evalAndWait() method. However, the evalAndWait() method is:

evalAndWait

There are no actual calls to the RubyConsole.eval() method where the Ruby script is evaluated, so if you trace the tainted request parameter, you will end up in this method and reach the end of your taint trace. The user controlled command just gets assigned to the stringToEval field, and that’s seemingly the end of the road. However, if you take a closer look, you will also see that this method is calling the notifyAll() method, which means that this method will effectively wake up the sleeping thread which will in turn run the following expression:

result = this.eval(stringToEval);`

To summarize:

Therefore, there is no direct (source to sink) data flow that a static code analysis tool can effectively follow. The good news is that by modelling the Java wait/notify pattern with a CodeQL taint step, I should be able to get this issue reported.

In this code pattern, you can see two different types of blocks: synchronized blocks that call notify and synchronized blocks that call wait. When the synchronization occurs on the same object, I want to connect writes in the notify block with reads of the same fields on the wait block. That means I need an additional taint step to connect these otherwise disconnected nodes so that CodeQL’s taint tracking can bridge this logical disconnect:

class NotifyWaitTaintStep extends TaintTracking::AdditionalTaintStep {
  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
    exists(MethodAccess notify, MethodAccess wait, SynchronizedStmt notifySync, SynchronizedStmt waitSync |
      notify.getMethod().hasQualifiedName("java.lang", "Object", ["notify", "notifyAll"]) and
      notify.getAnEnclosingStmt() = notifySync and
      wait.getMethod().hasQualifiedName("java.lang", "Object", "wait") and
      wait.getAnEnclosingStmt() = waitSync and
      waitSync.getExpr().getType() = notifySync.getExpr().getType() and
      exists(AssignExpr write, FieldAccess read |
        write.getAnEnclosingStmt() = notifySync and
        write = n1.asExpr() and
        read.getAnEnclosingStmt() = waitSync and
        read.getField() = write.getDest().(FieldAccess).getField() and
        read = n2.asExpr()
      )
    )
  }
}

With this additional taint step enabled, I was able to get this issue successfully reported:

code injection

What’s awesome is that this query wasn’t developed by GitHub CodeQL engineers but contributed and improved by several CodeQL community members. A big shoutout to @SpaceWhite, @p0wn4j, and @lucha-bc:

This community-contributed query is on its way to the standard query set and will soon be available to all open source projects running GitHub code scanning.

I also contributed my notify/wait pattern taint step to the CodeQL repository, which may soon enable similar dataflow analysis between synchronized fields for all CodeQL users!

Arbitrary file disclosure (CVE-2021-32093)

CodeQL found the arbitrary file disclosure with the default configuration, and therefore I won’t comment on the details of this vulnerability since it was already described in the SonarSource blog post.

Unsafe deserialization (CVE-2021-32634)

CodeQL default queries also reported three unsafe deserialization operations.

The first one was located in the WorkSpaceClientEnqueueAction REST endpoint:

  @POST
  @Path("/WorkSpaceClientEnqueue.action")
  @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
  @Produces(MediaType.TEXT_PLAIN)
  public Response workspaceClientEnqueue(@FormParam(WorkSpaceAdapter.CLIENT_NAME) String clientName,
      @FormParam(WorkSpaceAdapter.WORK_BUNDLE_OBJ) String workBundleString) {
    logger.debug("TPWorker incoming execute! check prio={}", Thread.currentThread().getPriority());
    // TODO Doesn't look like anything is actually calling this, should we remove this?
    final boolean success;
    try {
      // Look up the place reference
      final String nsName = KeyManipulator.getServiceLocation(clientName);
      final IPickUpSpace place = (IPickUpSpace) Namespace.lookup(nsName);
      if (place == null) {
        throw new IllegalArgumentException("No client place found using name " + clientName);
      }
      final ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(workBundleString.getBytes("8859_1")));
      WorkBundle paths = (WorkBundle) ois.readObject();
      success = place.enque(paths);
    }
    ...
  }

This endpoint can be reached via an authenticated POST request to /WorkSpaceClientEnqueue.action. As you can read in the source code, the form parameter WorkSpaceAdapterWORK_BUNDLE_OBJ (tpObj) gets decoded and deserialized in line 52.

final ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(workBundleString.getBytes("8859_1")));

Fortunately, this is a post-authentication issue, and since the SonarSource report got the cross-site request forgery (CSRF) vulnerability fixed, this vulnerability could not be exploited on behalf of a logged-in user through CSRF.

CodeQL also reported two other unsafe deserialization operations which are not currently exercised in the code. However, they are ticking bombs which could be enabled in future releases and therefore the Security Lab team also reported them.

The first one originates from the MoveToAction class which was not exposed by the Jersey server. As described in a comment “// TODO This is an initial crack at the new endpoint, I haven’t seen it called an am unsure when/if it does”

MoveToAction:

public Response moveTo(@Context HttpServletRequest request)
 final MoveToAdapter mt = new MoveToAdapter();
 final boolean status = mt.inboundMoveTo(request);
 ...

MoveToAdapter:

public boolean inboundMoveTo(final HttpServletRequest req)
 final MoveToRequestBean bean = new MoveToRequestBean(req);
   MoveToRequestBean(final HttpServletRequest req)
   final String agentData = RequestUtil.getParameter(req, AGENT_SERIAL);
   setPayload(agentData);
   this.payload = PayloadUtil.deserialize(s);
  ...

PayloadUtil:

ois = new ObjectInputStream(new ByteArrayInputStream(s.getBytes("8859_1")));

The second one originates from the inboundEnque method of the WorkSpaceAdapter class. The vulnerability requires a call to inboundEnque() which is currently not exercised.

WorkspaceAdapter:

  /**
   * Process the enque coming remotely over HTTP request params onto the specified (local) pickup client place
   */
  public boolean inboundEnque(final HttpServletRequest req) throws NamespaceException {
    logger.debug("TPA incoming elements! check prio={}", Thread.currentThread().getPriority());
    // Parse parameters
    final EnqueRequestBean bean = new EnqueRequestBean(req);
    // Look up the place reference
    final String nsName = KeyManipulator.getServiceLocation(bean.getPlace());
    final IPickUpSpace place = lookupPlace(nsName);
    if (place == null) {
      throw new IllegalArgumentException("No client place found using name " + bean.getPlace());
    }
    return place.enque(bean.getPaths());
  }

WorkspaceAdapter:

    EnqueRequestBean(final HttpServletRequest req) {
      setPlace(RequestUtil.getParameter