[MASWE-0009] Add Weak Cryptographic Key Generation (by appknox) (#2849)

* MASWE-0009 * fix spell * fix markdown-lint * updated weakness * change test ID * add semgrep as tool * change demo IDs * change demo id as duplicate * Update weaknesses/MASVS-CRYPTO/MASWE-0009.md * Apply suggestions from code review * updated changes * renamed TOOL-0105 -> TOOL-0109 * fix changes * rm semgrep (will be added separately) and update refs to the tool * update ios demo to use r2 and the MASTestApp for iOS * update spell checker ignore words list * rm ios folder * add ios folder to correct name and demo based on r2 * update MASTG-TEST-0209 with libraries and references. Extended to consider also dynamic analysis. * change to modes of introduction * update DEMO-0011 to be about RSA key size * Apply suggestions from code review * add binary for demo 11 * update r2 script and output * Update weaknesses/MASVS-CRYPTO/MASWE-0009.md * Update tests-beta/ios/MASVS-CRYPTO/MASTG-TEST-0209.md Co-authored-by: Carlos Holguera <[email protected]> * Apply suggestions from code review Co-authored-by: Carlos Holguera <[email protected]> * Apply suggestions from code review Co-authored-by: Sven <[email protected]> * updated android demo * changed semgrep rule to standard form * Apply suggestions from code review * remove extra line * fix link --------- Co-authored-by: Sven <[email protected]> Co-authored-by: Carlos Holguera <[email protected]>
OWASP · Aug 25, 2024 · c7bd3ef · c7bd3ef
1 parent 88b5e16
commit c7bd3ef
Show file tree

Hide file tree

Showing 18 changed files with 461 additions and 9 deletions.
diff --git a/.github/workflows/spell-checker-pr.yml b/.github/workflows/spell-checker-pr.yml
@@ -25,5 +25,5 @@ jobs:
         with:
           check_filenames: true
           skip: "*.json,*.yml,*.apk,*.ipa,*.svg"
-          ignore_words_list: "aas,aaS,ba,bund,compliancy,firt,ist,keypair,ligh,Manuel,Manual,ro,ser,synopsys,theses,zuser,lief"
+          ignore_words_list: "aas,aaS,ba,bund,compliancy,firt,ist,keypair,ligh,Manuel,Manual,ro,ser,synopsys,theses,zuser,lief,EDE"
           path: ${{ env.CHANGED_FILES }}
diff --git a/.github/workflows/spell-checker.yml b/.github/workflows/spell-checker.yml
@@ -13,6 +13,6 @@ jobs:
       - uses: actions/checkout@v4
       - uses: codespell-project/actions-codespell@master
         with:
-          ignore_words_list: "aas,aaS,ba,bund,compliancy,firt,ist,keypair,ligh,Manuel,Manual,ro,ser,synopsys,theses,zuser,lief"
+          ignore_words_list: "aas,aaS,ba,bund,compliancy,firt,ist,keypair,ligh,Manuel,Manual,ro,ser,synopsys,theses,zuser,lief,EDE"
           skip: "*.json,*.yml,*.apk,*.ipa,*.svg"
           exclude_file: docs/contributing.md
diff --git a/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/MASTG-DEMO-0012.md b/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/MASTG-DEMO-0012.md
@@ -0,0 +1,29 @@
+---
+platform: android
+title: Weak Cryptographic Key Generation
+code: [java]
+id: MASTG-DEMO-0012
+test: MASTG-TEST-0208
+---
+
+### Sample
+
+{{ MastgTest.kt # MastgTest_reversed.java }}
+
+### Steps
+
+Let's run our @MASTG-TOOL-0110 rule against the sample code.
+
+{{ ../../../../rules/mastg-android-weak-crypto-key-generation.yml }}
+
+{{ run.sh }}
+
+### Observation
+
+The rule has identified some instances in the code file where cryptographic keys are being generated. The specified line numbers can be located in the reverse-engineered code for further investigation and remediation.
+
+{{ output.txt }}
+
+### Evaluation
+
+The test fails because the key size of the RSA key is set to `1024` bits, and the size of the AES key is set to `128`, which is considered weak in both cases.
diff --git a/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/MastgTest.kt b/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/MastgTest.kt
@@ -0,0 +1,33 @@
+package org.owasp.mastestapp
+
+import android.util.Log
+import android.content.Context
+import android.security.keystore.KeyProperties
+import android.util.Base64
+import java.security.KeyPairGenerator
+import java.security.SecureRandom
+import javax.crypto.KeyGenerator
+import javax.crypto.SecretKey
+
+class MastgTest (private val context: Context){
+
+    fun mastgTest(): String {
+
+        val generator = KeyPairGenerator.getInstance(KeyProperties.KEY_ALGORITHM_RSA)
+        generator.initialize(1024, SecureRandom()) // for 1025 bit RSA Key
+        val keypair = generator.genKeyPair()
+        Log.d("Keypair generated RSA", Base64.encodeToString(keypair.public.encoded, Base64.DEFAULT))
+
+        val keyGen1 = KeyGenerator.getInstance("AES")
+        keyGen1.init(128) // for 128 bit AES key
+        val secretKey1: SecretKey = keyGen1.generateKey()
+
+        val keyGen2 = KeyGenerator.getInstance("AES")
+        keyGen2.init(256) // for 256 bit AES key
+        val secretKey2: SecretKey = keyGen2.generateKey()
+
+        return "Generated RSA Key:\n " + Base64.encodeToString(keypair.public.encoded, Base64.DEFAULT)+"Generated AES Key1\n "+ Base64.encodeToString(secretKey1.encoded, Base64.DEFAULT)+ "Generated AES Key2\n "+ Base64.encodeToString(secretKey2.encoded, Base64.DEFAULT);
+
+    }
+
+}
diff --git a/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/MastgTest_reversed.java b/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/MastgTest_reversed.java
@@ -0,0 +1,41 @@
+package org.owasp.mastestapp;
+
+import android.content.Context;
+import android.util.Base64;
+import android.util.Log;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.SecureRandom;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import kotlin.Metadata;
+import kotlin.jvm.internal.Intrinsics;
+
+/* compiled from: MastgTest.kt */
+@Metadata(d1 = {"\u0000\u0018\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0000\n\u0002\u0018\u0002\n\u0002\b\u0002\n\u0002\u0010\u000e\n\u0000\b\u0007\u0018\u00002\u00020\u0001B\r\u0012\u0006\u0010\u0002\u001a\u00020\u0003¢\u0006\u0002\u0010\u0004J\u0006\u0010\u0005\u001a\u00020\u0006R\u000e\u0010\u0002\u001a\u00020\u0003X\u0082\u0004¢\u0006\u0002\n\u0000¨\u0006\u0007"}, d2 = {"Lorg/owasp/mastestapp/MastgTest;", "", "context", "Landroid/content/Context;", "(Landroid/content/Context;)V", "mastgTest", "", "app_debug"}, k = 1, mv = {1, 9, 0}, xi = 48)
+/* loaded from: classes4.dex */
+public final class MastgTest {
+    public static final int $stable = 8;
+    private final Context context;
+
+    public MastgTest(Context context) {
+        Intrinsics.checkNotNullParameter(context, "context");
+        this.context = context;
+    }
+
+    public final String mastgTest() {
+        KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA");
+        generator.initialize(1024, new SecureRandom());
+        KeyPair keypair = generator.genKeyPair();
+        Log.d("Keypair generated RSA", Base64.encodeToString(keypair.getPublic().getEncoded(), 0));
+        KeyGenerator keyGen1 = KeyGenerator.getInstance("AES");
+        keyGen1.init(128);
+        SecretKey secretKey1 = keyGen1.generateKey();
+        Intrinsics.checkNotNullExpressionValue(secretKey1, "generateKey(...)");
+        KeyGenerator keyGen2 = KeyGenerator.getInstance("AES");
+        keyGen2.init(256);
+        SecretKey secretKey2 = keyGen2.generateKey();
+        Intrinsics.checkNotNullExpressionValue(secretKey2, "generateKey(...)");
+        return "Generated RSA Key:\n " + Base64.encodeToString(keypair.getPublic().getEncoded(), 0) + "Generated AES Key1\n " + Base64.encodeToString(secretKey1.getEncoded(), 0) + "Generated AES Key2\n " + Base64.encodeToString(secretKey2.getEncoded(), 0);
+    }
+}
diff --git a/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/output.txt b/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/output.txt
@@ -0,0 +1,15 @@
+
+
+┌─────────────────┐
+│ 2 Code Findings │
+└─────────────────┘
+
+    MastgTest_reversed.java
+    ❯❱ weak_key_size
+          Cryptographic implementations with insufficient key length are being used.
+
+           27┆ KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA");
+           28┆ generator.initialize(1024, new SecureRandom());
+            ⋮┆----------------------------------------
+           31┆ KeyGenerator keyGen1 = KeyGenerator.getInstance("AES");
+           32┆ keyGen1.init(128);
diff --git a/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/run.sh b/demos/android/MASVS-CRYPTO/MASTG-DEMO-0012/run.sh
@@ -0,0 +1 @@
+NO_COLOR=true semgrep -c ../../../../rules/mastg-android-weak_key_generation.yml ./MastgTest_reversed.java --text -o output.txt
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/MASTG-DEMO-0011.md b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/MASTG-DEMO-0011.md
@@ -0,0 +1,38 @@
+---
+platform: ios
+title: Uses of Weak Key Size in CCCrypt with r2
+code: [swift]
+id: MASTG-DEMO-0011
+test: MASTG-TEST-0209
+---
+
+### Sample
+
+{{ MastgTest.swift }}
+
+### Steps
+
+When calling [`SecKeyCreateRandomKey`](https://developer.apple.com/documentation/security/1823694-seckeycreaterandomkey) the key size is specified in the [`kSecAttrKeySizeInBits`](https://developer.apple.com/documentation/security/ksecattrkeysizeinbits) attribute within the `parameters` dictionary. See [Key Generation Attributes](https://developer.apple.com/documentation/security/certificate_key_and_trust_services/keys/key_generation_attributes) for details.
+
+1. Unzip the app package and locate the main binary file (@MASTG-TECH-0058), which in this case is `./Payload/MASTestApp.app/MASTestApp`.
+2. Open the app binary with @MASTG-TOOL-0073 with the `-i` option to run this script.
+
+{{ security_keysize.r2 }}
+
+{{ run.sh }}
+
+### Observation
+
+The output contains the disassembled code of the function using `SecKeyCreateRandomKey`.
+
+{{ output.txt }}
+
+This function is pretty big so we just included the relevant part of the code that's right before the call to `SecKeyCreateRandomKey`. Note that we can see attributes being set in the `parameters` dictionary such as `kSecAttrKeySizeInBits` as `reloc.kSecAttrKeySizeInBits`. In radare2, this means that the symbol `kSecAttrKeySizeInBits` is not directly referenced by an absolute address but rather through a relocation entry. This entry will be resolved by the dynamic linker at runtime to the actual address where `kSecAttrKeySizeInBits` is located in memory.
+
+### Evaluation
+
+In the output we can see how the `kSecAttrKeySizeInBits` attribute is set to `1024` bits (0x400 in hexadecimal) using the `x8` register. This is later used to call `SecKeyCreateRandomKey`.
+
+{{ evaluation.txt }}
+
+The test fails because the key size is set to `1024` bits, which is considered weak for RSA encryption. The key size should be increased to `2048` bits or higher to provide adequate security against modern cryptographic attacks.
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/MASTestApp b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/MASTestApp
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/MastgTest.swift b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/MastgTest.swift
@@ -0,0 +1,90 @@
+import Foundation
+import Security
+
+struct MastgTest {
+    static func mastgTest(completion: @escaping (String) -> Void) {
+
+        // Step 1: Generate an RSA key pair with a 1024-bit key size
+        let tag = "org.owasp.mas.rsa-1014".data(using: .utf8)!
+        let keyAttributes: [String: Any] = [
+            kSecAttrKeyType as String: kSecAttrKeyTypeRSA,
+            kSecAttrKeySizeInBits as String: 1024,  // Using 1024-bit RSA key
+            kSecPrivateKeyAttrs as String:
+               [kSecAttrIsPermanent as String:    true,     // to store it in the Keychain
+                kSecAttrApplicationTag as String: tag]      // to find and retrieve it from the Keychain later
+        ]
+
+        var error: Unmanaged<CFError>?
+        guard let privateKey = SecKeyCreateRandomKey(keyAttributes as CFDictionary, &error) else {
+            completion("Failed to generate private key: \(String(describing: error))")
+            return
+        }
+
+        guard let publicKey = SecKeyCopyPublicKey(privateKey) else {
+            completion("Failed to generate public key")
+            return
+        }
+
+        // Convert the private key to data (DER format)
+        guard let privateKeyData = SecKeyCopyExternalRepresentation(privateKey, &error) as Data? else {
+            completion("Failed to extract private key: \(String(describing: error))")
+            return
+        }
+
+        // Encode the private key for display
+        //let privateKeyBase64 = privateKeyData.base64EncodedString()
+        let privateKeyHex = privateKeyData.map { String(format: "%02hhx", $0) }.joined()
+
+        // Convert the public key to data (DER format)
+        guard let publicKeyData = SecKeyCopyExternalRepresentation(publicKey, &error) as Data? else {
+            completion("Failed to extract public key: \(String(describing: error))")
+            return
+        }
+
+        // Encode the public key for display
+        // let publicKeyBase64 = publicKeyData.base64EncodedString()
+        let publicKeyHex = publicKeyData.map { String(format: "%02hhx", $0) }.joined()
+
+        // Data to sign
+        let dataToSign = "This is a sample text".data(using: .utf8)!
+
+        // Step 2: Sign the data with the private key
+        guard let signature = SecKeyCreateSignature(
+            privateKey,
+            SecKeyAlgorithm.rsaSignatureMessagePKCS1v15SHA256,
+            dataToSign as CFData,
+            &error
+        ) else {
+            completion("Signing failed: \(String(describing: error))")
+            return
+        }
+
+        // Convert signature to hex string for display
+        let signatureHex = (signature as Data).map { String(format: "%02hhx", $0) }.joined()
+
+        // Step 3: Verify the signature with the public key
+        let verificationStatus = SecKeyVerifySignature(
+            publicKey,
+            SecKeyAlgorithm.rsaSignatureMessagePKCS1v15SHA256,
+            dataToSign as CFData,
+            signature as CFData,
+            &error
+        )
+
+        let verificationResult = verificationStatus ? "Signature is valid." : "Signature is invalid."
+
+        let value = """
+        Original: \(String(data: dataToSign, encoding: .utf8)!)
+        
+        Private Key (Hex): \(privateKeyHex)
+        
+        Public Key (Hex): \(publicKeyHex)
+        
+        Signature (Hex): \(signatureHex)
+        
+        Verification: \(verificationResult)
+        """
+
+        completion(value)
+    }
+}
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/evaluation.txt b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/evaluation.txt
@@ -0,0 +1,9 @@
+│       │   0x10000484c      080942f9       ldr x8, reloc.kSecAttrKeySizeInBits ; 0x10000c410   ->  Load the address of kSecAttrKeySizeInBits into x8
+│       │   0x100004850      000140f9       ldr x0, [x8]
+│       │   0x100004854      e30b0094       bl fcn.1000077e0
+│       │   0x100004858      800605a9       stp x0, x1, [x20, 0x50]
+│       │   0x10000485c      48000090       adrp x8, reloc.Foundation.__DataStorage._bytes.allocator__UnsafeMutableRawPointer______ ; 0x10000c000
+│       │   0x100004860      089d41f9       ldr x8, reloc.Swift.Int    ; 0x10000c338
+│       │   0x100004864      883e00f9       str x8, [x20, 0x78]
+│       │   0x100004868      08808052       mov w8, 0x400                                       ->  Move 0x400 (1024 in decimal) into w8, the lower 32 bits of x8
+│       │   0x10000486c      883200f9       str x8, [x20, 0x60]                                 ->  Store the final value (1024-bit key size) into memory
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/output.txt b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/output.txt
@@ -0,0 +1,70 @@
+Uses of SecKeyCreateRandomKey:
+0x1000078ac    1     12 sym.imp.SecKeyCreateRandomKey
+
+xrefs to SecKeyCreateRandomKey:
+sym.func.1000046f8 0x1000049a0 [CALL:--x] bl sym.imp.SecKeyCreateRandomKey
+
+Use of reloc.kSecAttrKeySizeInBits as input for SecKeyCreateRandomKey:
+            ; CALL XREF from sym.func.1000063a0 @ 0x1000063d8(x)
+┌ 2920: sym.func.1000046f8 (int64_t arg1, void *arg2, void *arg3);
+│           ; arg int64_t arg1 @ x0
+│           ; arg void *arg2 @ x1
+│           ; arg void *arg3 @ x2
+│           ; var void *var_0h @ sp+0x0
+│           ; var int64_t var_0h_14 @ sp+0x8
+│           ; var int64_t var_10h @ sp+0x10
+│           ; var int64_t var_10h_4 @ sp+0x18
+│           ; var void *var_20h @ sp+0x20
+│           ; var int64_t var_28h @ sp+0x28
+│           ; var int64_t var_30h @ sp+0x30
+│           ; var int64_t var_0h_12 @ sp+0x38
+│           ; var int64_t var_40h @ sp+0x40
+│           ; var int64_t var_0h_13 @ sp+0x48
+│           ; var void *var_50h_2 @ sp+0x50
+│           ; var int64_t var_0h_10 @ sp+0x58
+│           ; var int64_t var_0h_9 @ sp+0x60
+│           ; var void *var_0h_11 @ sp+0x68
+│           ; var int64_t var_0h_5 @ sp+0x70
+│           ; var int64_t var_0h_4 @ sp+0x78
+│           ; var int64_t var_80h @ sp+0x80
+│           ; var int64_t var_80h_2 @ sp+0x88
+│           ; var int64_t var_0h_2 @ sp+0x90
+│           ; var void *var_98h @ sp+0x98
+│           ; var void *var_98h_2 @ sp+0xa0
+│           ; var int64_t var_0h_7 @ sp+0xb0
+│           ; var int64_t var_b0h @ sp+0xb8
+│           ; var void *var_c0h @ sp+0xc0
+│           ; var void *arg0 @ sp+0xc8
+│           ; var int64_t var_0h_8 @ sp+0xd0
+│           ; var int64_t var_0h_6 @ sp+0xd8
+│           ; var void *var_e0h @ sp+0xe0
+│           ; var void *var_160h @ sp+0x160
+│           ; var int64_t var_0h_3 @ sp+0x210
+│           ; var int64_t var_60h @ sp+0x220
+│           ; var int64_t var_60h_2 @ sp+0x228
+│           ; var int64_t var_10h_2 @ sp+0x230
+│           ; var int64_t var_10h_3 @ sp+0x238
+│           ; var int64_t var_20h_2 @ sp+0x240
+│           ; var int64_t var_20h_3 @ sp+0x248
+│           ; var int64_t var_30h_2 @ sp+0x250
+│           ; var int64_t var_30h_3 @ sp+0x258
+│           ; var int64_t var_40h_2 @ sp+0x260
+│           ; var int64_t var_40h_3 @ sp+0x268
+│           ; var int64_t var_50h @ sp+0x270
+│           ; var int64_t var_50h_3 @ sp+0x278
+│           0x1000046f8      fc6fbaa9       stp x28, x27, [sp, -0x60]!
+...
+│           0x10000484c      080942f9       ldr x8, reloc.kSecAttrKeySizeInBits ; 0x10000c410
+│           0x100004850      000140f9       ldr x0, [x8]
+│           0x100004854      e30b0094       bl fcn.1000077e0
+│           0x100004858      800605a9       stp x0, x1, [x20, 0x50]
+│           0x10000485c      48000090       adrp x8, reloc.Foundation.__DataStorage._bytes.allocator__UnsafeMutableRawPointer______ ; 0x10000c000
+│           0x100004860      089d41f9       ldr x8, reloc.Swift.Int    ; 0x10000c338
+│           0x100004864      883e00f9       str x8, [x20, 0x78]
+│           0x100004868      08808052       mov w8, 0x400
+│           0x10000486c      883200f9       str x8, [x20, 0x60]
+...
+│           0x100004998      f40300aa       mov x20, x0
+│           0x10000499c      61620391       add x1, x19, 0xd8
+│           0x1000049a0      c30b0094       bl sym.imp.SecKeyCreateRandomKey
+│           0x1000049a4      fb0300aa       mov x27, x0
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/run.sh b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/run.sh
@@ -0,0 +1 @@
+r2 -q -i security_keysize.r2 -A MASTestApp
diff --git a/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/security_keysize.r2 b/demos/ios/MASVS-CRYPTO/MASTG-DEMO-0011/security_keysize.r2
@@ -0,0 +1,23 @@
+!printf "\n\n"
+
+!printf "Uses of SecKeyCreateRandomKey:\n"
+afl~SecKeyCreateRandomKey
+
+!printf "\n"
+
+!printf "xrefs to SecKeyCreateRandomKey:\n"
+axt @ 0x1000078ac
+
+!printf "\n"
+
+!printf "Use of reloc.kSecAttrKeySizeInBits as input for SecKeyCreateRandomKey:\n"
+pd 1 @ sym.func.1000046f8
+
+!printf "...\n"
+
+pd 9 @ 0x10000484c
+
+!printf "...\n"
+
+pd-- 2 @ 0x1000049a0
+
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		NO_COLOR=true semgrep -c ../../../../rules/mastg-android-weak_key_generation.yml ./MastgTest_reversed.java --text -o output.txt