-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Port MASTG-TEST-0005: Determining Whether Sensitive Data Is Shared with Third Parties via Notifications (android) #3464
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the creation of this new test case, that looks really good! The code works in the testing app, I could build it in Android Studio and the semgrep rules work too.
I made a few suggestions, please let me know what you think.
Regarding your note:
Note: For the demo notification to be visible (doesn't affect test functionality), we need to grant the POST_NOTIFICATIONS permission, i.e. adb shell pm grant org.owasp.mastestapp android.permission.POST_NOTIFICATIONS (or manually). This does not affect testability. Should that be visible?
It would make sense to point this out in the steps of the demo, in case someone wants to verify the notifications when running the app.
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sushi2k thanks for the review! Please check my comments.
@sushi2k + @cpholguera Let's allocate the final ID for both demo + test when all other topics are resolved.
rules/mastg-android-sensitive-data-in-notifications-manifest.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test and the corresponding demo should be in PRIVACY (even though it was in Platform before) and we should give them a "privacy" focus. The fact that the app puts PII in notifications isn't considered a leak, the developer is adding that very much intentionally, right? It's not like they could have done something (e.g., configure something) to prevent a "leak". Therefore, this seems like a privacy concern to me.
We consider sensitive data (this page needs improvement): PII, authenticators and cryptographic material. Many (if not most apps) have a legitimate reason to add PII such as contact names, message content, transaction amounts, etc. to notifications. Please consider this when going through the test and demo again.
Suggestion for the test title "PII Exposure via Notifications"
|
@cpholguera, I see your points. There was the suggestion by @sushi2k to move this to PLATFORM, with whom (based on my best knowledge) I tend to agree. But let's analyse it! Our basis definition is: "sensitive data" = "PIIs" + "other data that a company has defined that should not be public" I tried to think of a few notification examples for the "2nd category":
All those notifications are not PIIs but should be treated confidentially (depending on the company's policy). Now, regarding moving this to PRIVACY, I quote from PLATFORM MASVS:
This fits exactly with the demo/test. Wdyt? |
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
Co-authored-by: Sven <[email protected]>
This PR closes #2939
Description
Port mastg test 0005 from v1 to v2 incl. demo
TODO:
Note:✅For the demo notification to be visible (doesn't affect test functionality), we need to grant the POST_NOTIFICATIONS permission, i.e.
adb shell pm grant org.owasp.mastestapp android.permission.POST_NOTIFICATIONS(or manually). This does not affect testability. Should that be visible?