Missed sinks with lists and statements uninvolved in the taint path including negative numbers

[draftyfrog]

I ran into an issue where FlowDroid misses a leak if some statements that don't affect the propagation are added.

Please consider the following example-code where FlowDroid misses the sink at the end of onCreate:

public void onCreate(Bundle savedInstanceState){
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    List<String> taint_list = new ArrayList<String>();
    taint_list.add(source());

    Integer unused = -1; // If this is removed (or changed to an int >= 0), the leak is detected
    List<Boolean> unused2 = new ArrayList<Boolean>(); // If this is removed, the leak is detected

    sink(taint_list);
}
public String source(){
    return "Secret";
}

public void sink(List<String> param){
}

As annotated, if one of the two statements not related to the taint path is removed (or changed), FlowDroid finds the leak.

I run FlowDroid via the command-line tool with

java -jar ./soot-infoflow-cmd-2.13.0-jar-with-dependencies.jar \
 -a {path-to-apk} \
 -s ./SourcesAndSinks.xml \
 -o ./out.xml \
 -p {path-to-android-platforms-folder} \
 --mergedexfiles

If relevant, my SourcesAndSinks.xml looks like this

<sinkSources>
    <category id="NO_CATEGORY">
        <method signature="com.example.testapp.MainActivity: java.lang.String source()">
            <return type="java.lang.String">
                <accessPath isSource="true" isSink="false">
                </accessPath>
            </return>
        </method>
        <method signature="com.example.testapp.MainActivity: void sink(java.util.List)">
            <param index="0" type="java.util.List">
                <accessPath isSource="false" isSink="true"/>
            </param>
        </method>
    </category>
</sinkSources>

[t1mlange]

FlowDroid/soot-infoflow/src/soot/jimple/infoflow/problems/rules/forward/StrongUpdatePropagationRule.java

Lines 83 to 90 in dd2de62

	// x = y && x.f tainted -> no taint propagated. This must only check the precise
	// variable which gets replaced, but not any potential strong aliases
	else if (lhs instanceof Local) {
	if (aliasing.mustAlias((Local) lhs, source.getAccessPath().getPlainValue(), stmt)) {
	killAll.value = true;
	return null;
	}
	}

Quite sure the bug is in the lines above. The comment states that no aliases should be killed here, however, the code proceeds to ask the must alias analysis whether the lhs and the taint are aliases. Replacing the function call with a simple equals check fixes your issue. However, I didn't test this change, so it might break other things.

[draftyfrog]

Using the current version of FlowDroid (including up to the latest commit e8b193e), it still misses the sink but the statement Integer unused = -1; doesn't seem to be relevant any more (only the removal of List<Boolean> unused2 = new ArrayList<Boolean>(); makes FlowDroid detect this leak).

[draftyfrog]

I tinkered around a little and discovered that this issue only happens if the uninvolved list is not used in between instantiation and the sink.

If we add any statement involving the list, FlowDroid correctly reports the leak:

public void onCreate(Bundle savedInstanceState){
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    List<String> taint_list = new ArrayList<String>();
    taint_list.add(source());

    List<Boolean> unused2 = new ArrayList<Boolean>();
    System.out.println(unused2); // If this statement is removed, FlowDroid doesn't report the leak in the next statement

    sink(taint_list);
}

[StevenArzt]

This the Jimple code:

    public void onCreate(android.os.Bundle)
    {
        android.os.Bundle $r1;
        int i0;
        java.lang.String $r2;
        com.example.javatestapp.MainActivity r0;
        java.util.ArrayList r3, $r4;

        r0 := @this: com.example.javatestapp.MainActivity;

        $r1 := @parameter0: android.os.Bundle;

        specialinvoke r0.<androidx.appcompat.app.AppCompatActivity: void onCreate(android.os.Bundle)>($r1);

        i0 = <com.example.javatestapp.R$layout: int activity_main>;

        virtualinvoke r0.<com.example.javatestapp.MainActivity: void setContentView(int)>(i0);

        $r4 = new java.util.ArrayList;

        r3 = $r4;

        specialinvoke $r4.<java.util.ArrayList: void <init>()>();

        $r2 = virtualinvoke r0.<com.example.javatestapp.MainActivity: java.lang.String source()>();

        interfaceinvoke $r4.<java.util.List: boolean add(java.lang.Object)>($r2);

        staticinvoke <java.lang.Integer: java.lang.Integer valueOf(int)>(-1);

        $r4 = new java.util.ArrayList;      // kills taint on r4

        specialinvoke $r4.<java.util.ArrayList: void <init>()>();

        virtualinvoke r0.<com.example.javatestapp.MainActivity: void sink(java.util.List)>(r3);

        return;
    }

The analysis found that r3 and r4 are must-aliases at the statement that kills the taint (when r4 is overwritten). Therefore, it kills the taint on r3 as well, which makes no sense.

[draftyfrog]

Using the latest version of FlowDroid (02dba8a) I can confirm that this issue no longer exists. Thank you!