Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dexdump #35

Closed
wants to merge 3 commits into from
Closed

Conversation

counter-reverse
Copy link
Contributor

@counter-reverse counter-reverse commented Feb 16, 2020

Solve this issue: #9 radically by removing the regex. The regex used to get the wrong number of classes because these include types and superclasses. The unit test are also changed to get the real number of classes in a way that does not include types and superclasses.

Avoid any risk to get a command injection: #7

Solve this issue: #17 (no dexdump implies no check)

Make the code more readable: #16

Copy link
Member

@pnu-s pnu-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look really cool if this works as expected.
I need to make tests with this new code to make sure it doesn't break anything

exodus_core/analysis/static_analysis.py Outdated Show resolved Hide resolved
exodus_core/analysis/static_analysis.py Show resolved Hide resolved
exodus_core/analysis/static_analysis.py Outdated Show resolved Hide resolved
Copy link
Member

@pnu-s pnu-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made a few tests and there is a problem with some applications.
For instance this one: https://reports.exodus-privacy.eu.org/en/reports/128219/

With the new method, we do not detect 3 trackers:

  • Facebook ads
  • Millennial Media
  • Twitter Mopub

@counter-reverse
Copy link
Contributor Author

I just made a few tests and there is a problem with some applications.
For instance this one: https://reports.exodus-privacy.eu.org/en/reports/128219/

With the new method, we do not detect 3 trackers:

* Facebook ads

* Millennial Media

* Twitter Mopub

I have manually opened the apk textra. Then I understand what is going on.

My current program get all classes defined in the apk. I believed all the imports are present in the apk. I was wrong. It probably only contains overided method classes.

I have modified my code to list all the classes that contain a method called in any class defined in the binary. My code now works and detects the 8 trackers of textra. My code is still a bit slow. I am fixing that.

I am going to push.

@counter-reverse counter-reverse force-pushed the dexdump branch 2 times, most recently from 0f2a4a4 to 5d96c49 Compare May 4, 2020 21:10
@counter-reverse
Copy link
Contributor Author

@pnu-s I pushed. Now my code check each call in each class to find a tracker. It works. It is cleaner to read in the source code.

@counter-reverse
Copy link
Contributor Author

I have checked if the tracker was called before that I add it to the tracker list. I think I should have done that in another branch. The software is now slower. If you want I can come back to the old version of my pull request with no check. It will be quicker.

@counter-reverse
Copy link
Contributor Author

counter-reverse commented Oct 31, 2020

Some classses were missings. I have updated the unit tests on the classes. All is ok. We can merge!

Copy link
Contributor

@Gu1nness Gu1nness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the which function at the top of the file.

setup.py Show resolved Hide resolved
@counter-reverse
Copy link
Contributor Author

Nice.

I updated the unit test because the command used to add non-class object such as type and used to add it to the class list.

I also updated the unit test because the command used to ignore parent classes.

I think we can merge. :)

@counter-reverse
Copy link
Contributor Author

I really would like you check my pull request.

@counter-reverse
Copy link
Contributor Author

counter-reverse commented Dec 11, 2020

For the unit test I never knew what was the right number of classes. Too much to count and test. The current dexdump command takes more than it should.

But now I am sure to get the right number of class with the command radare2 classes.dex; icq; to list each. I will do it for each dex for each APK of the unit test and it should be nice.

@pnu-s
Copy link
Member

pnu-s commented Dec 15, 2020

@counter-reverse I'll take a look once more at this PR. I cannot guarantee when I will have time to do this though, but I'll try my best.

If you can resolve the conflict on the travis CI file that would be nice.

@counter-reverse
Copy link
Contributor Author

counter-reverse commented Dec 27, 2020

Yes @pnu-s I will solve the conflict. Can you just tell me if everything is fine first please?

@counter-reverse
Copy link
Contributor Author

@pnu-s I solved the conflict. Fell free to check.

@pnu-s
Copy link
Member

pnu-s commented Mar 1, 2022

As Androguard is not maintained anymore, I'm not sure we want to use even more of this tool and increase our dependency.

Closing this then.

@pnu-s pnu-s closed this Mar 1, 2022
@counter-reverse
Copy link
Contributor Author

@pnu-s exodus core was already using androguard.

I can only agree with the the fact it is a bad choice.

Could you consider this PR as an incremental work please?

Please accept this PR (after a rebase) and I will make a new one to migrate from androguard to lief for EVERY line of code of exodus privacy. Not only my pull request.

I spent some full months to make this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants