secure-software-engineering / FlowDroid

FlowDroid Static Data Flow Tracker
GNU Lesser General Public License v2.1
1.03k stars 293 forks source link

FastCallbackAnalyzer unsoundly deals with anonymous classes #500

Open amordahl opened 1 year ago

amordahl commented 1 year ago

Issue

The fast callback analyzer (i.e., running FlowDroid with configuration callbackanalyzer set to FAST) unsoundly deals with anonymous classes.

Test Cases

Many of the APKs in Droidbench 3.0's Callbacks category, including AnonymousClass1.apk, LocationLeak{1,2,3}.apk, RegisterGlobal{1,2}.apk, Button{2,3}.apk.

Observations

As far as I can tell, the issue is caused by line 50 of FastCallbackAnalyzer.java

for (SootClass sc : Scene.v().getApplicationClasses()) {
    if (sc.isConcrete()) {
        for (SootMethod sm : sc.getMethods()) {
            if (sm.isConcrete()) {
                analyzeMethodForCallbackRegistrations(null, sm); // HERE
                analyzeMethodForDynamicBroadcastReceiver(sm);
                analyzeMethodForServiceConnection(sm);
            }
        }

        // Check for method overrides
        analyzeMethodOverrideCallbacks(sc);
    }
}

When the method(s) implemented by the anonymous class are analyzed in the loop, and they are analyzed for callback registrations, the first parameter passed to analyzeMethodForCallbackRegistrations is null. The first parameter of analyzeMethodForCallbackRegistrations is supposed to be the lifecycle element to which the method should be associated, but by setting it to null, methods defined in callback classes are not reachable.

StevenArzt commented 1 year ago

The FastCallbackAnalyzer is indeed a hack.

Our default callback handler iteratively creates a model for reachable callbacks. It starts with the lifecycle methods of the Android components, looks for registered handlers, adds them to the model, checks for new handlers that have now become reachable, etc., and iterates until it reaches a fixpoint. This is quite time-consuming for large apps, but we have a clear link between the component and the callback that is (transitively) registered.

In the fast analyzer, we tried to avoid this effort by looking at the app as a single snapshot. We don't care where a callback is registered, we just want a set of callbacks. While this is obviously fast and simple, we no longer know where to put the callback invocation in our dummy main method. There is no link to the parent component (hence the null).

So what to do about the issue? We have tried associating each callback with each component. This doesn't scale in the data flow analysis, because of all the spurious taints. If someone has a better idea, I'm open to give some sort of a "fast analyzer" a second go. Until then, I'd rather drop the fast analyzer and stick with the default one.