diff --git a/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.qhelp b/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.qhelp new file mode 100644 index 000000000000..eb211f508da7 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.qhelp @@ -0,0 +1,23 @@ + + + +

Storing cryptographic hashes of passwords is standard security practice, but it is equally important to select the right hashing scheme. If an attacker obtains the hashed passwords of an application, the password hashing scheme should still prevent the attacker from easily obtaining the original cleartext passwords.

+

A good password hashing scheme requires a computation that cannot be done efficiently. Hashing schemes with low number of iterations are efficiently computable, and are therefore not suitable for password hashing.

+ + + +

Use the OWASP recommendation for sufficient number of iterations (currently, that is greater than or equal to 120,000) for password hashing schemes.

+ + + +

The following example shows a few cases where a password hashing scheme is instantiated. In the 'BAD' cases, the scheme is initialized with insufficient iterations, making it susceptible to password cracking attacks. In the 'GOOD' cases, the scheme is initialized with at least 120,000 iterations, which protects the hashed data against recovery.

+ + + + +
  • Password-Based Cryptography Specification Version 2.0. 2000.RFC2898.
    • +
  • OWASP Password Storage Cheat Sheet.
    • +     +     diff --git a/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.ql b/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.ql new file mode 100644 index 000000000000..046c86d2f5cc --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.ql @@ -0,0 +1,68 @@ +/** + * @name Insufficient hash iterations + * @description Using hash functions with fewer than 120,000 iterations is insufficient to protect passwords because a cracking attack will require a low level of computational effort. + * @kind path-problem + * @problem.severity error + * @security-severity 7.8 + * @precision high + * @id swift/insufficient-hash-iterations + * @tags security + * external/cwe/cwe-916 + */ + +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.dataflow.TaintTracking +import DataFlow::PathGraph + +/** + * An `Expr` that is used to initialize a password-based encryption key. + */ +abstract class IterationsSource extends Expr { } + +/** + * A literal integer that is 120,000 or less is a source of taint for iterations. + */ +class IntLiteralSource extends IterationsSource instanceof IntegerLiteralExpr { + IntLiteralSource() { this.getStringValue().toInt() < 120000 } +} + +/** + * A class for all ways to set the iterations of hash function. + */ +class InsufficientHashIterationsSink extends Expr { + InsufficientHashIterationsSink() { + // `iterations` arg in `init` is a sink + exists(ClassOrStructDecl c, AbstractFunctionDecl f, CallExpr call, int arg | + c.getFullName() = ["PBKDF1", "PBKDF2"] and + c.getAMember() = f and + f.getName().matches("init(%iterations:%") and + call.getStaticTarget() = f and + f.getParam(pragma[only_bind_into](arg)).getName() = "iterations" and + call.getArgument(pragma[only_bind_into](arg)).getExpr() = this + ) + } +} + +/** + * A dataflow configuration from the hash iterations source to expressions that use + * it to initialize hash functions. + */ +class InsufficientHashIterationsConfig extends TaintTracking::Configuration { + InsufficientHashIterationsConfig() { this = "InsufficientHashIterationsConfig" } + + override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof IterationsSource } + + override predicate isSink(DataFlow::Node node) { + node.asExpr() instanceof InsufficientHashIterationsSink + } +} + +// The query itself +from + InsufficientHashIterationsConfig config, DataFlow::PathNode sourceNode, + DataFlow::PathNode sinkNode +where config.hasFlowPath(sourceNode, sinkNode) +select sinkNode.getNode(), sourceNode, sinkNode, + "The value '" + sourceNode.getNode().toString() + + "' is an insufficient number of iterations for secure password hashing." diff --git a/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.swift b/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.swift new file mode 100644 index 000000000000..fb712f706e33 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.swift @@ -0,0 +1,14 @@ + +func hash() { + // ... + + // BAD: Using insufficient (that is, < 120,000) iterations for password hashing + _ = try PKCS5.PBKDF1(password: getRandomArray(), salt: getRandomArray(), iterations: 90000, keyLength: 0) + _ = try PKCS5.PBKDF2(password: getRandomArray(), salt: getRandomArray(), iterations: 90000, keyLength: 0) + + // GOOD: Using sufficient (that is, >= 120,000) iterations for password hashing + _ = try PKCS5.PBKDF1(password: getRandomArray(), salt: getRandomArray(), iterations: 120120, keyLength: 0) + _ = try PKCS5.PBKDF2(password: getRandomArray(), salt: getRandomArray(), iterations: 310000, keyLength: 0) + + // ... +} diff --git a/swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.expected b/swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.expected new file mode 100644 index 000000000000..0e46106d3f21 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.expected @@ -0,0 +1,17 @@ +edges +| test.swift:20:45:20:45 | 99999 : | test.swift:33:22:33:43 | call to getLowIterationCount() : | +| test.swift:33:22:33:43 | call to getLowIterationCount() : | test.swift:37:84:37:84 | lowIterations | +| test.swift:33:22:33:43 | call to getLowIterationCount() : | test.swift:44:84:44:84 | lowIterations | +nodes +| test.swift:20:45:20:45 | 99999 : | semmle.label | 99999 : | +| test.swift:33:22:33:43 | call to getLowIterationCount() : | semmle.label | call to getLowIterationCount() : | +| test.swift:37:84:37:84 | lowIterations | semmle.label | lowIterations | +| test.swift:38:84:38:84 | 80000 | semmle.label | 80000 | +| test.swift:44:84:44:84 | lowIterations | semmle.label | lowIterations | +| test.swift:45:84:45:84 | 80000 | semmle.label | 80000 | +subpaths +#select +| test.swift:37:84:37:84 | lowIterations | test.swift:20:45:20:45 | 99999 : | test.swift:37:84:37:84 | lowIterations | The value '99999' is an insufficient number of iterations for secure password hashing. | +| test.swift:38:84:38:84 | 80000 | test.swift:38:84:38:84 | 80000 | test.swift:38:84:38:84 | 80000 | The value '80000' is an insufficient number of iterations for secure password hashing. | +| test.swift:44:84:44:84 | lowIterations | test.swift:20:45:20:45 | 99999 : | test.swift:44:84:44:84 | lowIterations | The value '99999' is an insufficient number of iterations for secure password hashing. | +| test.swift:45:84:45:84 | 80000 | test.swift:45:84:45:84 | 80000 | test.swift:45:84:45:84 | 80000 | The value '80000' is an insufficient number of iterations for secure password hashing. | \ No newline at end of file diff --git a/swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.qlref b/swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.qlref new file mode 100644 index 000000000000..81a6dda0d0f0 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.qlref @@ -0,0 +1 @@ +queries/Security/CWE-916/InsufficientHashIterations.ql diff --git a/swift/ql/test/query-tests/Security/CWE-916/test.swift b/swift/ql/test/query-tests/Security/CWE-916/test.swift new file mode 100644 index 000000000000..8786d936c1d3 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-916/test.swift @@ -0,0 +1,48 @@ + +// --- stubs --- + +// These stubs roughly follows the same structure as classes from CryptoSwift +enum PKCS5 { } + +enum Variant { case md5, sha1, sha2, sha3 } + +extension PKCS5 { + struct PBKDF1 { + init(password: Array, salt: Array, variant: Variant = .sha1, iterations: Int = 4096, keyLength: Int? = nil) { } + } + + struct PBKDF2 { + init(password: Array, salt: Array, iterations: Int = 4096, keyLength: Int? = nil, variant: Variant = .sha2) { } + } +} + +// Helper functions +func getLowIterationCount() -> Int { return 99999 } + +func getEnoughIterationCount() -> Int { return 120120 } + +func getRandomArray() -> Array { + (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) +} + +// --- tests --- + +func test() { + let randomArray = getRandomArray() + let variant = Variant.sha2 + let lowIterations = getLowIterationCount() + let enoughIterations = getEnoughIterationCount() + + // PBKDF1 test cases + let pbkdf1b1 = PKCS5.PBKDF1(password: randomArray, salt: randomArray, iterations: lowIterations, keyLength: 0) // BAD + let pbkdf1b2 = PKCS5.PBKDF1(password: randomArray, salt: randomArray, iterations: 80000, keyLength: 0) // BAD + let pbkdf1g1 = PKCS5.PBKDF1(password: randomArray, salt: randomArray, iterations: enoughIterations, keyLength: 0) // GOOD + let pbkdf1g2 = PKCS5.PBKDF1(password: randomArray, salt: randomArray, iterations: 120120, keyLength: 0) // GOOD + + + // PBKDF2 test cases + let pbkdf2b1 = PKCS5.PBKDF2(password: randomArray, salt: randomArray, iterations: lowIterations, keyLength: 0) // BAD + let pbkdf2b2 = PKCS5.PBKDF2(password: randomArray, salt: randomArray, iterations: 80000, keyLength: 0) // BAD + let pbkdf2g1 = PKCS5.PBKDF2(password: randomArray, salt: randomArray, iterations: enoughIterations, keyLength: 0) // GOOD + let pbkdf2g2 = PKCS5.PBKDF2(password: randomArray, salt: randomArray, iterations: 120120, keyLength: 0) // GOOD +}