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

[MASWE-0007] Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction #2594

Merged
merged 39 commits into from
Jul 3, 2024

Conversation

serek8
Copy link
Collaborator

@serek8 serek8 commented Mar 5, 2024

Closes #2545.

@cpholguera cpholguera changed the title Add Risk and Tests for Sensitive Data Stored Unencrypted in External Storage Add Risk and Test - Sensitive Data Stored Unencrypted in External Storage [data-unencrypted-external] Apr 2, 2024
@@ -0,0 +1,37 @@
---
platform: android
title: Storing Data in External Locations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Storing Data in External Locations
title: Data Stored to External Locations on Runtime

frida-trace \
-U \
-f com.example \
-i "open" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is great to know the files but it won't get us the caller. I tried from a frida script to get the stack trace:

Stack trace:
0x74addead90 libjavacore.so!0x28d90
0x74addead90 libjavacore.so!0x28d90
0x700eda04 boot-core-libart.oat!0x12a04
0x700eda04 boot-core-libart.oat!0x12a04

maybe we can create another example for the Java/Kotlin methods

"java.io.FileOutputStream", "$init"
"java.io.FileWriter", "$init"
"java.io.BufferedWriter", "$init"
"java.io.PrintWriter", "$init"
"java.io.OutputStreamWriter", "$init"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hooking open will miss the files created using the MediaStore API because it seems to typically leverage pre-existing file descriptors or obtain them through complex interactions with content providers, which manage the actual file operations on behalf of the app.

If we'd hook open and write, when a MediaStore API operation occurs we'd only see write and not to open.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think hooking contentResolver.insert(MediaStore.Downloads.EXTERNAL_CONTENT_URI, contentValues) will be enough to cover MediaStore? Or should we also hook OutputStream? Or maybe get a file path by FD from write?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could replace this test with another one that is solely focused on the files themselves.

  • Benefits: quick way to get file changes and search sensitive data on them
  • Limitations: we'll only see files for the current testing session and we won't know what part of the code is responsible

Steps:

  1. Setup the initial time
start_time=$(date +%s)
  1. Exercise the app

  2. Diff and pull all new/changed files

adb shell "find /sdcard/ -type f -newermt @${start_time}" > new_files.txt

mkdir -p new_files
while read -r line; do
  adb pull "$line" ./new_files/
done < new_files.txt

Evaluation: inspect the files and if they contain sensitive data this test fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I will add it as another test

…rypted-external/android-data-unencrypted-external-frida/test.md

Co-authored-by: Carlos Holguera <[email protected]>
@serek8
Copy link
Collaborator Author

serek8 commented May 7, 2024

@cpholguera can you have a second look here? I updated the PR a lot.

Copy link
Collaborator

@cpholguera cpholguera left a comment

Choose a reason for hiding this comment

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

See more info in Slack.


Additionally, if the "external storage" is actually stored externally e.g. on an sd-card, it can be removed from the device and connected to a card reader to extract sensitive data.

### Testing Manifest permissions
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see what you were saying in our last call about this info being valid for all tests

For now you can keep it here but later, we can put it out of here.

As you may know we have new MASTG components for tests, techniques and tools but we're going to define a new one for "theory" or "knowledge". Currently we have all knowledge in this kind of chapters but it will be split into individual files such as MASTG-KNOW-0001.md (which will contain metadata as well, in this case sth. like title: External Storage). This way we can reuse it and reference to it from tests and other components and keep them as concise as possible.

https://github.com/OWASP/owasp-mastg/blob/master/Document/0x05d-Testing-Data-Storage.md#external-storage

Again, nothing to do for now, just keep it here until we decide otherwise.

@@ -0,0 +1,32 @@
---
platform: android
title: Find common APIs that return paths to External Storage locations
Copy link
Collaborator

Choose a reason for hiding this comment

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

This example seems to be about Scoped Storage, maybe we can adapt the title and text to it.

I created a new example that you can use for the non-scoped storage case: output-demo-4.zip

@cpholguera cpholguera changed the title Add Risk and Test - Sensitive Data Stored Unencrypted in External Storage [data-unencrypted-external] Add Risk and Test - Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction [data-unencrypted-shared-storage-no-user-interaction] May 19, 2024
@cpholguera cpholguera changed the title Add Risk and Test - Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction [data-unencrypted-shared-storage-no-user-interaction] Add Weakness and Test - Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction [data-unencrypted-shared-storage-no-user-interaction] Jul 3, 2024
Copy link
Collaborator

@cpholguera cpholguera left a comment

Choose a reason for hiding this comment

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

Awesome work @serek8! This PR is an excellent example of our new approach and the creation of MASWE items, tests and demos.

@cpholguera cpholguera merged commit 4e760f1 into OWASP:master Jul 3, 2024
3 checks passed
@cpholguera cpholguera changed the title Add Weakness and Test - Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction [data-unencrypted-shared-storage-no-user-interaction] [MASWE-0007] Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MASWE-0007] Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction
2 participants