-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Port MASTG-TEST-0025: Testing for Injection Flaws (android) (by @appknox) #3424
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?
Conversation
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.
Pull Request Overview
This PR ports MASTG-TEST-0025 to the new V2 format as MASTG-TEST-0288, focusing on SQL injection vulnerabilities in Android ContentProviders. The change deprecates the old test and introduces a new version with updated testing methodology.
- Deprecates MASTG-TEST-0025 and creates MASTG-TEST-0288 as its replacement
- Introduces a Semgrep rule to detect SQL injection patterns in ContentProvider implementations
- Adds comprehensive demo code showing vulnerable ContentProvider with SQL injection via URI path segments
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/android/MASVS-CODE/MASTG-TEST-0025.md | Marks the original test as deprecated with reference to new version |
| tests-beta/android/MASVS-CODE/MASTG-TEST-0288.md | New test specification for SQL injection in ContentProvider |
| rules/mastg-android-sql-injection-contentprovider.yml | Semgrep rule to detect unsafe URI path usage in SQL queries |
| demos/android/MASVS-CODE/MASTG-DEMO-0061/ | Complete demo implementation with vulnerable code and test execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
I left a couple comments in the files. Please have a look.
demos/android/MASVS-CODE/MASTG-DEMO-0061/MainActivityKt_reversed.java
Outdated
Show resolved
Hide resolved
| message: "Possible SQL Injection: Unvalidated user input from `Uri.getPathSegments()` used in `SQLiteQueryBuilder.appendWhere()" | ||
| patterns: | ||
| - pattern: | | ||
| $QB.appendWhere("..." + $VAR); |
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.
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.
This generates a lot of false positives, the vulnerability comes from how that text is constructed as its concatenation based unlike the path based used here.
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.
The test should cover all API that can lead to SQL injection. The title points at an overall issue too. Android doc mentions it here. Can you also add Best practices about using PreparedStatement
|
@serek8 , |
serek8
left a comment
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 still need to add best practices about Prepared Statements because they were present in MASTG-TEST-0025.md that we want to deprecate. We don't want to loose this information. Apart from that, let's fix the linter errors, everything else looks good.
|
@ScreaMy7 did you have some time to have a look at it? |
|
@serek8 I have added the changes. |
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
serek8
left a comment
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.
The content looks good. Please fix the building/linter issues.
|
The error is related dead links in other test files, not related with this PR. |
|
@ScreaMy7 can you please add |
| @@ -0,0 +1,23 @@ | |||
| --- | |||
| title: Prevent SQL Injection in ContentProviders | |||
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.
| title: Prevent SQL Injection in ContentProviders | |
| title: Prevent SQL Injection in Content Providers |
| @@ -0,0 +1,41 @@ | |||
| --- | |||
| platform: android | |||
| title: Injection flaws in Android Content providers | |||
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.
| title: Injection flaws in Android Content providers | |
| title: Injection Flaws in Android Content Providers |
| @@ -0,0 +1,83 @@ | |||
| --- | |||
| title: Testing Content URI Injection and Path-Segment Abuse | |||
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.
| title: Testing Content URI Injection and Path-Segment Abuse | |
| title: Content Providers URI Injection and Path-Segment Abuse |
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.
This needs a better title and potentially must be split in 2 techniques
| platform: android | ||
| id: MASTG-TEST-02XX | ||
| type: [static] | ||
| weakness: MASWE-0086 |
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.
Link MASTG-KNOW
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 There is no MASTG-KNOW related to this, should I create a new MASTG-KNOW.md file?
| title: Prevent SQL Injection in ContentProviders | ||
| alias: prevent-sqli-contentprovider | ||
| id: MASTG-BEST-XXXX | ||
| platform: android |
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.
Map to knowledge
closes #2999