Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Port MASTG-TEST-0022: Testing Custom Certificate Stores and Certificate Pinning (android) (by @guardsquare) #3035
base: master
Are you sure you want to change the base?
Port MASTG-TEST-0022: Testing Custom Certificate Stores and Certificate Pinning (android) (by @guardsquare) #3035
Changes from 3 commits
def3645
3ecfd1c
93bf241
1aa688c
8526097
06b395f
2adab0f
b7a3b0e
a20152f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is not completely true, the NSC only applies to connections done through the Android stack. Communication via low level APIs (e.g. from native code) do not take this into account.
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 changed this to:
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 need to split the tests according to the sections we defined in 0x05g.
For example, here we can create another static test for the custom Trust Manager case. Does the app use this approach and if it does, is it correct? For example: uses a Trust Manager that does nothing and therefore trusts everything.
You can open a ticket for that if you prefer, so we can get this PR merged soon and keep this test focused on the NSC way. Please update the content below to be NSC specific only.
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 created a task, this PR is getting quite big.
#3183
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.
Add a second dynamic test that uses Frida e.g. via objection or other scripts that try to bypass pinning in order to detect it. There are caveats of course as the original test indicates but it's useful.
See https://mas.owasp.org/MASTG/techniques/android/MASTG-TECH-0012/
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 not sure I understand what you mean. The link you mention explains how to deactivate pinning using Frida etc. How would that help identifying if pinning is done / done correctly?
What could be (somewhat) helpful, is to try the Frida scripts to see where it attaches, to then statically analyse those locations (relating to the to-be-created test for #3183).
Am I missing something?
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.
And monitor logcat for clues while doing this?
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 which clues do you have in mind?
The way I did this before was to just look at the intercepting proxy, logcat from the device never added anything too useful for this particular test.
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.
What about...?
We could add a new prerequisite and link to it here. As in https://github.com/OWASP/owasp-mastg/blob/master/prerequisites/identify-security-relevant-contexts.md
For example (REVIEW, AI generated):
Identifying Security-Sensitive Relevant Domains
What is a Security-Sensitive Relevant Domain?
A relevant domain is any endpoint where failing to enforce certificate pinning could expose sensitive data or compromise security. This typically includes:
What is NOT a Security-Sensitive Relevant Domain?
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 agree this would benefit a lot from a definition. Maybe sth to discuss in our next meeting, I don't think this is easy to define.
Imo, security-sensitive depends a lot on the app context, but also on the test / weakness itself. I could imagine certificate pinning being mandatory for L2 for specific domains, but it certainly shouldn't be mandatory for all domains for L1.
In the AI generated text, the 3rd party or analytics service is already not ideal imo. Whilst I agree with this for certificate pinning, I don't think analytics services are by default not security sensitive. E.g., thinking about GDPR, I can imagine personal identifiers being sent there, which need to be handled with care and are security sensitive...