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

[MASWE-0020] Weak Encryption (by @appknox) #2910

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

Conversation

sk3l10x1ng
Copy link
Collaborator

#2584 closes

@sushi2k sushi2k self-requested a review October 11, 2024 07:35
Copy link
Collaborator

@sushi2k sushi2k left a comment

Choose a reason for hiding this comment

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

Finished the weakness part. I applied my suggestions, let me know if anything to change from your end. Please look into the meta data of the weakness and clean it up accordingly.

Will look into the other parts in the upcoming days. Thanks for your contribution!

weaknesses/MASVS-CRYPTO/MASWE-0020.md Show resolved Hide resolved
weaknesses/MASVS-CRYPTO/MASWE-0020.md Outdated Show resolved Hide resolved
weaknesses/MASVS-CRYPTO/MASWE-0020.md Outdated Show resolved Hide resolved
weaknesses/MASVS-CRYPTO/MASWE-0020.md Outdated Show resolved Hide resolved
weaknesses/MASVS-CRYPTO/MASWE-0020.md Outdated Show resolved Hide resolved
weaknesses/MASVS-CRYPTO/MASWE-0020.md Outdated Show resolved Hide resolved
weaknesses/MASVS-CRYPTO/MASWE-0020.md Outdated Show resolved Hide resolved
tests-beta/android/MASVS-CRYPTO/MASTG-TEST-0211.md Outdated Show resolved Hide resolved

## Overview

In this test case, we will look for the use of weak encryption in Android applications. To do this, we need to focus on the cryptographic implementations of algorithm such as DES, 3DES,Weak encryption modes (e.g. ECB) and Cipher.getInstance("AES") defaults to ECB.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be more elaborate, similar to https://mas.owasp.org/MASTG/tests-beta/android/MASVS-CRYPTO/MASTG-TEST-0208/

Be careful with missing whitespace and ensure code elements are enclosed in backticks, e.g., Cipher

Also, try to use the refs we included in MASWE-0020 wherever possible as inline links for the text. Especially the ones from Support Google and Developer Android.

@@ -0,0 +1,33 @@
---
platform: android
title: Weak Encryption
Copy link
Collaborator

@cpholguera cpholguera Oct 19, 2024

Choose a reason for hiding this comment

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

To go inline with MAST-DEMO-0018

Suggested change
title: Weak Encryption
title: Uses of Insecure Encryption Algorithms in Cipher with semgrep

@sk3l10x1ng
Copy link
Collaborator Author

sk3l10x1ng commented Oct 21, 2024

@cpholguera noted. will work on the requested changes

@cpholguera
Copy link
Collaborator

Thank you very much @sk3l10x1ng!

@sk3l10x1ng
Copy link
Collaborator Author

sk3l10x1ng commented Nov 5, 2024

@cpholguera @sushi2k, requested changes have been done. thank you

@@ -0,0 +1,27 @@
---
title: Weak Encryption Algorithms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this file, we already have the one for algorithms: https://mas.owasp.org/MASTG/tests-beta/android/MASVS-CRYPTO/MASTG-TEST-0221/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cpholguera does this file need to be completely removed or only remove the DES algorithm section?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file can be completely removed. We already have this test in https://mas.owasp.org/MASTG/tests-beta/android/MASVS-CRYPTO/MASTG-TEST-0221/ including DES as well.

Comment on lines 45 to 46
- Always use modern, well-established cryptographic libraries in mobile apps that follow best practices and offer cryptographic algorithms that are aligned with the recommendations by NIST.
- For example it is recommended to use secure modes of operations like GCM (Galois/Counter Mode) mode for AES with 256-bit keys, providing both encryption and authentication.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can rework this section to mirror the previous one adding one mitigation for each of the modes of introduction.

Suggested change
- Always use modern, well-established cryptographic libraries in mobile apps that follow best practices and offer cryptographic algorithms that are aligned with the recommendations by NIST.
- For example it is recommended to use secure modes of operations like GCM (Galois/Counter Mode) mode for AES with 256-bit keys, providing both encryption and authentication.
- **Adopt Secure Algorithms**: Replace deprecated algorithms such as DES, 3DES, MD5, and SHA-1 with AES (minimum 256-bit key) and SHA-256 or higher.
- **Use Secure Encryption Modes**: Choose secure modes such as AES-GCM or AES-CCM and avoid insecure modes such as ECB.
- **Ensure Proper Initialization Vector Management**: Generate IVs using cryptographically secure random number generators and ensure they are unique for every operation.
- **Use Strong Key Sizes**: Enforce key lengths of at least 256 bits for AES and avoid using small or weak keys such as 56-bit DES keys.
- **Rely on Proper Cryptographic Libraries**: Avoid using XOR, Base64 encoding, or obfuscation as substitutes for encryption and rely on well-vetted cryptographic libraries.


### Evaluation

Review each of the reported instances.
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
Review each of the reported instances.
The reported instances include:

Comment on lines 31 to 33
- Line 37 seems the code utilize insecure DES algorithm.
- Line 56 seems to utilize Cipher.getInstance("AES") defaults to ECB, which is insecure.
- Line 79 seems the code utilize insecure 3DES algorithm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's using it, right? It doesn't "seem" like it's using it.

@@ -0,0 +1,33 @@
---
platform: android
title: Uses of Insecure Encryption Algorithms in Cipher with semgrep
Copy link
Collaborator

