[Java] Clarification Needed on DataFlow::FeatureHasSourceCallContext Behavior

[Hug0Vincent]

Hi,
I'm encountering unexpected behavior that I thought would be resolved by using DataFlow::FeatureHasSourceCallContext, but it persists. I'd appreciate some clarification on how the three different Feature settings interact.

To illustrate, I've reproduced the issue in Java: I'm tracking a taint parameter from my source method. However, when the taint flows into randomMethod, it incorrectly appears to continue after the return statement in otherMethod even though source never calls otherMethod. Why is this path being shown as possible?

public void source(String taint) {
      return randomMethod(taint)
}

public Object randomMethod(String taint) {
      return taint
}

public void otherMethod(String param) {
      Object obj = randomMethod(taint)
      // continue the tracking / flow
} 

I initially thought that DataFlow::FeatureHasSourceCallContext would solve this issue so I'm confused. There is the DataFlow::FeatureEqualSourceSinkCallContext value but I don't understand it's purpose, could you provide a simple code snippet to explain both FeatureHasSinkCallContext and FeatureEqualSourceSinkCallContext ?

I feel I'm not the only one struggling with this issue: #17241 I'm surprised by this kind of result.

I would like a way to ensure that my path goes from source to sink and that it can be called in my program.

Thank you

[hvitved]

Hi 👋

Can you share the query that you used to track flow? I'm surprised that you see flow going from source into randomMethod and then out to otherMethod; that certainly shouldn't happen.

As for flow features, they are used to restrict which kind of flow is allowed at sources/sinks:

• FeatureHasSourceCallContext: This means that a source is not allowed to flow out of the method containing the source.
• FeatureHasSinkCallContext: This means that a sink is not allowed to have flow in to the method containing the sink.
• FeatureEqualSourceSinkCallContext: This means, basically, that the source and sink must be in the same method.

Flow features are rarely needed in practice, and if the underlying problem is the erroneous flow mentioned above, then it sounds more like a bug in the data flow library.

[Hug0Vincent]

Hi, yes so here it is:

• the main query
• the main config
• some additional steps

When is FeatureHasSinkCallContext useful ?

[Hug0Vincent]

@hvitved I have extracted the relevant code from my codebase and created a query to show you.

So here is my Java code, I have basically copy/paste the weird function which is toUpperEnglish. I want to find flow from parameter of sourceMethod to arguments of sinkMethod. From my understanding their should be only one path in my example. I've omitted FeatureHasSourceCallContext here because it does not change anything but I'm using it to prevent flow to get out of my source method.

package com.test;

import java.util.Locale;

public class StringUtils {

   private static final int TO_UPPER_CACHE_LENGTH = 2048;
   private static final int TO_UPPER_CACHE_MAX_ENTRY_LENGTH = 64;
   private static final String[][] TO_UPPER_CACHE = new String[TO_UPPER_CACHE_LENGTH][];

   private StringUtils() {
   }

   public static String toUpperEnglish(String var0) {
      if (var0 == null) return null;
      if (var0.length() > TO_UPPER_CACHE_MAX_ENTRY_LENGTH) {
         return var0.toUpperCase(Locale.ENGLISH);
      } else {
         int var1 = var0.hashCode() & (TO_UPPER_CACHE_LENGTH - 1);
         String[] var2 = TO_UPPER_CACHE[var1];
         if (var2 != null && var2[0].equals(var0)) {
            return var2[1];
         } else {
            String var3 = var0.toUpperCase(Locale.ENGLISH);
            var2 = new String[]{var0, var3};
            TO_UPPER_CACHE[var1] = var2;
            return var3;
         }
      }
   }
}

class Sourceclass {
   public Object sourceMethod(Object source) {
      return (String) Sinkclass.sinkMethod(StringUtils.toUpperEnglish((String) source));
   }
}

class Other {
   private static String randomMethod(Object var1) {
      return (String) Sinkclass.sinkMethod(StringUtils.toUpperEnglish((String) var1));
   }
}

class Sinkclass {
   public static Object sinkMethod(Object source) {
      return source;
   }
}

Note the randomMethod that is also calling toUpperEnglish but sourceMethod never call it.

Here is my CodeQL query:

/**
 * @id test
 * @description test
 * @name test
 * @kind path-problem
 * @problem.severity warning
 * @tags security
 */

import java
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.security.CommandLineQuery

class SourceMethod extends Method {
  SourceMethod() { this.getName() = "sourceMethod" }
}

module MyConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    source.asParameter().getCallable() instanceof SourceMethod
  }

  predicate isSink(DataFlow::Node sink) {
    exists(Call c |
        c.getCallee().hasQualifiedName("com.test", "Sinkclass", "sinkMethod") and
        sink.asExpr() = c.getAnArgument()
    )
  }
}

module MyFlow = TaintTracking::Global<MyConfig>;

import MyFlow::PathGraph

from MyFlow::PathNode source, MyFlow::PathNode sink
where MyFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "sink: $@", source.getNode(), sink.toStringWithContext()

For this I get 2 paths, the one I'm looking for:

but also this one:

[aschackmull]

That looks completely correct to me. If flow reaches a static variable then surely that same static variable can be read from other unrelated calls to toUpperEnglish - storing into and reading from a static variable effectively throws away the call context. So even though this may be considered a false positive, it's working as intended. Data flow cannot distinguish between individual array indices here, so as soon as one entry in the cache is tainted, then they're all considered tainted.

[Hug0Vincent]

Alright so Is there a way to keep the call context from source to sink to prevent those "false positive" ?

[aschackmull]

If we're analysing the code as-is then I'd argue that we do in fact want this path: The call from sourceMethod taints the cache, which ought to be reflected as taint reaching other call sites.
An alternative approach is to supply a model giving a high-level description of the taint flow in toUpperEnglish, which can then supersede the source code analysis. You'd want a yml file with something like

extensions:
  - addsTo:
      pack: codeql/java-all
      extensible: summaryModel
    data:
      - ["com.test", "StringUtils", True, "toUpperEnglish", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]

[Hug0Vincent]

The problem is that I'm running it on multiple codebase and I don't know in advance which function will need such summaryModel

[aschackmull]

I guess alternatively you can introduce a barrier for the FieldValueNode. That should block the call-context-dropping flow path.

  predicate isBarrier(DataFlow::Node node) {
    node.(DataFlow::FieldValueNode).getField() instanceof MyStaticCacheFieldThatShouldntPropagateTaint
  }