Port MASTG-TEST-0034: Testing Object Persistence (android) (by @appknox)#3672
Port MASTG-TEST-0034: Testing Object Persistence (android) (by @appknox)#3672sk3l10x1ng wants to merge 12 commits intoOWASP:masterfrom
Conversation
|
@cpholguera @serek8 , please review. The request changes in #3672 are addressed here |
There was a problem hiding this comment.
Pull request overview
Ports the legacy Android test MASTG-TEST-0034 to a new v2 test format, deprecates the v1 test, and adds a semgrep rule + demo material intended to support the new test.
Changes:
- Deprecates
tests/android/MASVS-CODE/MASTG-TEST-0034.mdand links it to a v2 replacement. - Adds a new v2 test file under
tests-beta/android/MASVS-CODE/. - Adds a semgrep rule and a corresponding Android demo (sample code, reversed code, run script, and output).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/android/MASVS-CODE/MASTG-TEST-0034.md | Marks the v1 test as deprecated and points to a v2 replacement. |
| tests-beta/android/MASVS-CODE/MASTG-TEST-0x34.md | Introduces the proposed v2 test content for the port. |
| rules/mastg-android-object-deserialization.yml | Adds a semgrep rule intended to detect object deserialization usage. |
| demos/android/MASVS-CODE/MASTG-DEMO-0x34/MASTG-DEMO-0x34.md | Adds demo documentation describing how to run/interpret the rule. |
| demos/android/MASVS-CODE/MASTG-DEMO-0x34/MastgTest.kt | Adds demo Kotlin sample implementing the insecure pattern. |
| demos/android/MASVS-CODE/MASTG-DEMO-0x34/MastgTest_reversed.java | Adds reversed Java code used for static analysis in the demo. |
| demos/android/MASVS-CODE/MASTG-DEMO-0x34/run.sh | Adds a script to run semgrep for the demo. |
| demos/android/MASVS-CODE/MASTG-DEMO-0x34/output.txt | Adds captured semgrep output used in the demo documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Overview | ||
|
|
||
| If the app deserializes untrusted data without sufficient validation, it becomes vulnerable to malicious object injection. In Android, data can be passed between components via Intent objects. When an application receives a serialized object within an Intent and deserializes it using `ObjectInputStream.readObject()` without type filtering, a malicious application can send a specially crafted Intent containing a malicious serialized object. This can lead to arbitrary code execution, data tampering, privilege escalation, or denial of service depending on the available gadget chains in the application's classpath. | ||
|
|
There was a problem hiding this comment.
The new v2 test content focuses on untrusted object deserialization / object injection via ObjectInputStream.readObject() (e.g., from Intent extras or network input), but the deprecated v1 test "Testing Object Persistence" focuses on persistence/storage of serialized objects (Serializable/JSON/ORM) and protecting sensitive data at rest. This looks like a scope change from the PR title/issue; please align the v2 test with the v1 intent (object persistence of sensitive data) or update the PR/issue mapping accordingly.
There was a problem hiding this comment.
This was as agreed. Very nice that it noticed it though!!
| fun mastgTest(): String { | ||
| val user = UserManager.currentUser | ||
| val status = if (user is AdminUser && user.isAdmin) { | ||
| "PRIVILEGED ADMIN!" | ||
| } else { |
There was a problem hiding this comment.
To make the demo self-describing and easier to validate, add a short SUMMARY: comment and explicit FAIL:/PASS: annotations tied to the relevant test/rule near the vulnerable lines (this convention is used by other demos, e.g., demos/android/MASVS-CRYPTO/MASTG-DEMO-0008/MastgTest.kt). Right now the sample demonstrates the behavior but is missing those expected-outcome markers.
| patterns: | ||
| - pattern: | ||
| $VAR = new java.io.ObjectInputStream(...); | ||
| $OBJ = $VAR.readObject(); No newline at end of file |
There was a problem hiding this comment.
The pattern value is multi-line but isn’t using a block scalar (pattern: |/|-). Other multi-line semgrep rules in this repo do (e.g., rules/mastg-android-device-passcode-present.yml), which avoids YAML newline folding. Also, the current pattern requires the readObject() call to be directly adjacent to the ObjectInputStream construction; consider adding ... between statements so common real-world variants still match.
2 participants
closes #3000
changes in PR #3418 & #3259 addressed here