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

Only warn about singleTask/singleInstance when API level is <21 #750

Merged
merged 3 commits into from
Oct 28, 2018

Conversation

vollkorn1982
Copy link
Contributor

@vollkorn1982 vollkorn1982 commented Oct 16, 2018

What was a problem?

The underlying vulnerability is that with ActivityManager.getRecentTasks()
any app with the permission GET_TASKS can read the Intents sent to create
the task. This would reveal the possibly sensitive contents of these Intents.
MobSF warns about Activities using singleTask/singleInstance because these
would be affected by this issue.

According to the Android documentation at https://developer.android.com/reference/android/app/ActivityManager.html#getRecentTasks(int,%20int)
starting with API level 21 it is no longer possible to read all Intents sent
to a root activity by using ActivityManager.getRecentTasks(). After API
level 21, only Intents sent from your own app and other apps to be known
insensitive like the home/launcher app.

See also chapter 4.1.3.3 (page 94) in http://www.jssec.org/dl/android_securecoding_en.pdf

How this PR fixes the problem?

This PR checks if the min_sdk level is less than 21 and only then applies this issue in the manifest analysis.

Check lists (check x in [ ] of list items)

  • Run MobSF unit tests
  • Tested Working on Linux, Mac, and Windows
  • Coding style (indentation, etc)

Additional Comments (if any)

Only testet on Linux.

I did not find how to run the unit tests. Standard `python manage.py tests` runs 0 tests.

… than 21 is supported

The underlying vulnerability is that with ActivityManager.getRecentTasks()
any app with the permission GET_TASKS can read the Intents sent to create
the task. This would reveal the possibly sensitive contents of these Intents.

According to the Android documentation at https://developer.android.com/reference/android/app/ActivityManager.html#getRecentTasks(int,%20int)
starting with API level 21 it is no longer possible to read all Intents sent
to a root activity by using ActivityManager.getRecentTasks(). After API
level 21, only Intents sent from your own app and other apps to be known
insensitive like the home/launcher app.

See also chapter 4.1.3.3 (page 94) in http://www.jssec.org/dl/android_securecoding_en.pdf
@matandobr
Copy link
Member

Did you run the tests?

@vollkorn1982
Copy link
Contributor Author

@matandobr No, see the last line of my submission message.

@matandobr
Copy link
Member

It is through the browser, head to /runtest/

@vollkorn1982
Copy link
Contributor Author

[2018-10-16 18:43:47]
[ERROR] Performing Manifest Analysis (/home/work/Dokumente/Tools/mobsf_fork/Mobile-Security-Framework-MobSF/StaticAnalyzer/views/android/manifest_analysis.py, LINE 765 "int(man_data_dic['min_sdk']) < 21 and"): invalid literal for int() with base 10: ''
[ERROR] 'NoneType' object is not subscriptable
Internal Server Error: /StaticAnalyzer/
  [ERROR] Performing Static Analysis:  /StaticAnalyzer/?name=android_src.zip&type=zip&checksum=52c50ae824e329ba8b5b7a0f523efffe

I'll figure out tomorrow how to fix this.

@superpoussin22
Copy link
Collaborator

sounds like min_sdk is not an integer but empty in your case

@vollkorn1982
Copy link
Contributor Author

vollkorn1982 commented Oct 17, 2018

It's fine when running a real apk analysis. From my understanding it's impossible to build an .apk without having a min_sdk version set somewhere in the process. Therefore, I believe this to be a missing part of the test suite. Nonetheless I could insert a test if the value does not exist or is None, just to be on the safe side.

@sydowma
Copy link
Member

sydowma commented Oct 17, 2018

on the top of file, and use this constant.
like this

ANDROID_5_0_LEVEL = 21

@vollkorn1982
Copy link
Contributor Author

@magaofei Well, I built this in the fashion of the already existing code at line 1034 and 1035 and would make code clean-ups in a separate PR. But here you go.

@vollkorn1982
Copy link
Contributor Author

Btw: You really shouldn't talk about constants in Python. There is no such thing. (At least in the Python language or stdlib. You can build your own consts though.)

@sydowma
Copy link
Member

sydowma commented Oct 17, 2018

@sydowma
Copy link
Member

sydowma commented Oct 17, 2018

@vollkorn1982 Btw: You should not use except, you should specify exception

@vollkorn1982
Copy link
Contributor Author

@magaofei From the category "Naming Conventions". You see the problem yourself? ;)

Anymore changes? Please don't feed them to me one by one, but make a list I can work off at once. Thank you.

@sydowma
Copy link
Member

sydowma commented Oct 17, 2018

@vollkorn1982 If you come over and discuss the constants and naming conventions with me, you don't need them. I only pay attention to the results.

Copy link
Member

@ajinabraham ajinabraham left a comment

Choose a reason for hiding this comment

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

Test works fine!
Thanks @vollkorn1982 for the PR

@ajinabraham ajinabraham merged commit 3893415 into MobSF:master Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants