cant find taint flow in a LocalVariable statement

[HeouDonkey]

hi, i try to perform a taint analysis with following statment:

   protected void handleSimReady(int phoneId) {
          ··········
          String iccId = (uiccSlot != null) ? IccUtils.stripTrailingFs(uiccSlot.getIccId()) : null;
          ············

but it cant find any flow between the return of stripTrailingFs() and iccId.my query as follow:

predicate toIccUtilsReturn(DataFlow::Node sink) {
  exists(ReturnStmt return,VarAccess vac|
      return.getCompilationUnit().toString()="IccUtils"
      and
      return.getEnclosingCallable().toString()="stripTrailingFs"
      and
      sink.asExpr()=return.getResult()
  )
predicate toSubscriptionInfoUpdater(DataFlow::Node sink) {
  exists(LocalVariableDeclExpr iccid |
      iccid.getCompilationUnit().toString()="SubscriptionInfoUpdater"
      and
      iccid.getName()="iccId"
      and
      sink.asExpr()=iccid
  )
}
}
module SensitiveLoggerConfig implements DataFlow::ConfigSig {  // 1: module always implements DataFlow::ConfigSig or DataFlow::StateConfigSig
    predicate isSource(DataFlow::Node source) {     
      fromIccRecords(source)
    } // 3: no need to specify 'override'
    predicate isSink(DataFlow::Node sink) {     
      toSubscriptionManager(sink)
    }
    int fieldFlowBranchLimit() { result = 500 }
  }
  module SensitiveLoggerFlow = TaintTracking::Global<SensitiveLoggerConfig>; // 2: TaintTracking selected 
  
  import SensitiveLoggerFlow::PathGraph  // 7: the PathGraph specific to the module you are using
  
  from SensitiveLoggerFlow::PathNode source, SensitiveLoggerFlow::PathNode sink  // 8 & 9: using the module directly
  where SensitiveLoggerFlow::flowPath(source, sink)  // 9: using the flowPath from the module 
  select sink.getNode(), source, sink, "Sink is reached from $@.", source.getNode(), "here"

thank you!

[intrigus-lgtm]

Can you provide the full source code of your .ql file?
The snippet you shared does not contain the fromIccRecords and toSubscriptionManager method which are your source/sink.

[smowton]

The main problem is that dataflow doesn't go "to" a LocalVariableDeclExpr, rather it goes from the RHS of an initialiser or assignment straight to a read of the same variable. Therefore you want something like

predicate toSubscriptionInfoUpdater(DataFlow::Node sink) {
  exists(VariableAssign iccid |
      iccid.getCompilationUnit().toString()="SubscriptionInfoUpdater"
      and
      iccid.getDestVar().getName()="iccId"
      and
      sink.asExpr() = iccid.getSource()
  )
}

A few other notes:

1. VarAccess vac is unused; delete?
2. Relying on toString is usually a bad idea -- if you want "assignment happens in file Blah.java", then getFile().getBaseName() is probably better.
3. In the example the names of the pasted predicates and the predicates used in SensitiveLoggerConfig don't match, so you may have pasted the wrong code.

[smowton]

Tested with:

package testpkg;

public class Test {

  public static String stripTrailingFs() { return null; }

  protected void handleSimReady(int phoneId, Object uiccSlot) {
    String iccId = (uiccSlot != null) ? Test.stripTrailingFs() : null;
  }

}

[HeouDonkey]

thats very helpfull! thank you so much!

[HeouDonkey]

I've encountered another issue regarding the propagation of tainted data through arrays. I'm not sure if I've made a mistake in my query. The code I'm trying to analyze is as follows:

public class SubscriptionInfoUpdater extends Handler {
    ········
    protected void handleSimReady(int phoneId) {
        ········
        String iccId = (uiccSlot != null) ? IccUtils.stripTrailingFs(uiccSlot.getIccId()) : null;
        sIccId[phoneId] = iccId;
        ········
    }

    protected synchronized void updateSubscriptionInfoByIccId(int phoneId, boolean updateEmbeddedSubs) {
        ·······
        mSubscriptionManager.addSubscriptionInfoRecord(sIccId[phoneId], phoneId);
        ·······
    }
}

public class SubscriptionManager {
    ········
    public Uri addSubscriptionInfoRecord(String iccId, int slotIndex) {
    ········
    }
}

In this code snippet, I aim to consider the return value of getIccId() as a source of tainted data (it has been verified that the propagation from fromIccRecords to iccId is correct). Below is my query script:

predicate toSubscriptionManager(DataFlow::Node sink) {
    exists(Method method |
        method.getName() = "addSubscriptionInfoRecord"
        and
        sink.asParameter() = method.getParameter(0)
    )
}

predicate fromIccRecords(DataFlow::Node source) {
    exists(DataFlow::FieldValueNode fieldNode, Field field |
        source instanceof DataFlow::FieldValueNode
        and
        field.getQualifiedName() = "com.android.internal.telephony.uicc.IccRecords.mIccId"
        and
        fieldNode.getField() = field
        and
        source = fieldNode
    )
}

module SensitiveLoggerConfig implements DataFlow::ConfigSig {
    predicate isSource(DataFlow::Node source) {
        fromIccRecords(source)
    }
    predicate isSink(DataFlow::Node sink) {
        toSubscriptionManager(sink)
    }
    int fieldFlowBranchLimit() { result = 500 }
}

module TaintFlow = TaintTracking::Global<SensitiveLoggerConfig>;
import TaintFlow::PathGraph
from TaintFlow::PathNode source, TaintFlow::PathNode sink
where TaintFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "a flow to $@.",
source.getNode(), ""

Would you please review my approach and let me know if there are any issues?

[smowton]

It isn't clear from your example how the array carrying the tainted getIccId return is supposed to get to updateSubscriptionInfoByIccId.

For example, this works:

public class Test {

  public int source() { return 0; }

  public void sink(int x) { }

  private int[] arr;

  public void test(int key) {

    arr[key] = source();
    otherMethod(key);

  }

  public void otherMethod(int key) {

    sink(arr[key]);

  }

}

import java
import semmle.code.java.dataflow.TaintTracking

predicate toSubscriptionManager(DataFlow::Node sink) {
  sink.asExpr() = any(MethodCall mc | mc.getCallee().hasName("sink")).getArgument(0)
}

predicate fromIccRecords(DataFlow::Node source) {
  source.asExpr() = any(MethodCall mc | mc.getCallee().hasName("source"))
}

module SensitiveLoggerConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
      fromIccRecords(source)
  }
  predicate isSink(DataFlow::Node sink) {
      toSubscriptionManager(sink)
  }
  int fieldFlowBranchLimit() { result = 500 }
}

module TaintFlow = TaintTracking::Global<SensitiveLoggerConfig>;
import TaintFlow::PathGraph
from TaintFlow::PathNode source, TaintFlow::PathNode sink
where TaintFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "a flow to $@.",
source.getNode(), ""

Note that I use the argument to the sink method instead of the parameter as the sink, though the parameter works too. I recommend testing your code against a toy example like this and incrementally adding more complications to make it more like your real example to determine what exactly is going wrong here.

[HeouDonkey]

thankyou, i think the problem is the "static" ,please try with following sample:

public class Test {

  public int source() { return 0; }

  public void sink(int x) { System.out.println(x);}

  private static int[] arr;

  
  public void test(int key) {

    arr[key] = source();
    otherMethod(key);

  }

  public void otherMethod(int key) {

    sink(arr[key]);

  }

}

[smowton]

Thanks, we've confirmed that's a true problem and are investigating how best to fix it.