Codeql To detect log injection of sensitive data doesn't catch implicit toString() call

[yuval-piiano]

Hi,

Bases on this query https://codeql.github.com/codeql-query-help/java/java-sensitive-log/ I have written a query that considers
our sensitive data.

My problem is that an implicit call to toString() , which leaks secret data is not captured by this query, eg. :
this query is marked (correctly, as toString leakse sensitive data) -

logger.info(" Whole container, This one leaks data :"+customer.toString());
while this one

logger.info(" Whole container, This one leaks data too :"+cu);

which is the exact same isn't caught...
What should I do to make my query catch the implicit toString() call too?

Thanks,
Yuval

[atorralba]

Hi @yuval-piiano. How have you defined the sources, sinks and additional steps in the dataflow/taint tracking configuration of your query?

[atorralba]

It looks correct. In your false negative example, is cu a field read of secretAccessKey? And is secretAccessKey a String?

logger.info(" Whole container, This one leaks data too :"+cu);

[atorralba]

Or is it that the class of the cu object has an overridden toString implementation where the field read happens and is returned as part of the result string?

[yuval-piiano]

cu is inited just before the log statement,

public static void testExplicit() {
        Customer cu = new Customer();
        cu.setSecretAccessKey("**");
        logger.debug(" Whole container, This one leaks a pii :"+cu.toString());
    }



public static void testImplicit() {
        Customer cu = new Customer();
        cu.setSecretAccessKey("**");
        logger.debug(" Whole container, This one leaks a pii :"+cu);
    }

This is my class:

public class Customer  {
    private static final long serialVersionUID = 1L;

     private Long id;
     private String secretAccessKey;

    public Customer() {
    }

    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }

    public String getSecretAccessKey() {
        return secretAccessKey;
    }

    public void setSecretAccessKey(String secretAccessKey) {
        this.secretAccessKey = secretAccessKey;
    }


    public String toString(){
        
        return this.getSecretAccessKey();
    }

}

[atorralba]

Thanks, this makes it clear.

What you're seeing is consistent: the source is found inside the toString method, which means that there are only dataflow edges from the return value of toString to an explicit call that dispatches to that method. Since your second example doesn't include the call explicitly, such an edge doesn't exist.

To solve this problem, you can create the edge yourself by adding an additional flow step from the return value of toString to accesses of a Customer object that happen in an implicit toString context.

For example:

import semmle.code.java.StringFormat
import semmle.code.java.dataflow.FlowSteps

class ImplicitToStringStep extends AdditionalValueStep {
  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
    exists(ReturnStmt r |
      r.getResult() = n1.asExpr() and
      r.getEnclosingCallable().(ToStringMethod).getDeclaringType() = n2.getType() and
      implicitToStringCall(n2.asExpr())
    )
  }
}

(Note that this is a simplistic approach that doesn't take type hierarchy into account)

If you add this to the context of your query, you should get an alert in the testImplicit method.