@cpholguera cpholguera Nov 29, 2024

Choose a reason for hiding this comment

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

Please add another demo for title: Uses of Insecure Encryption Modes in Cipher with semgrep and test: MASTG-TEST-0231

Copy link
Collaborator Author

@sk3l10x1ng sk3l10x1ng Dec 19, 2024

Choose a reason for hiding this comment

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

@cpholguera

  • do we need to create new Demo file for title: Uses of Insecure Encryption Modes in Cipher with semgrep or make changes to the existing one.
  • The test:MASTG-TEST-0231 point to Reference to logging of APIs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ID of the "Weak Encryption Modes" has changed to MASTG-TEST-0232 https://mas.owasp.org/MASTG/tests-beta/android/MASVS-CRYPTO/MASTG-TEST-0232/

You may add a new demo pointing to that test or do it on a separate PR.

You can keep this demo with the changes indicated below.

rules/mastg-android-weak-encryption.yaml Show resolved Hide resolved
@cpholguera
Copy link
Collaborator

@sk3l10x1ng any news on this? Thanks!

@sk3l10x1ng
Copy link
Collaborator Author

@cpholguera will work on the request changes

@cpholguera
Copy link
Collaborator

Thank you!

title: Uses of Insecure Encryption Algorithms in Cipher with semgrep
id: MASTG-DEMO-0016
code: [java]
test: MASTG-TEST-0211
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can point to the existing test

Suggested change
test: MASTG-TEST-0211
test: MASTG-TEST-0221


### Observation

The rule has identified five instances in the code file where an insecure encryption is used. The specified line numbers can be located in the original code for further investigation and remediation.
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
The rule has identified five instances in the code file where an insecure encryption is used. The specified line numbers can be located in the original code for further investigation and remediation.
The rule has identified five instances in the code file where insecure encryption algorithms are used.

@cpholguera
Copy link
Collaborator

@sk3l10x1ng we still have some requested changes here, do you have any news?

@sk3l10x1ng
Copy link
Collaborator Author

@sk3l10x1ng we still have some requested changes here, do you have any news?

I've completed the demo locally, I will push the commits to the pull request

@sk3l10x1ng
Copy link
Collaborator Author

@cpholguera I've added a new demo file. Please review it

@cpholguera
Copy link
Collaborator

Hi @sk3l10x1ng,

I still can see only one demo, unfortunately. Could you please double check?

The following demos (each in its own folder) are expected:

  • Uses of Insecure Encryption Algorithms in Cipher with semgrep
  • Uses of Insecure Encryption Modes in Cipher with semgrep

This would include 2 separate semgrep files.

@sk3l10x1ng
Copy link
Collaborator Author

sk3l10x1ng commented Jan 17, 2025

Hi @sk3l10x1ng,

I still can see only one demo, unfortunately. Could you please double check?

The following demos (each in its own folder) are expected:

  • Uses of Insecure Encryption Algorithms in Cipher with semgrep
  • Uses of Insecure Encryption Modes in Cipher with semgrep

This would include 2 separate semgrep files.

@cpholguera updated the pull request with two demos

@cpholguera
Copy link
Collaborator

I can see them now, thank you!


For example the use of TDEA (Triple Data Encryption Algorithm), which is often referred to as "Triple DES" or "3DES" is disallowed by NIST since end of 2023, due to known attacks like, "meet-in-the-middle", collision attacks and [Sweet32](https://nvd.nist.gov/vuln/detail/CVE-2016-2183).

Similarly, hash functions such as MD5 and SHA-1, which were previously popular for ensuring data integrity, are now considered deprecated as attackers can generate hash collisions, potentially leading to data tampering.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove


## Mode of Introduction

- **Use of Deprecated Algorithms** : Relying on outdated or weak cryptographic algorithms can allow threat actors to attack the cipher text, key or exploit known vulnerabilities in the algorithm, for example through brute force attacks.
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
- **Use of Deprecated Algorithms** : Relying on outdated or weak cryptographic algorithms can allow threat actors to attack the cipher text, key or exploit known vulnerabilities in the algorithm, for example through brute force attacks.
- **Use of Deprecated Algorithms** : Relying on outdated or weak encryption algorithms can allow threat actors to attack the cipher text, key or exploit known vulnerabilities in the algorithm, for example through brute force attacks.

@@ -0,0 +1,12 @@
rules:
- id: weak-encryption
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
- id: weak-encryption
- id: weak-encryption-algorithms

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to MastgTest_reversed.java (see other demos)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to MastgTest_reversed.java (see other demos)

languages:
- java
severity: WARNING
message: Weak encryption modes
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
message: Weak encryption modes
message: [MASVS-CRYPTO-1] Weak encryption modes found in use.

- pattern: Cipher.getInstance("AES/ECB/ISO10126Padding")
- pattern: Cipher.getInstance("DES/ECB/PKCS5Padding")
- pattern: Cipher.getInstance("DESede/ECB/PKCS5Padding")
metadata:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move before message

- pattern: Cipher.getInstance("DES/ECB/PKCS5Padding")
- pattern: Cipher.getInstance("DESede/ECB/PKCS5Padding")
metadata:
summary: This rule looks for weak encryption mode like AES/ECB/NoPadding,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update to be one line:

This rule looks for weak encryption modes such as AES-ECB.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After all other changes were made. Rerun and push new outputs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Update rule file name here as well.

After all other changes were made. Rerun and push new outputs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants