Determining if an error message is sensitive?

[KylerKatz]

trafficstars

Hello, I am currently trying to create a query that will allow me to determine whether a servlet message contains sensitive information.

import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.frameworks.Servlets
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.dataflow.DataFlow
import CommonSinks.CommonSinks
import SensitiveInfo.SensitiveInfo

module Flow = TaintTracking::Global<SensitiveInfoLeakServletConfig>;

import Flow::PathGraph

module SensitiveInfoLeakServletConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    exists(SensitiveVariableExpr sve | source.asExpr() = sve) // Assume this has a list of sensitive variables in a file
    or     exists(CatchClause cc, MethodCall mc | mc.getQualifier() = cc.getVariable().getAnAccess() and source.asExpr() = mc)

  }

  predicate isSink(DataFlow::Node sink) {
    // Consider the case where the sink exposes sensitive info within a catch clause of type ServletException
    exists(CatchClause cc, MethodCall mc |
      // Ensure the CatchClause is catching ServletException
      cc.getACaughtType().hasQualifiedName("javax.servlet", "ServletException") and
      // Ensure the MethodCall is within the CatchClause for the ServletException
      mc.getEnclosingStmt().getEnclosingStmt*() = cc.getBlock() and
      // Ensure the sink matches one of the known sensitive sinks
      (
        getSinkAny(sink)
      ) and
      // Link the sink to the argument of the MethodCall
      sink.asExpr() = mc.getAnArgument()
    )
  }
}

from Flow::PathNode source, Flow::PathNode sink
where Flow::flowPath(source, sink)
select sink.getNode(), source, sink,
  "Servlet Runtime Error Message Containing Sensitive Information."

I almost have this working, however, the issue that I am having has to do with false positives.

Say I have this code

throw new ServletException("Invalid database connection parameters" + dbUrl + dbUser + dbPass);

 } catch (ServletException e) {
  // Catch the ServletException and send an error response
   response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "An error occurred while processing the request: " + e.getMessage());
}

In this example, there is sensitive information (dbUrl + dbUser + dbPass) in the e.getMessage() so this would be valid.

However, say we change it to

throw new ServletException("Invalid database connection parameters");

 } catch (ServletException e) {
  // Catch the ServletException and send an error response
   response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "An error occurred while processing the request: " + e.getMessage());
}

Now we no longer have sensitive information exposed. However, I am still detecting this. I believe this issue is in this line.

    exists(CatchClause cc, MethodCall mc | mc.getQualifier() = cc.getVariable().getAnAccess() and source.asExpr() = mc)

Without it, I don't detect any usages of e.getMessage(), but with it, I detect all of them. When I only want to detect them with sensitive information in them. I believe this should be moved to the isSink check, however, I am not sure how to integrate it.

I appreciate any help, thank you.

[jketema]

Hi @KylerKatz,

You're assessment is correct that

exists(CatchClause cc, MethodCall mc | mc.getQualifier() = cc.getVariable().getAnAccess() and source.asExpr() = mc)

should not occur in the source. This is because it effectively singles out every method call on the variable defined by the catch clause as a source, which is clearly too broad.

The usual strategy here would be to use something like a flow state. See here for query that uses one. In you case you would want two states, and initial state that tracks a sensitive variable until you reach a throw, and a second state that you switch to once you observe the throw. You switch by defining an appropriate isAdditionalFlowStep predicate. The sink then checks that you're in this second state.

Unfortunately, the above does not work in your case, because the dataflow library does not support flow from throws to catch clauses. If the throw and catch are in the same method, you can probably work around this by adding another clause to isAdditionalFlowStep that matches up throws with the appropriate catch clauses. Extending that to anything that is inter-procedural is likely going to be a significant engineering effort. We have been looking at extending the dataflow library with the ability to reason over exception handling, but thus far it has been difficult to come up with a solution that has good performance.

[KylerKatz]

Hello @jketema,

Thank you for being so helpful. I appreciate it.

[KylerKatz]

I tried to at least implement the isAdditionalFlowStep predicate that you had mentioned to be able to cover simple cases. However, I am not able to detect even simple cases.

Here is an example code snippet.

public class BAD_AccessControlError {
    public void validateUserAccess(String userName, int accessLevel) {
        try {
            if (accessLevel < 1 || accessLevel > 5) {
                throw new IllegalArgumentException("Access level " + accessLevel + " is out of valid range for " + userName);
            }
            // Access validation logic
        } catch (IllegalArgumentException e) {
            System.err.println(e.getMessage());
        }
    }

    public static void main(String[] args) {
        new BAD_AccessControlError().validateUserAccess("adminUser", 0);
    }
}

Here userName and accessLevel are the sources of sensitive information. I'd like to find the flow from the source that goes into the throw message to e.getMessage() and eventually to System.err.println.

This is my query for this example

/**
 * @name Generation of Error Message Containing Sensitive Information
 * @description Identifies instances where sensitive information such as file paths, usernames, or passwords might be included in error messages, potentially leading to information exposure.
 * @kind path-problem
 * @problem.severity warning
 * @id java/error-message-sensitive-info
 * @tags security
 */

 import java
 import semmle.code.java.dataflow.TaintTracking
 import CommonSinks.CommonSinks
 import SensitiveInfo.SensitiveInfo
 
 module Flow = TaintTracking::Global<SensitiveInfoInErrorMsgConfig>;
 import Flow::PathGraph
 
 /** A configuration for tracking sensitive information flow into error messages. */
 module SensitiveInfoInErrorMsgConfig implements DataFlow::ConfigSig {
    
    // Initial state: Track sensitive variables by name
    predicate isSource(DataFlow::Node source) {
      exists(Expr expr |
        (
          expr.toString() = "userName" or // Track variable named "userName"
          expr.toString() = "accessLevel" // Track variable named "accessLevel"
        ) and
        source.asExpr() = expr
      )
    }
 
    // Transition state: Track exceptions created by sensitive information
    predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
      // Flow from sensitive variables to an exception constructor (in a throw)
      exists(MethodCall mc |
        mc.getMethod().getDeclaringType().getASupertype*().hasQualifiedName("java.lang", "Throwable") and
        mc.getAnArgument() = node1.asExpr() and
        node2.asExpr() = mc.getQualifier()
      ) 
    }
 
    // Final state: Track flow of exception message to sink
    predicate isSink(DataFlow::Node sink) {
      exists(MethodCall mc |
        mc.getMethod().getName() = "println" and
        mc.getQualifier().(FieldAccess).getField().getDeclaringType().hasQualifiedName("java.lang", "System") and
        mc.getQualifier().(FieldAccess).getField().getName() = "err" and
        sink.asExpr() = mc.getAnArgument()
      )
    }
 }
 
 from Flow::PathNode source, Flow::PathNode sink
 where Flow::flowPath(source, sink)
 select sink.getNode(), source, sink, "CWE-209: Error message may contain sensitive information."

I have confirmed that both the source and sink work as expected by having an example where the source flows directly to the sink by just printing it directly. So, I believe it's an issue with my isAdditionalFlowStep predicate. This is my first time using this predicate so I am likely doing something wrong that I am missing.

Thank you for any help.

[rvermeulen]

Hi @KylerKatz,

Not sure if this is still relevant, but your additional step is empty and the following is a possible option to connect the thrown exception object with it uses in a catch clause.

  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
    // Propagate flow from sensitive information used in contructing the exception object
    // to the exception object in the catch clause.
    exists(ThrowStmt throw, CatchClause catch |
      throw.getExpr().(NewClassExpr).getAnArgument() = node1.asExpr() and
      catch.getTry() = throw.getParent+() and
      catch.getACaughtType() = throw.getExpr().getType()
    |
      node2.asExpr() = catch.getVariable().getAnAccess()
    )
    or
    // When the exception variable is tainted, so should any method call it qualifies.
    exists(MethodCall mc, CatchClause cc | mc.getQualifier().getType() = cc.getACaughtType() and
      mc.getEnclosingStmt().getEnclosingStmt*() = cc|
      node1.asExpr() = mc.getQualifier() and
      node2.asExpr() = mc
    )
  }

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity. Comment or remove the Stale label in order to avoid having this issue closed in 7 days.

[github-actions[bot]]

This issue was closed because it has been inactive for 7 days.