search

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

Propagate taints via object arguments #143

Closed pkesseli closed 1 year ago

pkesseli commented 1 year ago

Enable propagation of taints via object-type arguments. This behaviour is gated behind a new flag --propagate-via-arg and disabled by default.

Tests

Added a new end-to-end test propagation_via_arg checking the taint flow behaviour.

pkesseli commented 1 year ago

By the way, source/tests/integration/end-to-end/code/multi_sources failed on the previous CI run, and it seems flaky on main for me locally as well, so I assume that is unrelated to this PR.

arthaud commented 1 year ago

By the way, source/tests/integration/end-to-end/code/multi_sources failed on the previous CI run, and it seems flaky on main for me locally as well, so I assume that is unrelated to this PR.

Yeah, this is a known problem. I think our analysis is non deterministic.

pkesseli commented 1 year ago

Looks like main is failing as well with the same compilation error: https://github.com/facebook/mariana-trench/actions/runs/6404820726/job/17386110719

arthaud commented 1 year ago

Looks like main is failing as well with the same compilation error: https://github.com/facebook/mariana-trench/actions/runs/6404820726/job/17386110719

I see. I will look into that. Our internal build still works so it's surprising.

facebook-github-bot commented 1 year ago

@arthaud has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

arthaud commented 1 year ago

Looks like main is failing as well with the same compilation error: https://github.com/facebook/mariana-trench/actions/runs/6404820726/job/17386110719

I see. I will look into that. Our internal build still works so it's surprising.

This should be fixed by 50b66398cc35fd487372325674219fc45814296f. We still have the non-determinism issue with multi sources though.

arthaud commented 1 year ago

I have made a small change internally so we don't infer propagations Arg(i) -> Arg(i) for now. In the future we might need those for things like this.foo = add_feature(this.bar). We can improve that later if needed.

pkesseli commented 1 year ago

I have made a small change internally so we don't infer propagations Arg(i) -> Arg(i) for now. In the future we might need those for things like this.foo = add_feature(this.bar). We can improve that later if needed.

Thanks! Should I still rebase this PR or does it no longer matter, now that it's been imported?

facebook-github-bot commented 1 year ago

@arthaud merged this pull request in facebook/mariana-trench@5dc38a8b998ba301adf80b9f7495e769ded36aff.

arthaud commented 1 year ago

Thanks! Should I still rebase this PR or does it no longer matter, now that it's been imported?

You don't need to do anything. It's been merged. Thanks for the contribution!