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

Add Weakness and Test - Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction [data-unencrypted-shared-storage-no-user-interaction] #2594

Merged
merged 39 commits into from
Jul 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
0bc19a7
Add Risk and Tests for: Sensitive Data Stored Unencrypted in External…
serek8 Mar 5, 2024
af50568
Update risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unenc…
serek8 May 6, 2024
dc5a465
Fix spellings
serek8 Mar 5, 2024
e0ba4e1
Update tests and examples
serek8 May 7, 2024
7d23021
Update the title of a static test
serek8 May 7, 2024
d2df0de
Update examples and fix spellings
serek8 May 7, 2024
cc126f0
Added rules from Olivier
serek8 May 7, 2024
3cb96d2
Apply suggestions from code review
serek8 Jun 3, 2024
dfb01ff
Rename Sample to Demo
serek8 Jun 3, 2024
badad99
Update demo-2 with a reversed manifest file
serek8 Jun 3, 2024
5e5c8dc
Mention iOS in Risks
serek8 Jun 4, 2024
ac00a36
Update Demos with the MASTestApp
serek8 Jun 4, 2024
44b625b
Update demo-1
serek8 Jun 4, 2024
bdcea87
Add a new demo and refactor existing demos
serek8 Jun 5, 2024
1d0dbb6
Add a demo with listing all files
serek8 Jun 5, 2024
9ad1169
Fix the spelling errors
serek8 Jun 5, 2024
e8fab33
fix md lint issues
cpholguera Jun 7, 2024
e8093a9
fix md lint issues
cpholguera Jun 7, 2024
d79b2d8
update rules to remove false positive separating manifest from apis. …
cpholguera Jun 15, 2024
e17a638
minor corrections in android-data-unencrypted-shared-storage-no-user-…
cpholguera Jun 15, 2024
e7c2902
merge demo-4 into demo-1
cpholguera Jun 21, 2024
769257c
updated kotlin samples to include a password-like and API key-like st…
cpholguera Jun 21, 2024
709bfd4
Minor update to the risk mitigations paragraph.
cpholguera Jun 21, 2024
77ddf31
Updated tests titles and consolidated content. Additional content reg…
cpholguera Jun 21, 2024
247a928
Consolidated tests sections and linked to relevant techniques.
cpholguera Jun 21, 2024
858b4bd
Consolidated demos sections and titles. Added more details to the obs…
cpholguera Jun 21, 2024
81610ae
Remove SARIF support for now
cpholguera Jun 21, 2024
665171e
Merge branch 'master' of https://github.com/OWASP/owasp-mastg into pr…
cpholguera Jun 22, 2024
252ac64
Merge branch 'master' of https://github.com/OWASP/owasp-mastg into pr…
cpholguera Jun 23, 2024
7f4809c
fix paths to snippets
cpholguera Jun 23, 2024
dff1834
added one CWE and android risk maaping, some additional clarification…
cpholguera Jun 24, 2024
f68d567
Merge branch 'master' of https://github.com/OWASP/owasp-mastg into pr…
cpholguera Jun 24, 2024
d61e867
fix links to tools and tech
cpholguera Jun 24, 2024
99d0776
Merge branch 'master' of https://github.com/OWASP/owasp-mastg into pr…
cpholguera Jun 24, 2024
e373dcc
rename risk to weakness
cpholguera Jun 24, 2024
d44da95
move all to the weaknesses folder
cpholguera Jun 24, 2024
b8ad87d
Merge branch 'master' of https://github.com/OWASP/owasp-mastg into pr…
cpholguera Jun 24, 2024
b582669
Merge branch 'master' of https://github.com/OWASP/owasp-mastg into pr…
cpholguera Jun 24, 2024
edeee04
include link to frida and remove ref to run.sh from test
cpholguera Jun 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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" \
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?

| 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.

Check failure on line 1 in risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-frida/example-1/snippet.kt

View workflow job for this annotation

GitHub Actions / codespell

verions ==> versions
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
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

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 use a variety of APIs to obtain a file path and store a file. Collecting a comprehensive list of these APIs can be challenging, especially if an app uses a third-party framework, loads code at runtime, or includes native code. The most effective approach to testing applications that write to device memory is usually dynamic analysis, and specifically method tracing.
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
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

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
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

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:

Check failure on line 28 in risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-static/example-1/example.md

View workflow job for this annotation

GitHub Actions / codespell

occurence ==> occurrence
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
Loading