-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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-0009] Add Weak Cryptographic Key Generation (by appknox) #2622
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.
Thanks you @sk3l10x1ng for the PR! Have a look at the comments. The main change that is needed is to not mention AES-128 as insecure algorithm. You could use AES-64 instead as example. Let me know if any questions
|
||
Review each of the reported instances. | ||
|
||
- Line 2 has inialized the RSA key size of 1024 bits. |
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.
- Line 2 has inialized the RSA key size of 1024 bits. | |
- Line 2 has initialized the RSA key size of 1024 bits. |
|
||
## Overview | ||
|
||
The key size, also known as the key length, is dependent on the number of bits in which the message is stored. Encryption algorithms that utilize insufficient key sizes are vulnerable to attacks, while longer keys typically entail more intricate encryption. |
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.
Generating short keys is one thing, but the generation of keys should also be based on libraries or API's of the OS with accurate entropy and should have a true random-number generator. This is missing at the moment, could you add a paragraph about this? We should also add to use the available mechanisms on the OS, for example using the KeyStore on Android or Apple CryptoKit on iOS.
I don't understand the first sentence, why is the key length dependent on the message size? For example for AES, the key sizes are 128, 192, and 256 bits. The message will then be divided into blocks according to the key length to encrypt it.
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.
You can also reference to the android documentation about PRNG's: https://developer.android.com/privacy-and-security/risks/weak-prng
## Impact | ||
|
||
- **Risk of Brute-Force Attacks**: | ||
With a shorter key length, the number of possible combinations decreases, increasing the likelihood of an attacker successfully cracking the encryption through brute force method. |
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.
Not only the length, but also if the PRNG was having a predictable input.
With a shorter key length, the number of possible combinations decreases, increasing the likelihood of an attacker successfully cracking the encryption through brute force method. | ||
|
||
- **Loss of Confidentiality**: | ||
Encryption is utilized to safeguard the confidentiality of data, allowing only authorized users to access it. Weak encryption keys can enable attackers to obtain encrypted information and exploit it effortlessly. |
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.
Encryption is utilized to safeguard the confidentiality of data, allowing only authorized users to access it. Weak encryption keys can enable attackers to obtain encrypted information and exploit it effortlessly. | |
Encryption is utilized to safeguard the confidentiality of data, allowing only authorized users to access it. Weak encryption keys can enable attackers to obtain encrypted information and exploit it. |
Let's stay objective, therefore removed "effortlessly".
The libraries used by the application, the algorithms used and cryptographic schemes used are outdated. | ||
|
||
- **Use of Insecure Algorithms**: | ||
The use of insecure algorithms (e.g using the 1024-bit RSA key, 128-bit AES key, 160-bit ECDSA key) in an application poses significant security risks to data. |
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.
128-bit AES is not insecure but a recommended algorithm for encryption.
The use of insecure algorithms (e.g using the 1024-bit RSA key, 128-bit AES key, 160-bit ECDSA key) in an application poses significant security risks to data. | |
The use of deprecated algorithms (e.g using the 1024-bit RSA key or 160-bit ECDSA key) in an application poses significant security risks to data. |
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.
A 3rd point should be the usage of weak PRNGs
|
||
## Overview | ||
|
||
The application appears to utilize a symmetric algorithm known as `AES-128` with a weak key configuration, making it susceptible to multiple vulnerabilities and potential security breaches. |
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 can be insecure if the CBC mode is used for AES and if the IV is empty or weak when using the AES key for encryption as you can then try to brute force, but AES-128 is a strong encryption cipher.
See also https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-57pt1r5.pdf
## Modes of Introduction | ||
|
||
- **Third Party Libraries**: | ||
The libraries used by the application, the algorithms used and cryptographic schemes used are outdated. |
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 libraries used by the application, the algorithms used and cryptographic schemes used are outdated. | |
The libraries, algorithms and cryptographic schemes used by the application are out of date. |
$S = new ECGenParameterSpec("secp224r1"); | ||
... | ||
$K.initialize($S); | ||
- pattern: | |
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.
Please remove this pattern
@@ -0,0 +1,49 @@ | |||
rules: |
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.
Let's add the source, if it wasn't done by us from scratch.
rules: | |
# original source: https://github.com/MobSF/mobsfscan/blob/main/mobsfscan/rules/semgrep/crypto/weak_key_size.yaml | |
rules: |
@@ -0,0 +1,135 @@ | |||
{ |
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.
Can you please remove the SARIF file for now? We can add later to keep the PR cleaner.
|
Hi @sk3l10x1ng. You could use 3DES, as this is algorithm is deprecated, instead of AES-128. You are right AES-128 is listed as example in the issue, but this is not correct. Thanks for your support. |
@cpholguera @sushi2k . updated according to new file structure and requested changed done in #2849 |
closes #2573