-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[MASWE-0007] Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction #2594
Changes from 1 commit
0bc19a7
af50568
dc5a465
e0ba4e1
7d23021
d2df0de
cc126f0
3cb96d2
dfb01ff
badad99
5e5c8dc
ac00a36
44b625b
bdcea87
1d0dbb6
9ad1169
e8fab33
e8093a9
d79b2d8
e17a638
e7c2902
769257c
709bfd4
77ddf31
247a928
858b4bd
81610ae
665171e
252ac64
7f4809c
dff1834
f68d567
d61e867
99d0776
e373dcc
d44da95
b8ad87d
b582669
edeee04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
--- | ||
platform: android | ||
title: File Tracing | ||
tools: [frida-trace] | ||
code: [kotlin] | ||
--- | ||
|
||
### Sample | ||
|
||
The snippet below shows sample code that creates a file in external storage. You can put this code into your app and follow the steps to see a sample usage of `frida-trace` to identify a potential data leak. | ||
|
||
{{ snippet.kt }} | ||
|
||
### Steps | ||
|
||
Execute `frida-trace` against the sample app to trace all usage of file IO. | ||
|
||
{{ run.sh }} | ||
|
||
### Observation | ||
|
||
`frida-trace` has identified one file path in the external storage that the app opened. | ||
|
||
{{ output.txt }} | ||
|
||
### Evaluation | ||
|
||
Review each of the reported instances by manually opening a file and inspecting its content. | ||
|
||
NOTE: If you want to test more file locations than only the file locations inside the external storage, remove `grep` from the script. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
638 ms open(path="/storage/emulated/0/Android/data/com.example/files/secret.json", oflag=0x241) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
#!/bin/bash | ||
|
||
# SUMMARY: This script uses frida-trace to trace files that an app has opened since it spawned | ||
# The script filters the output of frida-trace to print only the paths belonging to external | ||
# storage but the the predefined list of external storage paths might not be complete. | ||
# A sample output is shown in "output.txt". If the output is empty, it indicates that no external | ||
# storage is used. | ||
|
||
frida-trace \ | ||
-U \ | ||
-f com.example \ | ||
-i "open" \ | ||
| grep -E "(/sdcard|/storage)" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
// On lower Android verions, you might need to add `WRITE_EXTERNAL_STORAGE` permission to the manifest to write to an external app-specific directory. | ||
serek8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
val externalDirPath = getExternalFilesDir(null)!!.absolutePath | ||
val file: File = File("$externalDirPath/secret.json") | ||
FileOutputStream(file).use { fos -> | ||
fos.write("password:123".toByteArray()) | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,37 @@ | ||||||
--- | ||||||
platform: android | ||||||
title: Storing Data in External Locations | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
apis: [Environment#getExternalStorageDirectory, Environment#getExternalStorageDirectory, Environment#getExternalFilesDir, Environment#getExternalCacheDir, SharedPreferences, FileOutPutStream] | ||||||
type: [dynamic] | ||||||
--- | ||||||
|
||||||
## Overview | ||||||
cpholguera marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
Android apps utilize a variety of APIs to obtain a file path and save a file. Collecting a comprehensive list of these APIs can be challenging, especially when an app employs a third-party framework, loads code at runtime, or incorporates native code. Therefore, dynamic testing might be the most effective approach to find writing to the external storage. | ||||||
serek8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
## Steps | ||||||
|
||||||
1. Install the app. | ||||||
|
||||||
2. Execute a [method trace](https://mas.owasp.org/MASTG/techniques/android/MASTG-TECH-00xx/) to spawn an app and log all interactions with files. | ||||||
|
||||||
3. Navigate to the screen of the mobile app that you want to analyse. | ||||||
|
||||||
4. Close the app to stop `frida-trace` | ||||||
|
||||||
|
||||||
## Observation | ||||||
cpholguera marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
The **method trace output** contains a list of file locations that your app interacts with. You may need to use [adb shell](https://mas.owasp.org/MASTG/techniques/android/MASTG-TECH-0002/) to inspect these files manually. | ||||||
|
||||||
## Evaluation | ||||||
|
||||||
The test case fails if the files found above are not encrypted and leak sensitive data. | ||||||
|
||||||
For example, the following output shows sample files that should be manually inspected. | ||||||
|
||||||
```shell | ||||||
/storage/emulated/0/Android/data/com.example/keys.json | ||||||
/storage/emulated/0/Android/data/com.example/files/config.xml | ||||||
/sdcard/secret.txt" | ||||||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
xmlns:tools="http://schemas.android.com/tools"> | ||
|
||
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> | ||
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" /> | ||
|
||
<application | ||
android:allowBackup="true" | ||
android:dataExtractionRules="@xml/data_extraction_rules" | ||
android:fullBackupContent="@xml/backup_rules" | ||
android:icon="@mipmap/ic_launcher" | ||
android:label="@string/app_name" | ||
android:roundIcon="@mipmap/ic_launcher_round" | ||
android:supportsRtl="true" | ||
android:theme="@style/Theme.MyKotlin" | ||
tools:targetApi="31"> | ||
<activity | ||
android:name=".MainActivity" | ||
android:exported="true"> | ||
<intent-filter> | ||
<action android:name="android.intent.action.MAIN" /> | ||
|
||
<category android:name="android.intent.category.LAUNCHER" /> | ||
</intent-filter> | ||
</activity> | ||
</application> | ||
|
||
</manifest> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
--- | ||
platform: android | ||
title: Common APIs to return paths to External Storage | ||
tools: [semgrep] | ||
code: [java] | ||
--- | ||
|
||
### Sample | ||
|
||
{{ use-of-external-store.kt }} | ||
|
||
### Steps | ||
|
||
Let's run our semgrep rule against the sample manifest file. | ||
|
||
{{ ../rules/mastg-android-data-unencrypted-external-manifest.yml }} | ||
|
||
{{ run.sh }} | ||
|
||
### Observation | ||
|
||
The rule has identified that the manifest file declares `WRITE_EXTERNAL_STORAGE` permission at line 9. | ||
|
||
{{ output.txt }} | ||
|
||
### Evaluation | ||
|
||
Review your code to make sure you don't store sensitive unencrypted data in the external storage unintentionally. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
{ | ||
"$schema": "https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/schemas/sarif-schema-2.1.0.json", | ||
"runs": [ | ||
{ | ||
"invocations": [ | ||
{ | ||
"executionSuccessful": true, | ||
"toolExecutionNotifications": [] | ||
} | ||
], | ||
"results": [], | ||
"tool": { | ||
"driver": { | ||
"name": "Semgrep OSS", | ||
"rules": [ | ||
{ | ||
"defaultConfiguration": { | ||
"level": "warning" | ||
}, | ||
"fullDescription": { | ||
"text": "[MASVS-STORAGE] Make sure you encrypt files in external storage if necessary" | ||
}, | ||
"help": { | ||
"markdown": "[MASVS-STORAGE] Make sure you encrypt files in external storage if necessary", | ||
"text": "[MASVS-STORAGE] Make sure you encrypt files in external storage if necessary" | ||
}, | ||
"id": "rules.mastg-android-data-unencrypted-external", | ||
"name": "rules.mastg-android-data-unencrypted-external", | ||
"properties": { | ||
"precision": "very-high", | ||
"tags": [] | ||
}, | ||
"shortDescription": { | ||
"text": "Semgrep Finding: rules.mastg-android-data-unencrypted-external" | ||
} | ||
} | ||
], | ||
"semanticVersion": "1.63.0" | ||
} | ||
} | ||
} | ||
], | ||
"version": "2.1.0" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
|
||
|
||
┌────────────────┐ | ||
│ 1 Code Finding │ | ||
└────────────────┘ | ||
|
||
AndroidManifest.xml | ||
mrules.mastg-android-data-unencrypted-external | ||
[MASVS-STORAGE] Make sure you encrypt files in external storage if necessary | ||
|
||
5┆ <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external-manifest.yml ./AndroidManifest.xml --text -o output.txt | ||
NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external-manifest.yml ./AndroidManifest.xml --sarif -o output.sarif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
rules: | ||
- id: mastg-android-data-unencrypted-external-manifest | ||
severity: WARNING | ||
languages: | ||
- generic | ||
metadata: | ||
summary: This rule looks for permissions that allows your app to write in External Storage | ||
message: "[MASVS-STORAGE] Make sure you encrypt files in external storage if necessary" | ||
patterns: | ||
- pattern: WRITE_EXTERNAL_STORAGE |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can merge this one with https://github.com/OWASP/owasp-mastg/pull/2594/files#diff-10572627ebb61180bb70a16dc8ec124986d53d4d012be5515179570cd4c99732 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Steps:
start_time=$(date +%s)
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I will add it as another test |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
--- | ||
platform: android | ||
title: Declaration of the external storage permission | ||
apis: [] | ||
type: [static] | ||
--- | ||
|
||
## Overview | ||
|
||
An app must declare an intent to write to external storage in order to save files in the public locations. | ||
|
||
## Steps | ||
|
||
1. Run a [static analysis](../../../../../techniques/android/MASTG-TECH-0014.md) tool on the app and look for a use of sensitive permissions. | ||
|
||
|
||
## Observation | ||
|
||
The output shows that the manifest files declares `WRITE_EXTERNAL_STORAGE` permission at line 5. | ||
|
||
## Evaluation | ||
|
||
Inspect app's source code to make sure the data stored externally is secure. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
--- | ||
platform: android | ||
title: Find common APIs that return paths to External Storage locations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
tools: [semgrep] | ||
code: [java] | ||
--- | ||
|
||
### Sample | ||
|
||
{{ use-of-external-store.kt }} | ||
|
||
### Steps | ||
|
||
Let's run our semgrep rule against the sample code. | ||
|
||
{{ ../rules/mastg-android-data-unencrypted-external.yml }} | ||
|
||
{{ run.sh }} | ||
|
||
### Observation | ||
|
||
The rule has identified 1 location in the code file where a path to external storage is retuened. Make sure you don't store unencrypted code there unintentionally. | ||
|
||
{{ output.txt }} | ||
|
||
### Evaluation | ||
|
||
Review each of the reported instances. In this case, it's only one instance. Line 9 shows the occurence of API that returns external storage location. Make sure to either: | ||
serek8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- encrypt this file if necessary | ||
- move the file to the internal storage | ||
- keep the file in the same location if intended and secure | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
{ | ||
"$schema": "https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/schemas/sarif-schema-2.1.0.json", | ||
"runs": [ | ||
{ | ||
"invocations": [ | ||
{ | ||
"executionSuccessful": true, | ||
"toolExecutionNotifications": [] | ||
} | ||
], | ||
"results": [ | ||
{ | ||
"fingerprints": { | ||
"matchBasedId/v1": "7fff0cca477ae1b0a93a5b8e265d8a7471c4144464dcbbd55d7256be707b55568882a55bbac36d334816deb905d562cb7f9c4cee168fb02f8ca6f31936c62fb7_0" | ||
}, | ||
"locations": [ | ||
{ | ||
"physicalLocation": { | ||
"artifactLocation": { | ||
"uri": "use-of-external-store.kt", | ||
"uriBaseId": "%SRCROOT%" | ||
}, | ||
"region": { | ||
"endColumn": 101, | ||
"endLine": 26, | ||
"snippet": { | ||
"text": " val externalDirPath = getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS)" | ||
}, | ||
"startColumn": 35, | ||
"startLine": 26 | ||
} | ||
} | ||
} | ||
], | ||
"message": { | ||
"text": "[MASVS-STORAGE] Make sure you encrypt files at these locations if necessary" | ||
}, | ||
"properties": {}, | ||
"ruleId": "rules.mastg-android-data-unencrypted-external" | ||
} | ||
], | ||
"tool": { | ||
"driver": { | ||
"name": "Semgrep OSS", | ||
"rules": [ | ||
{ | ||
"defaultConfiguration": { | ||
"level": "warning" | ||
}, | ||
"fullDescription": { | ||
"text": "[MASVS-STORAGE] Make sure you encrypt files at these locations if necessary" | ||
}, | ||
"help": { | ||
"markdown": "[MASVS-STORAGE] Make sure you encrypt files at these locations if necessary", | ||
"text": "[MASVS-STORAGE] Make sure you encrypt files at these locations if necessary" | ||
}, | ||
"id": "rules.mastg-android-data-unencrypted-external", | ||
"name": "rules.mastg-android-data-unencrypted-external", | ||
"properties": { | ||
"precision": "very-high", | ||
"tags": [] | ||
}, | ||
"shortDescription": { | ||
"text": "Semgrep Finding: rules.mastg-android-data-unencrypted-external" | ||
} | ||
} | ||
], | ||
"semanticVersion": "1.63.0" | ||
} | ||
} | ||
} | ||
], | ||
"version": "2.1.0" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
┌────────────────┐ | ||
│ 1 Code Finding │ | ||
└────────────────┘ | ||
|
||
use-of-external-store.kt | ||
rules.mastg-android-data-unencrypted-external 0m | ||
[MASVS-STORAGE] Make sure you encrypt files at these locations if necessary | ||
|
||
26┆ val externalDirPath = getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./use-of-external-store.kt --text -o output.txt | ||
NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./use-of-external-store.kt --sarif -o output.sarif | ||
serek8 marked this conversation as resolved.
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 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"
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.
Hooking
open
will miss the files created using theMediaStore
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
andwrite
, when aMediaStore
API operation occurs we'd only seewrite
and not toopen
.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.
Do you think hooking
contentResolver.insert(MediaStore.Downloads.EXTERNAL_CONTENT_URI, contentValues)
will be enough to cover MediaStore? Or should we also hookOutputStream
? Or maybe get a file path by FD fromwrite
?