Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cant find taint flow in a LocalVariable statement #15972

Closed
houanran-hzh opened this issue Mar 19, 2024 · 8 comments · Fixed by #16500
Closed

cant find taint flow in a LocalVariable statement #15972

houanran-hzh opened this issue Mar 19, 2024 · 8 comments · Fixed by #16500
Assignees
@smowton
Labels
Java question Further information is requested

Comments

@houanran-hzh
Copy link

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!
@houanran-hzh houanran-hzh added the question Further information is requested label Mar 19, 2024
@intrigus-lgtm
Copy link
Contributor

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
Copy link
Contributor

smowton commented Mar 19, 2024

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
Copy link
Contributor

smowton commented Mar 19, 2024

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;
  }

}

@houanran-hzh
Copy link
Author

thats very helpfull! thank you so much!

@houanran-hzh
Copy link
Author

houanran-hzh commented Mar 21, 2024 •
edited

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
Copy link
Contributor

smowton commented Mar 21, 2024

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.

@houanran-hzh
Copy link
Author

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
Copy link
Contributor

smowton commented Apr 4, 2024

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

@sidshank sidshank added the Java label Apr 30, 2024
@aschackmull aschackmull mentioned this issue May 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@smowton smowton

Labels
Java question Further information is requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Java: Add support for flow through side-effects on static fields. aschackmull/ql
4 participants
@smowton @sidshank @intrigus-lgtm @houanran-hzh