Port MASTG-TEST-0034: Testing Object Persistence (android) (by @appknox)#3418
Port MASTG-TEST-0034: Testing Object Persistence (android) (by @appknox)#3418sk3l10x1ng wants to merge 22 commits intoOWASP:masterfrom
Conversation
|
@cpholguera please check |
There was a problem hiding this comment.
Pull Request Overview
This PR ports MASTG-TEST-0034 (Testing Object Persistence) to a new version MASTG-TEST-0287 (Unwanted Object Deserialization Using Serializable) as part of MASTG V2 migration. The original test is marked as deprecated and replaced with an updated version that focuses specifically on insecure deserialization vulnerabilities in Android applications.
Key changes:
- Deprecates the original MASTG-TEST-0034 with proper metadata linking to the new version
- Creates new test MASTG-TEST-0287 with updated content focusing on deserialization security issues
- Adds comprehensive demo materials including Kotlin/Java code samples and semgrep rules
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/android/MASVS-CODE/MASTG-TEST-0034.md | Adds deprecation metadata linking to new test version |
| tests-beta/android/MASVS-CODE/MASTG-TEST-0287.md | New test definition focusing on insecure deserialization vulnerabilities |
| rules/mastg-android-object-deserialization.yml | Semgrep rule to detect ObjectInputStream.readObject() usage patterns |
| demos/android/MASVS-CODE/MASTG-DEMO-0061/ | Complete demo package with vulnerable code samples, analysis tools, and expected outputs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@serek8 please review |
serek8
left a comment
There was a problem hiding this comment.
Great work! It adds new knowledge to our guide. I would just make sure we don't lose anything from MASTG-TEST-0034.md we want to deprecate. E.g. the old test describes that a serialized objects might contain sensitive data. Let's add tests and demos for these.
Also let's make sure to keep HMAC/encryption for this:
There are a few generic remediation steps that you can always take:
Make sure that sensitive data has been encrypted and HMACed/signed after serialization/persistence. Evaluate the signature or HMAC before you use the data. See the chapter "Android Cryptographic APIs" for more details.
Make sure that the keys used in step 1 can't be extracted easily. The user and/or application instance should be properly authenticated/authorized to obtain the keys. See the chapter "Data Storage on Android" for more details.
Make sure that the data within the de-serialized object is carefully validated before it is actively used (e.g., no exploit of business/application logic).
|
@serek8 The above demo is created based on carlos suggestion #3259 (comment) , the focus is on what actually matters in the context of this MASVS-CODE control is the risk of unsafe deserialization. |
|
Correct, let's continue that on a new PR for #3646 |
|
|
||
| MastgTest_reversed.java | ||
| ❯❱ rules.mastg-android-object-deserialization | ||
| [MASVS-CODE-4] The application make use of Object deserialization in the code. |
There was a problem hiding this comment.
| [MASVS-CODE-4] The application make use of Object deserialization in the code. | |
| [MASVS-CODE-4] The application makes use of Object deserialization in the code. |
|
|
||
| ## Observation | ||
|
|
||
| The output file shows usages of the object Deserialization using `readObject()` in the code. |
There was a problem hiding this comment.
Not according to the guidelines. This is a test, not a demo.
|
|
||
| ## Steps | ||
|
|
||
| 1. Run a static analysis tool such as @MASTG-TOOL-0110 on the codebase for usages of `readObject()`. |
There was a problem hiding this comment.
This sounds like testing for that method is enough, but it's not. This must be generalized. See other tests for reference.
|
|
||
| ## Evaluation | ||
|
|
||
| The test fails due to the application deserializing data from an untrusted `Intent` extra through the insecure `ObjectInputStream.readObject()` method. A malicious application can create a serialized `AdminUser` object, transmit it via an Intent, and have it deserialized by the processIntent method. This action would overwrite the current user and provide the attacker with administrative privileges within the application. |
There was a problem hiding this comment.
This also reads like a demo and it's not compliant:
|
|
||
| ### Evaluation | ||
|
|
||
| The test fails because `readObject()` method was found in the code. |
There was a problem hiding this comment.
You need to be more specific here. This cannot be the sole reason for failing. Also, this isn't aligned with the test evaluation.
The output has uses of readObject but you need to do something to really know if the app is vulnerable, right? You look at the reverse engineered code and ...?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Evaluation | ||
|
|
||
| The test fails due to the application deserializing data from an untrusted `Intent` extra through the insecure `ObjectInputStream.readObject()` method. A malicious application can create a serialized `AdminUser` object, transmit it via an Intent, and have it deserialized by the processIntent method. This action would overwrite the current user and provide the attacker with administrative privileges within the application. |
There was a problem hiding this comment.
The method name "processIntent" should be formatted as inline code using backticks for consistency with other method references in the text.
| The test fails due to the application deserializing data from an untrusted `Intent` extra through the insecure `ObjectInputStream.readObject()` method. A malicious application can create a serialized `AdminUser` object, transmit it via an Intent, and have it deserialized by the processIntent method. This action would overwrite the current user and provide the attacker with administrative privileges within the application. | |
| The test fails due to the application deserializing data from an untrusted `Intent` extra through the insecure `ObjectInputStream.readObject()` method. A malicious application can create a serialized `AdminUser` object, transmit it via an Intent, and have it deserialized by the `processIntent` method. This action would overwrite the current user and provide the attacker with administrative privileges within the application. |
|
|
||
| ### Sample | ||
|
|
||
| The code snippet shows the utilization of object deserialization using `readObject()` method. |
There was a problem hiding this comment.
The article "the" is missing before "method". It should read "using the readObject() method".
|
|
||
| ### Evaluation | ||
|
|
||
| The test fails because `readObject()` method was found in the code. |
There was a problem hiding this comment.
The article "the" is missing before "method". It should read "because the readObject() method was found".
| languages: | ||
| - java | ||
| metadata: | ||
| summary: This rule looks for use of Object Serialization |
There was a problem hiding this comment.
The summary states "This rule looks for use of Object Serialization" but the rule actually detects deserialization (using readObject()), not serialization. The summary should say "Object Deserialization" instead of "Object Serialization".
| summary: This rule looks for use of Object Serialization | |
| summary: This rule looks for use of Object Deserialization |
| id: MASTG-DEMO-0x34 | ||
| code: [kotlin] | ||
| test: MASTG-TEST-0x34 | ||
| profiles: [L1, L2] |
There was a problem hiding this comment.
The "profiles" field in the front matter is not typically included in demo files. Based on other demos in the repository (e.g., MASTG-DEMO-0001, MASTG-DEMO-0025, MASTG-DEMO-0050), demos do not include this field. Consider removing it to maintain consistency with the codebase conventions.
|
|
||
| MastgTest_reversed.java | ||
| ❯❱ rules.mastg-android-object-deserialization | ||
| [MASVS-CODE-4] The application make use of Object deserialization in the code. |
There was a problem hiding this comment.
The message has a grammatical error. It should be "makes" instead of "make" to agree with the singular subject "application". This appears to be a discrepancy with the rule file which correctly uses "makes".
| [MASVS-CODE-4] The application make use of Object deserialization in the code. | |
| [MASVS-CODE-4] The application makes use of Object deserialization in the code. |
|
|
||
| ## Overview | ||
|
|
||
| Insecure Deserialization is a vulnerability that occurs when an application deserializes untrusted data without sufficient validation. In Android, data can be passed between components via Intent objects. If an application receives a serialized object within an Intent and deserializes it using an unsafe method like `ObjectInputStream.readObject()`, it becomes vulnerable. A malicious application could send a specially crafted Intent containing a serialized object. When the vulnerable app deserializes this object, it can lead to arbitrary code execution, data tampering, or denial of service. In this testcase, it allows for privilege escalation by overwriting the current user's state. |
There was a problem hiding this comment.
The term "testcase" should be written as two separate words: "test case".
| Insecure Deserialization is a vulnerability that occurs when an application deserializes untrusted data without sufficient validation. In Android, data can be passed between components via Intent objects. If an application receives a serialized object within an Intent and deserializes it using an unsafe method like `ObjectInputStream.readObject()`, it becomes vulnerable. A malicious application could send a specially crafted Intent containing a serialized object. When the vulnerable app deserializes this object, it can lead to arbitrary code execution, data tampering, or denial of service. In this testcase, it allows for privilege escalation by overwriting the current user's state. | |
| Insecure Deserialization is a vulnerability that occurs when an application deserializes untrusted data without sufficient validation. In Android, data can be passed between components via Intent objects. If an application receives a serialized object within an Intent and deserializes it using an unsafe method like `ObjectInputStream.readObject()`, it becomes vulnerable. A malicious application could send a specially crafted Intent containing a serialized object. When the vulnerable app deserializes this object, it can lead to arbitrary code execution, data tampering, or denial of service. In this test case, it allows for privilege escalation by overwriting the current user's state. |
|
|
||
| ## Observation | ||
|
|
||
| The output file shows usages of the object Deserialization using `readObject()` in the code. |
There was a problem hiding this comment.
The word "Deserialization" should not be capitalized here as it's not at the beginning of a sentence or a proper noun.
| The output file shows usages of the object Deserialization using `readObject()` in the code. | |
| The output file shows usages of the object deserialization using `readObject()` in the code. |
* Improve moderator output * Add contributor
…thors who are collaborators (OWASP#3666)
* Add Issue Assignment Bot workflow to manage assignment requests and confirmations * Apply suggestion from @TheDauntless Co-authored-by: Jeroen Beckers <info@dauntless.be> * Fix marker output in acceptance branch to prevent false positives (OWASP#3665) * Initial plan * Fix marker output in acceptance branch to prevent false positives Co-authored-by: cpholguera <29175115+cpholguera@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cpholguera <29175115+cpholguera@users.noreply.github.com> --------- Co-authored-by: Jeroen Beckers <info@dauntless.be> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
* Initial plan * Add Frida Handbook references to MASTG tools - Add comprehensive "Frida Handbook" section to MASTG-TOOL-0031 - Add Stalker tutorial reference in Frida tool page - Add r2frida tutorial reference in r2frida tool page Co-authored-by: cpholguera <29175115+cpholguera@users.noreply.github.com> * Add comprehensive Frida Handbook links across MASTG techniques and tools - Add hooking references for Android and iOS method hooking techniques - Add Gadget mode references for non-rooted/jailbroken device testing - Add frida-trace references for native code and method tracing - Add platform-specific references to Android and iOS Frida tools Co-authored-by: cpholguera <29175115+cpholguera@users.noreply.github.com> * Fix Frida Handbook URLs to use correct section anchors - Update hooking link to use basic_usage/#hooks-and-the-interceptor-api/ - Remove unverifiable links for /modes/, /android/, and /ios/ paths - Keep working frida-trace links that use basic_usage/#frida-trace pattern Co-authored-by: cpholguera <29175115+cpholguera@users.noreply.github.com> * Add references to Frida Handbook for Android and iOS practical examples --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cpholguera <29175115+cpholguera@users.noreply.github.com> Co-authored-by: Carlos Holguera <perezholguera@gmail.com>
* Fix markdown table formatting (MD060)
Standardize table column separators to use consistent `| --- | --- |`
format and add proper spacing around table pipes to comply with
markdownlint MD060 rule.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Fix heading level increments (MD001)
Change heading levels in demo files and techniques to comply with
MD001 rule requiring headings to increment by one level at a time.
- Demos: ### Sample/Steps/Observation/Evaluation → ##
- MASTG-DEMO-0059: #### Option 1/2 → ### Option 1/2
- Various techniques: ### → ## where appropriate
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Replace non-descriptive link text (MD059)
Replace generic [here] links with descriptive text to improve
accessibility and comply with MD059 rule.
Examples:
- [here] → [the project wiki]
- [here] → [in the official documentation]
- [here] → [in Corellium's documentation]
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Fix curly apostrophes in instructions and CODE_OF_CONDUCT
Replace curly single quotes (') with straight apostrophes (')
to comply with markdownlint curly-single-quotes rule.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Remove trailing commas
* Fix table
* Fix 2 wrong urls
* Fix examples
* Make all tables compact style
https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md#md060---table-column-style
---------
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Carlos Holguera <perezholguera@gmail.com>
* Refine documentation for iOS security testing techniques * Refine documentation for iOS security testing techniques * Refine documentation for iOS security testing techniques * Remove em-dashes * Fix copilot feedback * Fix ios-deploy name --------- Co-authored-by: Carlos Holguera <perezholguera@gmail.com>
* Initial frooky addition and migration * Use JSON format and rename files * Change DEMOs to use JSON format * Add wrappers for running Frooky on Android and iOS demos in a centralized directory. In that directory the nodejs modules would be generated * Inline frooky * 0058 output * 0058 output formatted * DEMO-0081 * DEMO-0059 * DEMO-0060 * Update instruction * Add frooky in instruction * Add frooky in requirements.txt * Apply suggestions from code review * Apply suggestion from @cpholguera * Remove output file argument from run scripts for Android demos which is output.json by default --------- Co-authored-by: Carlos Holguera <perezholguera@gmail.com>
* Fix instructions.md to be checked with our own lint rules * Use MAS-LINT- as rule prefix * Fix md lint issues --------- Co-authored-by: Carlos Holguera <perezholguera@gmail.com>
* Refine documentation for Android KeyStore and Key Attestation, improving clarity and consistency. Add https://www.android-device-security.org/database/ * end lines * Apply suggestion from @cpholguera --------- Co-authored-by: Carlos Holguera <perezholguera@gmail.com>
* Revise code samples and dynamic analysis instructions Updated code sample references and instructions for running test apps. Modified examples for dynamic analysis tools to use the correct application package names. * Apply suggestion from @cpholguera
* Improved the existing instruction files: - Introduced a new index.md file with guidelines for writing various MASTG content types, including tests, demos, knowledge articles, techniques, tools, and best practices. - Updated markdown.instructions.md to correct the weakness identifier example and improve the description. - Enhanced all mastg-xxxx.instructions.md with metadata to be used as instructions for certain areas. * Add metadata header to Frooky hook configurations guide * fix md lint * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tool metadata to use 'hosts' key for operating systems * Update instruction files to replace 'description' with 'name' for consistency * Add anything missing from the style guide * Apply suggestions from code review * fix urls * Update instruction files to replace 'name' with 'title' for consistency --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@cpholguera @serek8 , not sure other commit got included. I've addressed the changes in New PR #3672 |
closes #3000