Apply suggestions from code review

Co-authored-by: Carlos Holguera <[email protected]>

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-frida/example-1/example.md

@@ -7,7 +7,7 @@ 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 below to identify a potential data leak.
+The snippet below shows sample code that creates a file in external storage.
 
 {{ snippet.kt }}
 
@@ -21,10 +21,10 @@ The snippet below shows sample code that creates a file in external storage. You
 
 ### Observation
 
-There is only one file written to the external storage - `/storage/emulated/0/Android/data/com.gs.owasp_storage_app1/files/secret.json`. Make sure you don't store unencrypted data there unintentionally.
+There is only one file written to the external storage - `/storage/emulated/0/Android/data/org.owasp.mastestapp/files/secret.json`.
 
 {{ output.txt }}
 
 ### Evaluation
 
-Review each warning in the output file and make sure you intended to store this file in the external storage.
+This test fails since the file contains a password.

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-frida/example-1/snippet.kt

@@ -1,7 +1,22 @@
-// On lower Android versions, you might need to add `WRITE_EXTERNAL_STORAGE` permission to the manifest to write to an external app-specific directory.
+package org.owasp.mastestapp
+
+import android.content.Context
+import android.os.Environment
+import android.os.Environment.getExternalStoragePublicDirectory
+import java.io.File
+import java.io.FileOutputStream
+
+class MastgTest (private val context: Context){
+
+ fun mastgTest(): String {
+ val secret1 = "{\"password\":\"12345\"}\n"
+ val externalDirPath = getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS)
+ val file1 = File("$externalDirPath/secrets.json")
+ FileOutputStream(file1).use { fos ->
+ fos.write(secret1.toByteArray())
+ }
+
+ return "Secrets written to $file1:\n\n$secret1"
+ }
 
-val externalDirPath = getExternalFilesDir(null)!!.absolutePath
-val file: File = File("$externalDirPath/secret.json")
-FileOutputStream(file).use { fos ->
- fos.write("password:123".toByteArray())
 }

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-static/example-1/example.md

@@ -19,14 +19,11 @@ Let's run our semgrep rule against the sample code.
 
 ### 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 data there unintentionally.
+The rule has identified one location in the code file where a path to external storage is returned.
 
 {{ output.txt }}
 
 ### Evaluation
 
-Review each of the reported instances. In this case, it's only one instance. Line 9 shows the occurrence of API that returns external storage location. Make sure to either:
-- encrypt this file if necessary
-- move the file to the internal storage
-- keep the file in the same location if intended and secure
+Review the decompiled code at the location specified in the output (file and line number). This test fails because the file written by this instance contains sensitive data, specifically a password.
 

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-static/example-1/run.sh

@@ -1,2 +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
+NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./MastgTest_reversed.java --text -o output.txt
+NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./MastgTest_reversed.java --sarif -o output.sarif

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-static/example-2/run.sh

@@ -1,2 +1,2 @@
-NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./AndroidManifest.xml --text -o output.txt
-NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./AndroidManifest.xml --sarif -o output.sarif
+NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./AndroidManifest_reversed.xml --text -o output.txt
+NO_COLOR=true semgrep -c ../rules/mastg-android-data-unencrypted-external.yml ./AndroidManifest_reversed.xml --sarif -o output.sarif

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-static/rules/mastg-android-data-unencrypted-external.yml

@@ -2,19 +2,19 @@ rules:
  - id: mastg-android-data-unencrypted-external-api
  severity: WARNING
  languages:
- - kotlin
+ - java
  metadata:
  summary: This rule looks for methods that returns locations to "external storage" which is shared with other apps
  message: "[MASVS-STORAGE] Make sure to encrypt files at these locations if necessary"
  pattern-either:
- - pattern: getExternalFilesDir(...)
- - pattern: getExternalFilesDirs(...)
- - pattern: getExternalStorageDirectory(...)
- - pattern: getExternalStoragePublicDirectory(...)
- - pattern: getExternalCacheDir(...)
- - pattern: getExternalCacheDirs(...)
- - pattern: getExternalMediaDirs(...)
- - pattern: getDownloadCacheDirectory(...)
+ - pattern: $X.getExternalFilesDir(...)
+ - pattern: $X.getExternalFilesDirs(...)
+ - pattern: $X.getExternalStorageDirectory(...)
+ - pattern: $X.getExternalStoragePublicDirectory(...)
+ - pattern: $X.getExternalCacheDir(...)
+ - pattern: $X.getExternalCacheDirs(...)
+ - pattern: $X.getExternalMediaDirs(...)
+ - pattern: $X.getDownloadCacheDirectory(...)
  - pattern: Intent.ACTION_CREATE_DOCUMENT
  - id: mastg-android-data-unencrypted-external-manifest
  severity: WARNING
@@ -33,7 +33,7 @@ rules:
  - id: mastg-android-data-unencrypted-external-mediastore
  severity: WARNING
  languages:
- - generic
+ - java
  metadata:
  summary: This rule scans for uses of MediaStore API that writes data to the external storage. This data can be accessed by other apps.
  message: "[MASVS-STORAGE] Make sure to want this data to be shared with other apps"

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/android-data-unencrypted-external-static/test.md

@@ -7,23 +7,28 @@ type: [static]
 
 ## Overview
 
-This test searches for Manifest permissions and APIs that let your app write to locations which are shared with other apps. It means that a third-party app with a proper permissions may access data you write to these locations. Therefore, this test verifies whether an app:
-* declares permissions required to write data to shared locations
-* uses API to obtain location to shared locations 
-* uses MediaStore API
+This test looks for Android manifest permissions and APIs that allow an app to write to locations that are shared with other apps. This means that a third-party app with the proper permissions may be able to access data written to these locations. Therefore, this test verifies whether an app:
 
-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.
+- declares permissions required to write data to shared locations
+- uses API to obtain location to shared locations 
+- uses MediaStore API
+
+Additionally, if the "external storage" is actually stored externally, e.g. on an SD card, it can be removed from the device and inserted into a card reader to extract sensitive data.
 
 ### Testing Manifest permissions
 
 An app must declare in the Manifest file an intention to write to shared locations. Below you can find a list of such manifest permissions:
-* [WRITE_EXTERNAL_STORAGE](https://developer.android.com/reference/android/Manifest.permission#WRITE_EXTERNAL_STORAGE) - allows an app to write a file to the "external storage" which either resides on an external disk, or is internally emulated by the system. Regardless the actual storage origin, this permissions allows an app to write files to locations shared with other apps. This permission is deprecated starting from `target API 30` but can be preserved with [requestLegacyExternalStorage](https://developer.android.com/reference/android/R.attr#requestLegacyExternalStorage) and [preserveLegacyExternalStorage](https://developer.android.com/reference/android/R.attr#preserveLegacyExternalStorage). 
-* [MANAGE_EXTERNAL_STORAGE](https://developer.android.com/reference/android/Manifest.permission#MANAGE_EXTERNAL_STORAGE) - a successor permission of `WRITE_EXTERNAL_STORAGE`. It allows an an app to read and write files to shared locations.
+
+- [WRITE_EXTERNAL_STORAGE](https://developer.android.com/reference/android/Manifest.permission#WRITE_EXTERNAL_STORAGE): allows an app to write a file to the "external storage", regardless of the actual storage origin (external disk or internally emulated by the system).
+ - This permission is **deprecated since Android 11.0 (API level 30)** but can be preserved with [requestLegacyExternalStorage](https://developer.android.com/reference/android/R.attr#requestLegacyExternalStorage) and [preserveLegacyExternalStorage](https://developer.android.com/reference/android/R.attr#preserveLegacyExternalStorage).
+ - If the app declares a minSdkVersion of 19 or higher, you don't need to declare this permission to read and write files in your application-specific directories returned by [Context.getExternalFilesDir(String)](https://developer.android.com/reference/android/content/Context#getExternalFilesDir(java.lang.String)) and [Context.getExternalCacheDir()](https://developer.android.com/reference/android/content/Context#getExternalCacheDir()). 
+ - On Android 4.4 (API level 19) or higher, your app doesn't need to request any storage-related permissions to access app-specific directories within external storage. The files stored in these directories are removed when your app is uninstalled. See <https://developer.android.com/training/data-storage/app-specific#external>.
+ - On devices that run Android 9 (API level 28) or lower, your app can access the app-specific files that belong to other apps, provided that your app has the appropriate storage permissions. To give users more control over their files and to limit file clutter, apps that target Android 10 (API level 29) and higher are given scoped access into external storage, or [scoped storage](https://developer.android.com/training/data-storage#scoped-storage), by default. When scoped storage is enabled, apps cannot access the app-specific directories that belong to other apps.
+- [MANAGE_EXTERNAL_STORAGE](https://developer.android.com/reference/android/Manifest.permission#MANAGE_EXTERNAL_STORAGE): a successor permission of `WRITE_EXTERNAL_STORAGE`. It allows an an app to read and write files to shared locations special app access called ["All files access"](https://developer.android.com/preview/privacy/storage#all-files-access). This permission only applies to apps target Android 11.0 (API level 30) and its usage is restricted by Google Play unless the app satisfies [certain requirements](https://support.google.com/googleplay/android-developer/answer/10467955).
 
 ### Testing External Storage APIs
 
-There are APIs such as [[[getExternalStoragePublicDirectory]]](https://developer.android.com/reference/kotlin/android/os/Environment#getExternalStoragePublicDirectory(kotlin.String))
-that return paths to a shared locations that other apps can access. [Example 1](./example-1/example.md) illustrates a case where an app obtains a path to an "external" location and writes sensitive data to it. This location is Shared Storage Requiring No User Interaction, so a third-party app with proper permissions can read this sensitive data.
+There are APIs such as [`getExternalStoragePublicDirectory`](https://developer.android.com/reference/kotlin/android/os/Environment#getExternalStoragePublicDirectory(kotlin.String)) that return paths to a shared location that other apps can access. [Demo 1](demo-1/demo.md) illustrates a case where an app obtains a path to an "external" location and writes sensitive data to it. This location is Shared Storage Requiring No User Interaction, so a third-party app with proper permissions can read this sensitive data.
 
 #### External app-specific files
 
@@ -35,7 +40,7 @@ If your app stores data with MediaStore API, a third-party app with proper permi
 
 ## Steps
 
-1. Run a [static analysis](../../../../../techniques/android/MASTG-TECH-0014.md) tool on an app to find whether your app uses storage locations shared with other apps and pinpoint the invocations of these APIs. 
+1. Run a [static analysis](../../../../../techniques/android/MASTG-TECH-0014.md) tool on the app to find if it uses storage locations shared with other apps, and identify the calls to those APIs and the relevant permissions.
 
 
 ## Observation
@@ -48,5 +53,6 @@ Inspect app's source code using the provided information. The test case fails if
 
 ## References
 
-1. https://developer.android.com/training/data-storage/manage-all-files
-2. https://developer.android.com/training/data-storage/shared/media
+- [Manage all files on a storage device](https://developer.android.com/training/data-storage/manage-all-files)
+- [Access media files from shared storage](https://developer.android.com/training/data-storage/shared/media)
+

risks/MASVS-STORAGE/1-store-sensitive-data-securely/data-unencrypted-external/risk.md

@@ -1,6 +1,6 @@
 ---
-title: Sensitive Data Stored in Shared Storage Requiring No User Interaction
-alias: data-unencrypted-external
+title: Sensitive Data Stored Unencrypted in Shared Storage Requiring No User Interaction
+alias: data-unencrypted-shared-storage-no-user-interaction
 platform: [android]
 profiles: [L1, L2]
 mappings: