-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Align TESTs 242-244 with iOS (Pinning) #3882
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
Draft
cpholguera
wants to merge
2
commits into
master
Choose a base branch
from
align-242-244-with-ios
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,25 +6,41 @@ type: [static, code] | |||||
| weakness: MASWE-0047 | ||||||
| profiles: [L2] | ||||||
| knowledge: [MASTG-KNOW-0014, MASTG-KNOW-0015] | ||||||
| prerequisites: | ||||||
| - identify-first-party-domains | ||||||
| --- | ||||||
|
|
||||||
| ## Overview | ||||||
|
|
||||||
| Apps can configure [certificate pinning using the Network Security Configuration](../../../knowledge/android/MASVS-NETWORK/MASTG-KNOW-0015.md#pinning-via-network-security-configuration-api-24). For each domain, one or multiple digests can be pinned. | ||||||
|
|
||||||
| The goal of this test is to check if the app does not implement certificate pinning using the NSC. However, note that the app may be using other pinning methods covered in other tests. | ||||||
| This test checks whether the app configures certificate pinning in the NSC for the relevant first-party domains it connects to. Relevant domains are remote endpoints under the developer's control that support the app's core or security-sensitive functionality. Third-party domains outside the developer's control **should not be reported** as missing pins only because they appear in app traffic. Note that the app may implement certificate pinning through other mechanisms covered in other tests. | ||||||
|
|
||||||
| ## Steps | ||||||
|
|
||||||
| 1. Use @MASTG-TECH-0013 to reverse engineer the app. | ||||||
| 2. Use @MASTG-TECH-0117 to obtain the AndroidManifest.xml | ||||||
| 3. Use @MASTG-TECH-0150 to check if a `networkSecurityConfig` is set in the `<application>` tag. | ||||||
| 4. Use @MASTG-TECH-0151 to extract all domains from `<domain-config>` that have a pin set (`<pin-set>`) from the Network Security Configuration file. | ||||||
| 5. Use @MASTG-TECH-0022 to identify the first-party domains the app connects to. | ||||||
|
|
||||||
| ## Observation | ||||||
|
|
||||||
| The output should contain a list of domains which enable certificate pinning. | ||||||
| The output should contain a list of domains that enable certificate pinning. The output should also identify any relevant first-party domains that were found in the app but do not have a pin set. | ||||||
|
|
||||||
| ## Evaluation | ||||||
|
|
||||||
| The test case fails if no `networkSecurityConfig` is set, or any relevant domain does not enable certificate pinning. | ||||||
| The test case fails if the app connects to relevant first-party domains but no `networkSecurityConfig` is set, or if `networkSecurityConfig` is set but does not enable certificate pinning for those domains. | ||||||
|
|
||||||
| The test case should not fail only because unrelated third-party domains are not pinned. | ||||||
|
|
||||||
| If another certificate pinning implementation is identified for the same domains, such as a custom `TrustManager` or a third-party library, the result should be treated as not covered by NSC pinning rather than as a confirmed absence of certificate pinning. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| **Further Validation Required:** | ||||||
|
|
||||||
| Before reporting a missing pin, confirm that the app actually establishes connections to the relevant first-party domains: | ||||||
|
|
||||||
| - Statically, follow the data references from the hardcoded URLs to the code that initiates the network connections (@MASTG-TECH-0023). | ||||||
| - Dynamically, capture and analyze the network traffic (@MASTG-TECH-0011) or hook the relevant network APIs at runtime to log the domains the app connects to. | ||||||
|
|
||||||
| Determining which domains are first-party and security-relevant typically requires information that is not present in the app binary and may require contact with the developers. | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 is NSC?
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.