[Android] Enabling dataflow through OS triggered callbacks (ServiceConnection)

[szwl]

Hi all,
I'm trying to figure out a problem with taint propagation in an Android application.

Example code:

  public static void sourceMethod(Context context, SomeObject options) {
    ServiceConnection connection =
        new ServiceConnection() {
          @Override
          public void onServiceConnected(ComponentName name, IBinder service) {
            try {
              ISomeService serviceInterface =
                ISomeService.Stub.asInterface(service);
                  serviceInterface.doStuff(options);
            } catch (RemoteException e) {} 
          }

          @Override
          public void onServiceDisconnected(ComponentName name) {}
        };

    Intent serviceIntent =
        new Intent().setClassName(context, "Some class name");

    context.bindService(serviceIntent, connection, BIND_AUTO_CREATE);
  }

My query:

module TaintPropagationTestConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) { 
      exists(MethodCall mc |
        mc.getMethod().hasName("sourceMethod") and mc.getMethod().isStatic() and source.asExpr() = mc.getArgument(1)
      )
  }

  predicate isSink(DataFlow::Node sink) { 
      // to debug taint propagation
      any()
  }
}


module TaintPropagationTestConfigFlow = TaintTracking::Global<TaintPropagationTestConfig>;

from TaintPropagationTestConfigFlow::PathNode source, TaintPropagationTestConfigFlow::PathNode sink
where TaintPropagationTestConfigFlow::flowPath(source, sink)
select sink.getNode(), sink, source, source.getNode()

The problem:

It seems that codeql doesn't analyze onServiceConnected method, so I'm unable to propagate taint to serviceInterface.doStuff.
I can workaround this by adding an AdditionalTaintStep eg.

private class ServiceConnectionStep extends AdditionalTaintStep {
  override predicate step(DataFlow::Node n1, DataFlow::Node n2){
    exists(VarAccess va | va.getVariable() = n1.asParameter().(Variable) and va = n2.asExpr())
  }
}

however, it seems to be a hack rather than a solution that would work for more applications.

Questions:

1. How can I instruct codeql that context.bindService will eventually trigger connection.onServiceConnected?
2. Is AdditionalDataFlowNode a solution here? If yes, how am I supposed to use it?

[jketema]

Hi,

Extending AdditionalTaintStep to handle this is a good approach. The same is, for example, done in the Java CodeQL library for Android's AsyncTask::execute

codeql/java/ql/lib/semmle/code/java/frameworks/android/AsyncTask.qll

Lines 16 to 25 in c989e01

	private class AsyncTaskExecuteAdditionalValueStep extends AdditionalTaintStep {
	override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
	exists(ExecuteAsyncTaskMethodCall ma, AsyncTaskRunInBackgroundMethod m |
	DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType()
	|
	node1.asExpr() = ma.getParamsArgument() and
	node2.asParameter() = m.getParameter(0)
	)
	}
	}

[jketema]

I'm not sure why you think that AdditionalDataFlowNode would help? You will still need to introduce addition steps to and from that additional node. Presumably a step from your options parameter to the node, and a step from the node to the use of options in onServiceConnected.

[szwl]

I'm not very good with CodeQL, so maybe indeed this is completely wrong :) It's challenging to me to understand parts of documentation (especially if there is no example using that API).

My general idea would be to introduce another node whenever bindService call is detected - this node would be a fake method call on connection object (connection.onServiceConnected). My hope was that this way CodeQL wouldn't consider that path as unreachable and correctly taint options variable access.

Access of variables from outer scope in lambda/anonymous functions are generally tainted correctly I checked with Java Supplier API that has similar properties to my case:

// propagates taint correctly, finds doStuff sink
((Supplier<Void>) () -> {
  serviceInterface.doStuff(options);
}).get();

// doesn't find the doStuff sink, taint not passed 
Supplier<Void> ignored = ((Supplier<Void>) () -> {
  serviceInterface.doStuff(options);
});

There is a difference for CodeQL whether get() is or isn't called - I believe that finding what the difference is would help me solve my issue.

[jketema]

I'm not very good with CodeQL, so maybe indeed this is completely wrong :) It's challenging to me to understand parts of documentation (especially if there is no example using that API).

The dataflow library is not the easiest piece of QL code, so that's fully understandable. Note that there's one example that uses AddinitionalDataFlowNode, see the PR that introduced the class: #14297.

The difference between your two Supplier<Void> examples strikes me as odd. You might want to open an issue in this repo (not a discussion) to flag that up. I would expect there to be no difference there. This is of course still very much different from the original issue, as all the code is known in this case, while it isn't in your original problem.

[szwl]

Thanks, I'll take a look at this PR.

I actually think this is working as intended - ignored supplier is never "get()", so its code would never run. However, understanding how is that distinction made, could be a key to solve my problem.

[jketema]

Access of variables from outer scope in lambda/anonymous functions are generally tainted correctly I checked with Java Supplier API that has similar properties to my case:

// propagates taint correctly, finds doStuff sink
((Supplier<Void>) () -> {
  serviceInterface.doStuff(options);
}).get();

// doesn't find the doStuff sink, taint not passed 
Supplier<Void> ignored = ((Supplier<Void>) () -> {
  serviceInterface.doStuff(options);
});

There is a difference for CodeQL whether get() is or isn't called - I believe that finding what the difference is would help me solve my issue.

I'm not sure how similar this is to your situation, actually. Did you have a look at the actual dataflow path that is produced by the first of these two examples?

Note that the get method for Supplier is modelled here:

codeql/java/ql/lib/ext/com.google.common.base.model.yml

Line 95 in ffab199

- ["com.google.common.base", "Supplier", True, "get", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]