Java: Detecting flow through throw - catch statements

[KylerKatz]

Hello,

I'm having a confusing issue trying to track the data flow from a throw to a catch statement. I had similar queries working just fine. However, I was messing around with my qlpack last week and I think I may have messed something up. To give you some context, I am using CLI version 2.20.3 and here is my

qlpack.yml

---
library: false
warnOnImplicitThis: false
name: custom-codeql-queries
version: 0.0.1
dependencies:
  # codeql/java-queries: ^0.8.9
  codeql/java-all: 6.1.0
libraries:
  - name: SensitiveInfo
    path: /SensitiveInfo
dataExtensions:
  - SensitiveInfo/*.yml

Along with my codeql-pack.lock.yml

---
lockVersion: 1.0.0
dependencies:
  codeql/dataflow:
    version: 1.1.9
  codeql/java-all:
    version: 6.1.0
  codeql/mad:
    version: 1.0.15
  codeql/rangeanalysis:
    version: 1.0.15
  codeql/regex:
    version: 1.0.15
  codeql/ssa:
    version: 1.0.15
  codeql/threat-models:
    version: 1.0.15
  codeql/tutorial:
    version: 1.0.15
  codeql/typeflow:
    version: 1.0.15
  codeql/typetracking:
    version: 1.0.15
  codeql/util:
    version: 2.0.2
  codeql/xml:
    version: 1.0.15
compiled: false

I don't fully understand all of the dependency versions needed to work with my CLI version so I would appriacte it if someones could point out if this looks wrong.

On to my specific issue.

I am trying to detect this toy example.

import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.ejb.EJBException;
import java.io.IOException;

public class BAD_AuthenticationFailureServlet extends HttpServlet {
    @Override
    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
        String username = request.getParameter("username");
        String password = request.getParameter("password");
        try {
            authenticateUser(username, password);
        } catch (EJBException e) {
            response.getWriter().write("Authentication failed: " + e.getMessage());
        }
    }

    private void authenticateUser(String username, String password) throws EJBException {
        if (username == null || password == null) {
            throw new EJBException("Username or password is null. Provided username: " + username + ", password: " + password);
        }
        throw new EJBException("Invalid credentials provided for user " + username + ". Attempted password: " + password);
    }
}

With this query

/**
 * @name CWE-550: Exposure of sensitive information through servlet responses
 * @description Detects flows of the `password` parameter into EJBException messages that are written back to HTTP clients.
 * @kind path-problem
 * @problem.severity warning
 * @id java/servlet-info-exposure/550
 * @tags security
 *       external/cwe/cwe-550
 *       external/cwe/cwe-200
 * @cwe CWE-550
 */

 import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.DataFlow
 
 // Define flow states
 private newtype MyFlowState =
   State1() or
   State2() or
   State3()
 
 module ServerGeneratedErrorMessageConfig implements DataFlow::StateConfigSig {
   class FlowState = MyFlowState;
 
     // Source: the `password` servlet parameter
   predicate isSource(DataFlow::Node source, FlowState state) {
     state instanceof State1 and
     exists(VarAccess va |
       va.getVariable().getName() = "password" and
       source.asExpr() = va
     )
   }
 
 
   // Sink: explicit HTTP response sinks that expose the message
   predicate isSink(DataFlow::Node sink, FlowState state) {
     state instanceof State3 and (
       // response.getWriter().write(msg)
       exists(MethodCall mc |
         mc.getMethod().getName() = "write" and
         exists(MethodCall getWriter |
           getWriter.getMethod().getName() = "getWriter" and
           getWriter = mc.getQualifier() and
           getWriter.getQualifier().getType().(RefType).hasQualifiedName("javax.servlet.http", "HttpServletResponse")
         ) and
         mc.getArgument(0) = sink.asExpr()
       )
     )
   }
 
   // Transitions between flow states
   predicate isAdditionalFlowStep(
     DataFlow::Node node1, FlowState state1,
     DataFlow::Node node2, FlowState state2
   ) {
     // State1->State2: flows into EJBException constructor
     state1 instanceof State1 and state2 instanceof State2 and
     exists(ConstructorCall cc |
       cc.getAnArgument() = node1.asExpr() and
       cc.getConstructor().getDeclaringType().(RefType)
         .getASupertype+().hasQualifiedName("javax.ejb", "EJBException") and
       node2.asExpr() = cc
     )
     or
     // State2->State3: flows from throw to catch capturing e or e.getMessage()
     state1 instanceof State2 and state2 instanceof State3 and
     exists(ThrowStmt t, CatchClause cc, Expr use |
       t.getExpr() = node1.asExpr() and
       cc.getEnclosingCallable() = t.getEnclosingCallable() and
       (
         use = cc.getVariable().getAnAccess()
         or
         exists(MethodCall mc2 |
           mc2.getQualifier() = cc.getVariable().getAnAccess() and
           mc2.getMethod().getName() = "getMessage" and
           use = mc2
         )
       ) and
       node2.asExpr() = use
     )
   }
 }
 
 module SensitiveInfoInErrorMsgFlow =
   TaintTracking::GlobalWithState<ServerGeneratedErrorMessageConfig>;
 import SensitiveInfoInErrorMsgFlow::PathGraph
 
 from SensitiveInfoInErrorMsgFlow::PathNode source, SensitiveInfoInErrorMsgFlow::PathNode sink
 where SensitiveInfoInErrorMsgFlow::flowPath(source, sink)
 select sink, source, sink,
   "CWE-550: password parameter is exposed via server error message."

I have confirmed that the isSource and isSink do work. So the issue is likely in the flow between them. I have used a few queries in the past that follow this similar structure. However, they are also missing some cases randomly at the moment. Which is leading me to believe it has something to do with my qlpack.

As always, thank you for the help.

[redsun82]

👋 @KylerKatz It doesn't seem to me there's anything wrong with your pack definition. Can you elaborate on "However, they are also missing some cases randomly at the moment"? Did you change the version of the codeql/java-all dependency? Do you know if the versions listed in the lock file changed in the meantime?

[hvitved]

CC: @aschackmull, who has been working on adding general flow through exceptions for Java.