-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Port MASTG v1->v2 MASTG-TEST-0002: Testing Local Storage for Input Validation (android) (by @appknox) #3328
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?
Changes from 17 commits
86aa179
69ebf81
8c4060b
96a766f
3e1c45a
2a90252
d31713c
970f558
a323683
16acaa0
e1a0099
3bb04a9
bcaec04
2a4b407
fce6000
4ba84d9
c6b5232
a855378
f8639f8
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,32 @@ | ||
| --- | ||
| platform: android | ||
| title: Local Storage for Input Validation with semgrep | ||
| id: MASTG-DEMO-0061 | ||
| code: [kotlin] | ||
| test: MASTG-TEST-0288 | ||
| profiles: [L1, L2] | ||
| --- | ||
|
|
||
| ### Sample | ||
|
|
||
| The code snippet demonstrates the insecure use of `SharedPreferences`, as data is loaded without an integrity check. | ||
|
|
||
| {{ MastgTest.kt # MastgTest_reversed.java }} | ||
|
|
||
| ### Steps | ||
|
|
||
| Let's run @MASTG-TOOL-0110 rules against the sample code. | ||
|
|
||
| {{ ../../../../rules/mastg-android-local-storage-input-validation.yml }} | ||
|
|
||
| {{ run.sh }} | ||
|
|
||
| ### Observation | ||
|
|
||
| The rule identifies that data is being loaded without being validated. | ||
|
|
||
| {{ output.txt }} | ||
|
|
||
| ### Evaluation | ||
|
|
||
| The test fails as the code does not use an `HMAC` integrity check together with `SharedPreferences` data. | ||
|
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. Please describe how, because of this, an attacker can exploit the app. Use references to techniques when possible. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| package org.owasp.mastestapp | ||
|
|
||
| import android.content.Context | ||
| import android.content.SharedPreferences | ||
| import android.util.Log | ||
| import androidx.core.content.edit | ||
| import java.security.InvalidKeyException | ||
| import java.security.NoSuchAlgorithmException | ||
| import javax.crypto.Mac | ||
| import javax.crypto.spec.SecretKeySpec | ||
|
|
||
| /** | ||
| * This is the main test class that orchestrates the demonstration. | ||
| * It now contains all logic in a single class to simplify decompilation. | ||
| * It uses a two-step process to allow for manual tampering. | ||
| */ | ||
| class MastgTest(private val context: Context) { | ||
|
|
||
| companion object { | ||
| private const val PREFS_NAME = "app_settings" | ||
| private const val HMAC_ALGORITHM = "HmacSHA256" | ||
| // WARNING: In a real application, this key should NOT be hardcoded. | ||
| // It should be stored securely, for instance, in the Android Keystore. | ||
| // For this self-contained demo, we hardcode it to illustrate the HMAC mechanism. | ||
| private const val SECRET_KEY = "this-is-a-very-secret-key-for-the-demo" | ||
cpholguera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * Main test function that runs the setup or verification phase. | ||
| */ | ||
| fun mastgTest(): String { | ||
| val prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
|
|
||
| // Check if the initial setup has been performed. | ||
| if (!prefs.contains("setup_complete")) { | ||
| // --- FIRST-TIME EXECUTION: SETUP PHASE --- | ||
| // This block runs only once. | ||
|
|
||
| // 1. Set up the insecure preference (without HMAC). | ||
| saveData("user_role_insecure", "user", useHmac = false) | ||
|
|
||
| // 2. Set up the secure preference (with HMAC). | ||
| saveData("user_role_secure", "user", useHmac = true) | ||
|
|
||
| // 3. Mark setup as complete so this block doesn't run again. | ||
| prefs.edit(commit = true) { | ||
| putBoolean("setup_complete", true) | ||
| } | ||
|
|
||
| // 4. Return instructions for the user. | ||
| return "INITIAL SETUP COMPLETE.\n\n" + | ||
| "The role for both secure and insecure tests has been set to 'user'.\n\n" + | ||
| "ACTION REQUIRED:\n" + | ||
| "1. Use a file explorer or ADB shell on a rooted device.\n" + | ||
| "2. Go to: /data/data/org.owasp.mastestapp/shared_prefs/\n" + | ||
| "3. Open the file: app_settings.xml\n" + | ||
| "4. Change BOTH <string>user</string> values to <string>admin</string>.\n" + | ||
| "5. Save the file and run this test again to see the results." | ||
|
Comment on lines
+51
to
+58
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. Well done on he pen-testing example! 👏 I followed the steps but sadly could not "tamper" it. <?xml version='1.0' encoding='utf-8' standalone='yes' ?>
<map>
<string name="user_role_secure_hmac">690d026aeeba673e7225ee95a3460a0054752777afec8e4fb7ec8294934011ed</string>
<boolean name="setup_complete" value="true" />
<string name="user_role_insecure">admin</string>
<string name="user_role_secure">admin</string>
</map> is the file state between the 2 steps |
||
|
|
||
| } else { | ||
| // --- SUBSEQUENT EXECUTION: VERIFICATION PHASE --- | ||
| // This block runs after the user has tampered with the file. | ||
|
|
||
| val results = StringBuilder() | ||
|
|
||
| // 1. Verify the 'fail' case (insecure) | ||
| results.append("--- VERIFYING SCENARIO 1: 'kind: fail' (No HMAC Protection) ---\n") | ||
| val insecureRole = loadData("user_role_insecure", "error", useHmac = false) | ||
| results.append("Loaded role from 'user_role_insecure': '$insecureRole'\n") | ||
| if (insecureRole == "admin") { | ||
| results.append(">> OUTCOME: VULNERABLE. The application accepted the tampered 'admin' role because there was no integrity check.\n") | ||
| } else { | ||
| results.append(">> OUTCOME: NOT EXPLOITED. The role is still '$insecureRole'. Please ensure you changed it to 'admin' in the XML file.\n") | ||
| } | ||
|
|
||
| // 2. Verify the 'pass' case (secure) | ||
| results.append("\n--- VERIFYING SCENARIO 2: 'kind: pass' (HMAC Protection Enabled) ---\n") | ||
| val secureRole = loadData("user_role_secure", "tampering_detected", useHmac = true) | ||
| results.append("Loaded role from 'user_role_secure': '$secureRole'\n") | ||
| if (secureRole == "tampering_detected") { | ||
| results.append(">> OUTCOME: SECURE. The application detected that the data was tampered with and correctly rejected the invalid 'admin' role.\n") | ||
| } else if (secureRole == "admin") { | ||
| results.append(">> OUTCOME: UNEXPECTED. The role is 'admin', which means the HMAC check failed. This should not happen.\n") | ||
| } else { // secureRole == "user" | ||
| results.append(">> OUTCOME: NOT TAMPERED. The role is still '$secureRole', and its HMAC signature is valid.\n") | ||
| } | ||
|
|
||
| results.append("\n\nTest complete. To run the setup again, please clear the application's data in Android Settings and restart the test.") | ||
|
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. fyi restarting the application (thus test) would rewrite the xml. No need for clearing app data |
||
| return results.toString() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Saves a key-value pair. If HMAC is enabled, it also saves an integrity check value. | ||
| */ | ||
| private fun saveData(key: String, value: String, useHmac: Boolean) { | ||
| val prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| prefs.edit(commit = true) { | ||
| putString(key, value) | ||
| if (useHmac) { | ||
| val hmac = calculateHmac(value) | ||
| if (hmac != null) { | ||
| putString("${key}_hmac", hmac) | ||
| Log.d("MASTG-TEST", "Saved data with HMAC.") | ||
| } | ||
| } else { | ||
| Log.d("MASTG-TEST", "Saved data WITHOUT HMAC.") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Loads data for a given key. If HMAC is enabled, it first verifies the data's integrity. | ||
| */ | ||
| private fun loadData(key: String, defaultValue: String, useHmac: Boolean): String { | ||
| val prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) | ||
| val value = prefs.getString(key, null) ?: return defaultValue | ||
|
|
||
| if (!useHmac) { | ||
| Log.d("MASTG-TEST", "Loaded data without HMAC check. Value is: $value") | ||
| return value | ||
| } | ||
|
|
||
| val storedHmac = prefs.getString("${key}_hmac", null) | ||
| if (storedHmac == null) { | ||
| Log.w("MASTG-TEST", "HMAC verification failed: No HMAC found for key '$key'.") | ||
| return defaultValue | ||
| } | ||
|
|
||
| val calculatedHmac = calculateHmac(value) | ||
|
|
||
| return if (storedHmac == calculatedHmac) { | ||
| Log.d("MASTG-TEST", "HMAC verification SUCCESS. Value is: $value") | ||
| value | ||
| } else { | ||
| Log.e("MASTG-TEST", "HMAC verification FAILED! Data has been tampered with.") | ||
| defaultValue | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the HMAC for a given piece of data. | ||
| */ | ||
| private fun calculateHmac(data: String): String? { | ||
| return try { | ||
| val mac = Mac.getInstance(HMAC_ALGORITHM) | ||
| val secretKeySpec = SecretKeySpec(SECRET_KEY.toByteArray(), HMAC_ALGORITHM) | ||
| mac.init(secretKeySpec) | ||
| val hmacBytes = mac.doFinal(data.toByteArray()) | ||
| bytesToHex(hmacBytes) | ||
| } catch (e: NoSuchAlgorithmException) { | ||
| Log.e("MASTG-TEST", "HMAC algorithm not found", e) | ||
| null | ||
| } catch (e: InvalidKeyException) { | ||
| Log.e("MASTG-TEST", "Invalid HMAC key", e) | ||
| null | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Helper function to convert a byte array to a hexadecimal string. | ||
| */ | ||
| private fun bytesToHex(bytes: ByteArray): String { | ||
| val hexChars = "0123456789abcdef" | ||
| val result = StringBuilder(bytes.size * 2) | ||
| bytes.forEach { | ||
| val i = it.toInt() | ||
| result.append(hexChars[i shr 4 and 0x0f]) | ||
| result.append(hexChars[i and 0x0f]) | ||
| } | ||
| return result.toString() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||||||||||||
| package org.owasp.mastestapp; | ||||||||||||||
|
|
||||||||||||||
| import android.content.Context; | ||||||||||||||
| import android.content.SharedPreferences; | ||||||||||||||
| import kotlin.Metadata; | ||||||||||||||
| import kotlin.jvm.internal.Intrinsics; | ||||||||||||||
|
|
||||||||||||||
| /* compiled from: MastgTest.kt */ | ||||||||||||||
| @Metadata(d1 = {"\u0000\u0018\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0000\n\u0002\u0018\u0002\n\u0002\b\u0003\n\u0002\u0010\u000e\n\u0000\b\u0007\u0018\u00002\u00020\u0001B\u000f\u0012\u0006\u0010\u0002\u001a\u00020\u0003¢\u0006\u0004\b\u0004\u0010\u0005J\u0006\u0010\u0006\u001a\u00020\u0007R\u000e\u0010\u0002\u001a\u00020\u0003X\u0082\u0004¢\u0006\u0002\n\u0000¨\u0006\b"}, d2 = {"Lorg/owasp/mastestapp/MastgTest;", "", "context", "Landroid/content/Context;", "<init>", "(Landroid/content/Context;)V", "mastgTest", "", "app_debug"}, k = 1, mv = {2, 0, 0}, xi = 48) | ||||||||||||||
| /* loaded from: classes3.dex */ | ||||||||||||||
| public final class MastgTest_reversed { | ||||||||||||||
| public static final int $stable = 8; | ||||||||||||||
| private final Context context; | ||||||||||||||
|
|
||||||||||||||
| public MastgTest_reversed(Context context) { | ||||||||||||||
| Intrinsics.checkNotNullParameter(context, "context"); | ||||||||||||||
| this.context = context; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public final String mastgTest() { | ||||||||||||||
| SharedPreferences prefs = this.context.getSharedPreferences("app_settings", 0); | ||||||||||||||
| if (!prefs.contains("setup_complete")) { | ||||||||||||||
| SecureSharedPreferences insecurePrefs = new SecureSharedPreferences(this.context, false); | ||||||||||||||
| insecurePrefs.saveData("user_role_insecure", "user"); | ||||||||||||||
| SecureSharedPreferences securePrefs = new SecureSharedPreferences(this.context, true); | ||||||||||||||
| securePrefs.saveData("user_role_secure", "user"); | ||||||||||||||
|
Comment on lines
+23
to
+26
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. Extracting this file gives quite a different result. Most importantly, the following. Therefore, Semgrep finds no results for me.
Suggested change
|
||||||||||||||
| Intrinsics.checkNotNull(prefs); | ||||||||||||||
| SharedPreferences.Editor editor$iv = prefs.edit(); | ||||||||||||||
| editor$iv.putBoolean("setup_complete", true); | ||||||||||||||
| editor$iv.commit(); | ||||||||||||||
| return "INITIAL SETUP COMPLETE.\n\nThe role for both secure and insecure tests has been set to 'user'.\n\nACTION REQUIRED:\n1. Use a file explorer or ADB shell on a rooted device.\n2. Go to: /data/data/org.owasp.mastestapp/shared_prefs/\n3. Open the file: app_settings.xml\n4. Change BOTH <string>user</string> values to <string>admin</string>.\n5. Save the file and run this test again to see the results."; | ||||||||||||||
| } | ||||||||||||||
| StringBuilder results = new StringBuilder(); | ||||||||||||||
| results.append("--- VERIFYING SCENARIO 1: 'kind: fail' (No HMAC Protection) ---\n"); | ||||||||||||||
| SecureSharedPreferences insecurePrefs2 = new SecureSharedPreferences(this.context, false); | ||||||||||||||
| String insecureRole = insecurePrefs2.loadData("user_role_insecure", "error"); | ||||||||||||||
| results.append("Loaded role from 'user_role_insecure': '" + insecureRole + "'\n"); | ||||||||||||||
| if (Intrinsics.areEqual(insecureRole, "admin")) { | ||||||||||||||
| results.append(">> OUTCOME: VULNERABLE. The application accepted the tampered 'admin' role because there was no integrity check.\n"); | ||||||||||||||
| } else { | ||||||||||||||
| results.append(">> OUTCOME: NOT EXPLOITED. The role is still '" + insecureRole + "'. Please ensure you changed it to 'admin' in the XML file.\n"); | ||||||||||||||
| } | ||||||||||||||
| results.append("\n--- VERIFYING SCENARIO 2: 'kind: pass' (HMAC Protection Enabled) ---\n"); | ||||||||||||||
| SecureSharedPreferences securePrefs2 = new SecureSharedPreferences(this.context, true); | ||||||||||||||
| String secureRole = securePrefs2.loadData("user_role_secure", "tampering_detected"); | ||||||||||||||
| results.append("Loaded role from 'user_role_secure': '" + secureRole + "'\n"); | ||||||||||||||
| if (Intrinsics.areEqual(secureRole, "tampering_detected")) { | ||||||||||||||
| results.append(">> OUTCOME: SECURE. The application detected that the data was tampered with and correctly rejected the invalid 'admin' role.\n"); | ||||||||||||||
| } else if (Intrinsics.areEqual(secureRole, "admin")) { | ||||||||||||||
| results.append(">> OUTCOME: UNEXPECTED. The role is 'admin', which means the HMAC check failed. This should not happen.\n"); | ||||||||||||||
| } else { | ||||||||||||||
| results.append(">> OUTCOME: NOT TAMPERED. The role is still '" + secureRole + "', and its HMAC signature is valid.\n"); | ||||||||||||||
| } | ||||||||||||||
| results.append("\n\nTest complete. To run the setup again, please clear the application's data in Android Settings and restart the test."); | ||||||||||||||
| String sb = results.toString(); | ||||||||||||||
| Intrinsics.checkNotNullExpressionValue(sb, "toString(...)"); | ||||||||||||||
| return sb; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
|
|
||
|
|
||
| ┌─────────────────┐ | ||
| │ 2 Code Findings │ | ||
| └─────────────────┘ | ||
|
|
||
| MastgTest_reversed.java | ||
| ❯❱rules.mastg-android-local-storage-input-validation | ||
| [MASVS-CODE-4] The application reads data from SharedPreferences without | ||
| an integrity check (like HMAC). This data could be tampered with by an | ||
| attacker on a rooted device, leading to privilege escalation or other | ||
| vulnerabilities. | ||
|
|
||
| 23┆ SecureSharedPreferences insecurePrefs = new | ||
| SecureSharedPreferences(this.context, false); | ||
| ⋮┆---------------------------------------- | ||
| 35┆ SecureSharedPreferences insecurePrefs2 = new | ||
| SecureSharedPreferences(this.context, false); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1 @@ | ||||||
| NO_COLOR=true semgrep -c ../../../../rules/mastg-android-local-storage-input-validation.yml ./MastgTest_reversed.java --text -o output.txt | ||||||
|
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
to align with rest of project |
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| rules: | ||
| - id: mastg-android-local-storage-input-validation | ||
| severity: WARNING | ||
| languages: | ||
| - java | ||
| metadata: | ||
| summary: Detects SharedPreferences usage without an integrity check. | ||
| message: "[MASVS-CODE-4] The application reads data from SharedPreferences without an integrity check (like HMAC). This data could be tampered with by an attacker on a rooted device, leading to privilege escalation or other vulnerabilities." | ||
| patterns: | ||
| - pattern: new SecureSharedPreferences($CONTEXT, false) | ||
|
|
||
cpholguera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||
| --- | ||||||||||
| title: Use of Local Storage for Input Validation | ||||||||||
|
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. How about the following? Input validation to me sounds validating input rather than checking the integrity/authenticity of data.
Suggested change
or
Suggested change
|
||||||||||
| platform: android | ||||||||||
| id: MASTG-TEST-0288 | ||||||||||
|
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. you might need a new id just before merge (test 288 already exists) |
||||||||||
| type: [static] | ||||||||||
| weakness: MASWE-0082 | ||||||||||
| profiles: [L1, L2] | ||||||||||
| --- | ||||||||||
|
|
||||||||||
| ## Overview | ||||||||||
|
|
||||||||||
| Data stored in Android's `SharedPreference`s can be tampered with on a rooted device. If an application reads this data without verifying its integrity (e.g., with an HMAC signature), it can lead to security vulnerabilities. This test checks if the application properly validates data read from local 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. This is already really well-written @jeel38! How about we generalise it a bit and add a note, comment, or intro that this test is not only about SharedPrefs, but also any other local storage, like the v1 test mentions? Additionally, we should add the requirement for data authenticity (HMAC is about both integrity and authenticity after all):
Suggested change
What we could highlight here to help the reader is that SHA (integrity check without authenticity check) might not be optimal unless the SHA hashes are stored in a backend server. Therefore, by using HMAC, we only need to store the key safely and the data + HMAC hash in the local storage. |
||||||||||
|
|
||||||||||
| ## Steps | ||||||||||
|
|
||||||||||
| 1. Run a static analysis tool such as @MASTG-TOOL-0110 on the code and look for patterns where data is read from `SharedPreferences` without a corresponding integrity check. | ||||||||||
|
|
||||||||||
| ## Observation | ||||||||||
|
|
||||||||||
| The output identifies code where `SharedPreferences` data is loaded without an integrity check. | ||||||||||
|
|
||||||||||
| ## Evaluation | ||||||||||
|
|
||||||||||
| The test fails if the application reads data from `SharedPreferences` without verifying its integrity using a mechanism like `HMAC`. | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,9 @@ masvs_v1_levels: | |||||
| - L1 | ||||||
| - L2 | ||||||
| profiles: [L1, L2] | ||||||
| status: deprecated | ||||||
| covered_by: [MASTG-TEST-0281] | ||||||
|
||||||
| covered_by: [MASTG-TEST-0281] | |
| covered_by: [MASTG-TEST-0288] |
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.
you might need a new id just before merge (demo 61 exists)