Skip to content
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

Add MASWE-0047, MASWE-0048, MASWE-0049, MASWE-0050, MASWE-0051, MASWE-0052 #2919

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cpholguera
Copy link
Collaborator

@cpholguera cpholguera commented Oct 26, 2024

This PR
closes #2686
closes #2688
closes #2689
closes #2690
closes #2691
closes #2692

@cpholguera cpholguera changed the title Add MASWE-0050 Add MASWE-0047, MASWE-0048, MASWE-0049, MASWE-0050, MASWE-0051, MASWE-0052 Nov 6, 2024
@@ -22,3 +22,31 @@ status: draft

---

## Overview

Certificate pinning is a security technique used to ensure that an app only trusts specific certificates or public keys when establishing secure connections. Insecure identity pinning occurs when the implementation of certificate or public key pinning is flawed or improperly configured. This weakness can leave the app vulnerable to Man-in-the-Middle (MITM) attacks and other security threats. Common issues include outdated pins, improper validation, accepting all certificates, or using insecure methods for dynamic pinning.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename weakness to use Certificate pinning? If not, this sentence should start with Identity pinning and quickly explain that it's also called certificate pinning or TLS pinning. This should be done in both cases even, to make sure all names are mentioned at the start.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird to have 'accepting all certificates' in here. We could go for 'insecure fallback' or something, or 'insecure default', but 'accepting all certificates' seems like the opposite of identity pinning and should be a separate weakness ("Not validating TLS certificate" or something)

Copy link
Collaborator

@TheDauntless TheDauntless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions :)

@@ -21,3 +21,29 @@ status: draft

---

## Overview

Applications that do not utilize platform-provided networking APIs or well-established security libraries are susceptible to security vulnerabilities. When developers implement custom networking code or "roll their own" security mechanisms, they risk introducing flaws due to a lack of deep expertise in cryptography and network security. Platform-provided APIs and libraries, such as `NSURLSession` on iOS or `HttpsURLConnection` on Android, are designed and maintained by experts, incorporating security best practices and regular updates to address new threats and vulnerabilities.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weakness title should be 'Proven' instead of 'Proved'. While both are correct past participles, 'proven' is typically used for the adjective case.

---

## Overview

When data is sent in cleartext—without encryption—it becomes accessible to attackers who can monitor network channels. Attackers can perform passive eavesdropping to intercept data or employ active Man-in-the-Middle (MITM) attacks to manipulate data, potentially altering app behavior or injecting malicious content.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title should maybe be aligned with MASWE-0048? (Insecure Non-HTTP Traffic)? So "Insecure HTTP Traffic" would be better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When data is sent in cleartextwithout encryptionit becomes accessible to attackers who can monitor network channels. Attackers can perform passive eavesdropping to intercept data or employ active Man-in-the-Middle (MITM) attacks to manipulate data, potentially altering app behavior or injecting malicious content.
When data is sent in cleartext (ie. without encryption) it becomes accessible to attackers who can monitor network channels. Attackers can perform passive eavesdropping to intercept data or employ active Man-in-the-Middle (MITM) attacks to manipulate data, potentially altering app behavior or injecting malicious content.

weaknesses/MASVS-NETWORK/MASWE-0050.md Outdated Show resolved Hide resolved
weaknesses/MASVS-NETWORK/MASWE-0050.md Outdated Show resolved Hide resolved

## Modes of Introduction

- **Cleartext Traffic Allowed Globally in Configurations:** Global configuration settings allow cleartext traffic, making all network communication insecure.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling the flag doens't make everything insecure. Everything can be secure even with this flag.

Suggested change
- **Cleartext Traffic Allowed Globally in Configurations:** Global configuration settings allow cleartext traffic, making all network communication insecure.
- **Cleartext Traffic Allowed Globally in Configurations:** Global configuration settings allow cleartext traffic, making insecure APIs available to the developer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but what do you mean by "insecure APIs available to the developer"?

