-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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?
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.
Added comment to NSC config
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.
Thank you for the new test cases and the PR @titze! Please check the suggestions below.
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.
Hi @titze there's a problem with the PR, it now affects 240 files. Could you please recheck/fix the merging of the main branch so that only your changes are showing?
5eb14dd
to
def3645
Compare
Fixed. I messed up the rebase on master... |
No problem, thanks so much for the fix! |
|
||
#### Pinning via Network Security Configuration (API 24+) | ||
|
||
The Network Security Configuration can be used to pin [declarative certificates](https://developer.android.com/training/articles/security-config.html#CertificatePinning) to specific domains. This is done by providing a `<pin-set>` in the Network Security Configuration, which is a set of digests (hashes) of the public key (`SubjectPublicKeyInfo`) of the corresponding X.509 certificate. |
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 Network Security Configuration can be used to pin [declarative certificates](https://developer.android.com/training/articles/security-config.html#CertificatePinning) to specific domains. This is done by providing a `<pin-set>` in the Network Security Configuration, which is a set of digests (hashes) of the public key (`SubjectPublicKeyInfo`) of the corresponding X.509 certificate. | |
The **Network Security Configuration (NSC)** is the preferred and recommended way to implement certificate pinning in Android, as it provides a declarative, maintainable, and secure approach without requiring code changes. It applies to all network traffic within the app, including `HttpsURLConnection`-based connections and `WebView` requests (unless a custom `TrustManager` is used). | |
To use [NSC for pinning](https://developer.android.com/training/articles/security-config.html#CertificatePinning), a `<pin-set>` is defined in the `network_security_config.xml` file. This `<pin-set>` contains the **hashes of the public key (`SubjectPublicKeyInfo`)** from the X.509 certificate of the trusted server. |
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.
It applies to all network traffic within the app
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:
The Network Security Configuration (NSC) is the preferred and recommended way to implement certificate pinning in Android, as it provides a declarative, maintainable, and secure approach without requiring code changes. It applies to all network traffic managed by the Android framework within the app, including
HttpsURLConnection
-based connections andWebView
requests (unless a customTrustManager
is used). For communication from native code, NSC does not apply, and other mechanisms need to be considered.
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.
One small change from my side. Otherwise this looks good to me.
Thank you for the work @titze and the suggestions @cpholguera!
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.
MiTM change
Co-authored-by: Carlos Holguera <[email protected]>
3ceb456
to
06b395f
Compare
Excellent, thanks for the updates! I'm reviewing the tests now and will post my suggestions soon. |
@@ -0,0 +1,38 @@ | |||
--- | |||
title: Missing Certificate Pinning in Code |
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.
title: Missing Certificate Pinning in Code | |
title: Missing Certificate Pinning in Network Security Configuration |
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
@@ -0,0 +1,29 @@ | |||
--- | |||
title: Missing Certificate Pinning in Network Traffic |
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?
|
||
1. Set up an intercepting proxy, for example @MASTG-TOOL-0077 or @MASTG-TOOL-0097. | ||
2. Install the application on a device connected to that proxy, and intercept the communication. | ||
3. Extract all domains which were intercepted. |
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.
|
||
## Evaluation | ||
|
||
The test case fails if any relevant domain was intercepted. |
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...?
The test case fails if any relevant domain was intercepted. | |
The test fails if any **security-sensitive domain** was intercepted. |
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:
- Authentication endpoints (e.g., login APIs, OAuth/token exchanges)
- Endpoints handling sensitive user data (e.g., financial transactions, personal data transfers)
- Endpoints that initiate encrypted tunnels (e.g., VPN, secure messaging services)
- Internal API endpoints used for secure app-server communication
What is NOT a Security-Sensitive Relevant Domain?
- Third-party CDNs or analytics services that the app does not control
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...
|
||
Cross-platform frameworks like Flutter, React Native, Cordova and Xamarin might require special considerations. Depending on the framework one of the following can apply: | ||
|
||
- The framework might support NSC. This is the case for Flutter apps on Android, but the NSC needs to be enabled in the `AndroidManifest`. See the [Flutter documentation](https://docs.flutter.dev/release/breaking-changes/network-policy-ios-android#migration-guide) on how to enable the network policy. |
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.
It seems this "feature" as well as the support for android:usesCleartextTraffic="true"
in the manifest was reverted in Flutter 2.2: https://docs.flutter.dev/release/breaking-changes#reverted-change-in-2-2
According to this comment it's still kind of there: flutter/flutter#106678 (comment) but I'm not sure what's going on.
It looks like the way to do pinning in Flutter is via securityContext.setTrustedCertificatesBytes
or securityContext.setTrustedCertificates
See https://api.flutter.dev/flutter/dart-io/SecurityContext/setTrustedCertificatesBytes.html
Co-authored-by: Carlos Holguera <[email protected]>
Thank you for submitting a Pull Request to the OWASP MASTG. Please make sure that:
If your PR is related to an issue. Please end your PR test with the following line:
This PR closes #2958.
Couple of notes: