facebook / mariana-trench

A security focused static analysis tool for Android and Java applications.
https://mariana-tren.ch/
MIT License
1.1k stars 139 forks source link

False positive with taint propagated via class attribute #173

Open draftyfrog opened 3 days ago

draftyfrog commented 3 days ago

Bug

Bug description Please consider the following code

MyClass myInstance = new MyClass();
String myString = myInstance.myField;
myInstance.myField = source();
sink(myString); // Mariana Trench falsely reports this as issue

where public String source() and public void sink(String param) are defined as source and sink respectively in Marian Trenchs config and the implementation if MyClass is simply

class MyClass{
  String myField = "";
}

Running Mariana Trench on this code returns one issue (as annotated in the code above), but actually no taint is leaked in this code.

I'm using mariana-trench Version: 1.0.6

arthaud commented 1 day ago

I understand why that could be confusing, but this is the expected behavior. We consider that if an object is tainted, then any field from that object is also tainted.

Unfortunately, I would need to go into a lot of theoretical details to explain why, but basically, without that behavior, we would have a lot of false negatives. The design choice in our analyzers is to favor false positives (such as this one) rather than false negatives.

Note that the issue found here should have a feature called via-issue-broadening. You could consider excluding issues with that breadcrumb from your results (for instance, using sapp filters).

Finally, note that all this is documented here: https://www.internalfb.com/intern/staticdocs/mariana-trench/docs/models/#issue-broadening-feature I would recommend to read the entire section about taint broadening.