-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Port MASTG-TEST-0034: Testing Object Persistence (android) (by @appknox) #3259
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
Conversation
@cpholguera please review |
@@ -0,0 +1,23 @@ | |||
--- | |||
title: Use of Object Persistence using JSON |
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.
We shouldn't simply test for the use of JSONObject
and JSONArray
as this isn't a vulnerability in itself. While it's true that JSON may hold sensitive data, its presence alone is not indicative of a security issue. What actually matters in the context of this MASVS-CODE control is the risk of unsafe deserialization, where attacker-controlled input is used to reconstruct objects, potentially triggering unintended behavior or code execution.
If you check the related MASWE-0088: Insecure Object Deserialization
you'll see that we corrected the scope there when we created it.
This is a common and well-documented threat in Android, especially when using custom classes with Serializable
, Parcelable
, or reflection-based deserialization.
The current proposal in this PR seems to deal with other concerns like data persistence or secure storage. These concerns are already covered under MASVS-STORAGE and MASVS-CRYPTO, so this test should focus specifically on identifying and mitigating deserialization risks in the app logic.
I totally see how this can be confusing when reading the original test (MASTG-TEST-0034). I went thought it now, took the relevant excerpts for you and explained why it relates to unsafe deserialization:
- "First identify all instances of object serialization and check if they carry any sensitive data. If yes, check if it is properly protected against eavesdropping or unauthorized modification."
This relates to unsafe deserialization because serialized data can be intercepted or modified by attackers, leading to code execution or data tampering if deserialized without validation.
- "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)."
This is relevant because lack of validation of deserialized data may trigger unintended behavior or exploit logic flaws in the application.
- "Use
Serializable
only when the serialized classes are stable... not using reflection-based persistence..."
Reflection-based deserialization combined with Serializable
can allow attackers to manipulate inputs and trigger arbitrary code paths, a classic unsafe deserialization vector.
- "Search the source code for...
import java.io.Serializable
,implements Serializable
"
This indicates that the app is using Java serialization, which could enable a deserialization attack when handling untrusted data.
See "Risk: Unwanted Object Deserialization".
- "Use @MASTG-TOOL-0001 (Frida) to hook into the deserialization methods..."
Monitoring how objects are deserialized may help detecting unsafe behavior, such as the use of untrusted input to reconstruct objects. While relevant, I think you can skip this part of the test (the dynamic testing) for now and focus on static.
- "Make sure that appropriate security measures are taken when sensitive information is stored in an Intent via a Bundle that contains a Parcelable..."
Parcelables are Android’s custom serialization mechanism. If used without proper access control, attackers can inject malicious objects through IPC and exploit unsafe deserialization.
See Risk: Deserialization of untrusted input
Suggestion
- Use Risk: Deserialization of untrusted input as a reference to create a test called e.g. "Unsafe Deserialization of Untrusted Input Using Parcelable" and related demo.
- Use "Risk: Unwanted Object Deserialization" as a reference to create a test called e.g. "Unwanted Object Deserialization Using Serializable" and related demo.
@cpholguera please review it |
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.
If you're performing a pentest or developing an automated tool for mobile app security testing, would you blindly report all apps that use Serializable/Parcelable
classes?
See the Suggestion section at #3259 (comment).
The two links from Android Risks contain more specific cases that we should address. The test should explain to the reader what is important when testing for unsafe deserialization in these cases, as well as how to do so. Simply reporting the use of a class is not enough. If manual work is required, this should also be explained.
The demos must contain actual unsafe implementations, not just the use of classes.
|
||
class MastgTest(private val context: Context) { | ||
|
||
// Custom Parcelable class that can be abused |
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.
How exactly can this be abused? This isn't explained in the demo. The presence of the classes is not enough for the app to be considered vulnerable.
https://owasp.org/www-community/vulnerabilities/Deserialization_of_untrusted_data
|
@cpholguera , we have understood it and are currently implementing the necessary changes. We will push the updates as soon as possible. |
Perfect, thanks a lot @sk3l10x1ng ! |
updated Object Deserialization Serializable demo
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.
Not reviewing yet but @sk3l10x1ng please be careful not to push these files to the repo. See this one and the rest of .gradle/ below. Thanks!
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.
@cpholguera I'm uncertain about how this files was included; I will create a new, clean pull request. Thank you
closes #3000