- **Restrict Network Bindings**: Configure the application to bind only to specific, necessary network interfaces, avoiding the use of wildcard addresses like `INADDR_ANY`.
- **Implement Strong Access Controls**: Enforce authentication and authorization for any services exposed through open ports to ensure only authorized entities can connect.
- **Disable Debugging Services in Production**: Ensure that all development and debugging network services are disabled or removed in production builds.
- **Validate and Sanitize Inputs**: Properly validate all incoming data from network connections to prevent injection attacks and buffer overflows.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that this is irrelevant for this specific weakness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain this a bit more?

- **Implement Strong Access Controls**: Enforce authentication and authorization for any services exposed through open ports to ensure only authorized entities can connect.
- **Disable Debugging Services in Production**: Ensure that all development and debugging network services are disabled or removed in production builds.
- **Validate and Sanitize Inputs**: Properly validate all incoming data from network connections to prevent injection attacks and buffer overflows.
- **Use Secure Communication Protocols**: Employ encryption and secure protocols (e.g., TLS/SSL) for any network communication to protect data in transit.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also irrelevant for this weakness?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain this a bit more?


---

## Overview

Applications that do not properly validate SSL/TLS certificates during secure communication are susceptible to man-in-the-middle attacks and other security breaches. This weakness occurs when an application accepts invalid, expired, self-signed, or untrusted certificates without appropriate verification, compromising the integrity and confidentiality of data in transit.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially remove SSL? SSL shouldn't be used anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change man-in-the-middle to machine-in-the-middle which is a common new phrase for it to be gender neutral.

weaknesses/MASVS-NETWORK/MASWE-0052.md Outdated Show resolved Hide resolved
weaknesses/MASVS-NETWORK/MASWE-0052.md Outdated Show resolved Hide resolved
Co-authored-by: Jeroen Beckers <info@dauntless.be>
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 suggestion.

Files not reviewed (1)
  • weaknesses/MASVS-NETWORK/MASWE-0050.md: Evaluated as low risk
Comments skipped due to low confidence (1)

weaknesses/MASVS-NETWORK/MASWE-0049.md:46

  • The term "NSURLSession" should be "URLSession".
Utilize Platform-Provided Networking APIs: Always use the networking APIs provided by the platform, such as `NSURLSession` for iOS and `HttpsURLConnection` or `OkHttp` for Android, which handle many security concerns internally.

@@ -21,3 +21,29 @@ status: draft

---

## Overview

Applications that do not utilize platform-provided networking APIs or well-established security libraries are susceptible to security vulnerabilities. When developers implement custom networking code or "roll their own" security mechanisms, they risk introducing flaws due to a lack of deep expertise in cryptography and network security. Platform-provided APIs and libraries, such as `NSURLSession` on iOS or `HttpsURLConnection` on Android, are designed and maintained by experts, incorporating security best practices and regular updates to address new threats and vulnerabilities.
Copy link
Preview

Copilot AI Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "roll their own" should be "roll-your-own".

Suggested change
Applications that do not utilize platform-provided networking APIs or well-established security libraries are susceptible to security vulnerabilities. When developers implement custom networking code or "roll their own" security mechanisms, they risk introducing flaws due to a lack of deep expertise in cryptography and network security. Platform-provided APIs and libraries, such as `NSURLSession` on iOS or `HttpsURLConnection` on Android, are designed and maintained by experts, incorporating security best practices and regular updates to address new threats and vulnerabilities.
Applications that do not utilize platform-provided networking APIs or well-established security libraries are susceptible to security vulnerabilities. When developers implement custom networking code or "roll-your-own" security mechanisms, they risk introducing flaws due to a lack of deep expertise in cryptography and network security. Platform-provided APIs and libraries, such as `NSURLSession` on iOS or `HttpsURLConnection` on Android, are designed and maintained by experts, incorporating security best practices and regular updates to address new threats and vulnerabilities.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants