PS: Weak kdf query

powershell/ql/src/queries/security/cwe-327/WeakKDFConfiguration.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+When deriving cryptographic keys from passwords using <code>Rfc2898DeriveBytes</code> (PBKDF2),
+both the iteration count and hash algorithm must be configured securely.
+An insufficient iteration count or a weak hash algorithm makes the derived key
+vulnerable to brute-force attacks.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Always specify at least 100,000 iterations and use SHA-256 or a stronger hash algorithm
+(SHA-384, SHA-512) when creating an <code>Rfc2898DeriveBytes</code> instance or calling the
+static <code>Pbkdf2</code> method.
+</p>
+</recommendation>
+
+<example>
+<p>The following example shows insecure usage with default settings:</p>
+<sample src="examples/WeakKDFConfigurationBad.ps1" />
+<p>The following example shows secure usage with adequate iterations and a strong hash:</p>
+<sample src="examples/WeakKDFConfigurationGood.ps1" />
+</example>
+
+<references>
+<li>
+  OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html">Password Storage Cheat Sheet</a>
+</li>
+<li>
+  Microsoft: <a href="https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.rfc2898derivebytes">Rfc2898DeriveBytes Class</a>
+</li>
+<li>
+  CWE-327: <a href="https://cwe.mitre.org/data/definitions/327.html">Use of a Broken or Risky Cryptographic Algorithm</a>
+</li>
+</references>
+</qhelp>

powershell/ql/src/queries/security/cwe-327/WeakKDFConfiguration.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Weak key derivation function configuration
+ * @description Rfc2898DeriveBytes (PBKDF2) should use at least 100,000 iterations
+ *              and a hash algorithm of SHA-256 or stronger to resist brute-force attacks.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id powershell/weak-kdf-configuration
+ * @tags security
+ *       external/cwe/cwe-327
+ *       external/cwe/cwe-328
+ *       cryptography
+ */
+
+import powershell
+import WeakKDFConfiguration
+
+from WeakKdfConfig config
+select config, config.getMessage()

powershell/ql/src/queries/security/cwe-327/WeakKDFConfiguration.qll

@@ -0,0 +1,238 @@
+/**
+ * Provides classes and predicates for reasoning about weak key derivation
+ * function (KDF) configurations using `Rfc2898DeriveBytes` (PBKDF2).
+ */
+
+import powershell
+import semmle.code.powershell.ApiGraphs
+import semmle.code.powershell.dataflow.DataFlow
+
+/** Gets the minimum recommended PBKDF2 iteration count. */
+int minIterationCount() { result = 100000 }
+
+/** Gets the `System.Security.Cryptography` namespace. */
+private API::Node cryptographyNamespace() {
+  result =
+    API::getTopLevelMember("system").getMember("security").getMember("cryptography")
+}
+
+/** Gets the `System.Security.Cryptography.Rfc2898DeriveBytes` type. */
+private API::Node rfc2898DeriveBytesType() {
+  result = cryptographyNamespace().getMember("rfc2898derivebytes")
+}
+
+/**
+ * An instantiation of Rfc2898DeriveBytes via New-Object or [Type]::new().
+ */
+class Rfc2898DeriveBytesCreation extends DataFlow::CallNode {
+  Rfc2898DeriveBytesCreation() {
+    this = rfc2898DeriveBytesType().getMember("new").asCall()
+    or
+    // New-Object pattern
+    exists(DataFlow::ObjectCreationNode oc |
+      oc = this and
+      oc.getLowerCaseConstructedTypeName() =
+        [
+          "system.security.cryptography.rfc2898derivebytes",
+          "rfc2898derivebytes"
+        ]
+    )
+  }
+
+  private DataFlow::Node getNewObjectArgumentList() {
+    this.getExprNode().getExpr() instanceof DotNetObjectCreation and
+    (
+      result = this.getNamedArgument("argumentlist")
+      or
+      not this.hasNamedArgument("argumentlist") and result = this.getPositionalArgument(1)
+    )
+  }
+
+  private DataFlow::Node getNewObjectArgument(int index) {
+    exists(ArrayLiteral args |
+      args = this.getNewObjectArgumentList().asExpr().getExpr() and
+      result.asExpr().getExpr() = args.getExpr(index)
+    )
+    or
+    exists(ParenExpr paren, ArrayLiteral args |
+      paren = this.getNewObjectArgumentList().asExpr().getExpr() and
+      args = paren.getExpr() and
+      result.asExpr().getExpr() = args.getExpr(index)
+    )
+  }
+
+  private predicate hasKnownNewObjectArgumentList() {
+    this.getNewObjectArgumentList().asExpr().getExpr() instanceof ArrayLiteral
+    or
+    this.getNewObjectArgumentList().asExpr().getExpr().(ParenExpr).getExpr() instanceof ArrayLiteral
+  }
+
+  private DataFlow::Node getConstructorArgument(int index) {
+    this.getExprNode().getExpr() instanceof NewObjectCreation and
+    result = this.getPositionalArgument(index)
+    or
+    result = this.getNewObjectArgument(index)
+  }
+
+  /** Gets the iteration count argument (position 2, 0-indexed), if any. */
+  DataFlow::Node getIterationCountArg() { result = this.getConstructorArgument(2) }
+
+  /** Gets the hash algorithm argument (position 3, 0-indexed), if any. */
+  DataFlow::Node getHashAlgorithmArg() { result = this.getConstructorArgument(3) }
+
+  /** Holds if the constructor is known to omit the iteration count argument. */
+  predicate hasDefaultIterationCount() {
+    not this.getExprNode().getExpr() instanceof DotNetObjectCreation and
+    not exists(this.getIterationCountArg())
+    or
+    this.hasKnownNewObjectArgumentList() and not exists(this.getNewObjectArgument(2))
+  }
+
+  /** Holds if the constructor is known to omit the hash algorithm argument. */
+  predicate hasDefaultHashAlgorithm() {
+    not this.getExprNode().getExpr() instanceof DotNetObjectCreation and
+    not exists(this.getHashAlgorithmArg())
+    or
+    this.hasKnownNewObjectArgumentList() and not exists(this.getNewObjectArgument(3))
+  }
+}
+
+/**
+ * A call to the static Rfc2898DeriveBytes.Pbkdf2 method (.NET 6+).
+ */
+class Pbkdf2StaticCall extends DataFlow::CallNode {
+  Pbkdf2StaticCall() { this = rfc2898DeriveBytesType().getMember("pbkdf2").asCall() }
+
+  /** Gets the iteration count argument (position 2, 0-indexed). */
+  DataFlow::Node getIterationCountArg() { result = this.getPositionalArgument(2) }
+
+  /** Gets the hash algorithm argument (position 3, 0-indexed). */
+  DataFlow::Node getHashAlgorithmArg() { result = this.getPositionalArgument(3) }
+}
+
+/**
+ * Holds if `node` is an integer literal less than the minimum iteration count.
+ */
+predicate isLowIterationValue(DataFlow::Node node, int value) {
+  value = node.asExpr().getExpr().getValue().asInt() and
+  value < minIterationCount()
+}
+
+/**
+ * Holds if `node` references a weak hash algorithm (MD5 or SHA1).
+ */
+predicate isWeakHashAlgorithm(DataFlow::Node node, string name) {
+  // [HashAlgorithmName]::MD5 or [HashAlgorithmName]::SHA1
+  node = cryptographyNamespace().getMember("hashalgorithmname").getMember(name).asSource() and
+  name = ["md5", "sha1"]
+  or
+  // String literal "MD5" or "SHA1"
+  exists(string s |
+    s = node.asExpr().getExpr().getValue().asString().toLowerCase() and
+    s = ["md5", "sha1"] and
+    name = s
+  )
+}
+
+/**
+ * A weak key derivation function configuration that should be reported.
+ */
+abstract class WeakKdfConfig extends DataFlow::CallNode {
+  abstract string getMessage();
+}
+
+/**
+ * Rfc2898DeriveBytes created without specifying an iteration count.
+ */
+class DefaultIterationCountConfig extends WeakKdfConfig, Rfc2898DeriveBytesCreation {
+  DefaultIterationCountConfig() { this.hasDefaultIterationCount() }
+
+  override string getMessage() {
+    result =
+      "Rfc2898DeriveBytes uses default iteration count of 1000. Specify at least " +
+        minIterationCount().toString() + " iterations."
+  }
+}
+
+/**
+ * Rfc2898DeriveBytes created with a low iteration count.
+ */
+class LowIterationCountConfig extends WeakKdfConfig, Rfc2898DeriveBytesCreation {
+  LowIterationCountConfig() {
+    isLowIterationValue(this.getIterationCountArg(), _)
+  }
+
+  override string getMessage() {
+    exists(int value |
+      isLowIterationValue(this.getIterationCountArg(), value) and
+      result =
+        "Rfc2898DeriveBytes uses iteration count of " + value.toString() +
+          ", which is below the minimum of " + minIterationCount().toString() + "."
+    )
+  }
+}
+
+/**
+ * Rfc2898DeriveBytes created without specifying a hash algorithm (defaults to SHA1).
+ */
+class DefaultHashAlgorithmConfig extends WeakKdfConfig, Rfc2898DeriveBytesCreation {
+  DefaultHashAlgorithmConfig() { this.hasDefaultHashAlgorithm() }
+
+  override string getMessage() {
+    result = "Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger."
+  }
+}
+
+/**
+ * Rfc2898DeriveBytes created with a weak hash algorithm.
+ */
+class WeakHashAlgorithmConfig extends WeakKdfConfig, Rfc2898DeriveBytesCreation {
+  WeakHashAlgorithmConfig() {
+    isWeakHashAlgorithm(this.getHashAlgorithmArg(), _)
+  }
+
+  override string getMessage() {
+    exists(string name |
+      isWeakHashAlgorithm(this.getHashAlgorithmArg(), name) and
+      result =
+        "Rfc2898DeriveBytes uses weak hash algorithm " + name.toUpperCase() +
+          ". Use SHA-256 or stronger."
+    )
+  }
+}
+
+/**
+ * Rfc2898DeriveBytes.Pbkdf2 called with a low iteration count.
+ */
+class Pbkdf2LowIterationCountConfig extends WeakKdfConfig, Pbkdf2StaticCall {
+  Pbkdf2LowIterationCountConfig() {
+    isLowIterationValue(this.getIterationCountArg(), _)
+  }
+
+  override string getMessage() {
+    exists(int value |
+      isLowIterationValue(this.getIterationCountArg(), value) and
+      result =
+        "Rfc2898DeriveBytes.Pbkdf2 uses iteration count of " + value.toString() +
+          ", which is below the minimum of " + minIterationCount().toString() + "."
+    )
+  }
+}
+
+/**
+ * Rfc2898DeriveBytes.Pbkdf2 called with a weak hash algorithm.
+ */
+class Pbkdf2WeakHashAlgorithmConfig extends WeakKdfConfig, Pbkdf2StaticCall {
+  Pbkdf2WeakHashAlgorithmConfig() {
+    isWeakHashAlgorithm(this.getHashAlgorithmArg(), _)
+  }
+
+  override string getMessage() {
+    exists(string name |
+      isWeakHashAlgorithm(this.getHashAlgorithmArg(), name) and
+      result =
+        "Rfc2898DeriveBytes.Pbkdf2 uses weak hash algorithm " + name.toUpperCase() +
+          ". Use SHA-256 or stronger."
+    )
+  }
+}

powershell/ql/src/queries/security/cwe-327/examples/WeakKDFConfigurationBad.ps1

@@ -0,0 +1,8 @@
+# BAD: Default iteration count (1000) and default hash algorithm (SHA1)
+$kdf = [System.Security.Cryptography.Rfc2898DeriveBytes]::new($password, $salt)
+
+# BAD: Low iteration count
+$kdf = [System.Security.Cryptography.Rfc2898DeriveBytes]::new($password, $salt, 1000)
+
+# BAD: Weak hash algorithm
+$kdf = [System.Security.Cryptography.Rfc2898DeriveBytes]::new($password, $salt, 100000, [System.Security.Cryptography.HashAlgorithmName]::SHA1)

powershell/ql/src/queries/security/cwe-327/examples/WeakKDFConfigurationGood.ps1

@@ -0,0 +1,5 @@
+# GOOD: 100,000+ iterations with SHA-256
+$kdf = [System.Security.Cryptography.Rfc2898DeriveBytes]::new($password, $salt, 600000, [System.Security.Cryptography.HashAlgorithmName]::SHA256)
+
+# GOOD: Static Pbkdf2 with strong configuration
+$key = [System.Security.Cryptography.Rfc2898DeriveBytes]::Pbkdf2($password, $salt, 600000, "SHA256", 32)

powershell/ql/test/query-tests/security/cwe-327/WeakKDFConfiguration/WeakKDFConfiguration.expected

@@ -0,0 +1,25 @@
+| WeakKDFConfiguration.ps1:7:8:7:79 | Call to new | Rfc2898DeriveBytes uses default iteration count of 1000. Specify at least 100000 iterations. |
+| WeakKDFConfiguration.ps1:7:8:7:79 | Call to new | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:11:8:11:85 | Call to new | Rfc2898DeriveBytes uses iteration count of 1000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:11:8:11:85 | Call to new | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:14:8:14:86 | Call to new | Rfc2898DeriveBytes uses iteration count of 10000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:14:8:14:86 | Call to new | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:17:8:17:86 | Call to new | Rfc2898DeriveBytes uses iteration count of 50000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:17:8:17:86 | Call to new | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:21:8:21:87 | Call to new | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:25:8:25:143 | Call to new | Rfc2898DeriveBytes uses weak hash algorithm SHA1. Use SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:28:8:28:142 | Call to new | Rfc2898DeriveBytes uses weak hash algorithm MD5. Use SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:32:8:32:89 | Call to new-object | Rfc2898DeriveBytes uses iteration count of 5000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:32:8:32:89 | Call to new-object | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:36:8:36:83 | Call to new-object | Rfc2898DeriveBytes uses default iteration count of 1000. Specify at least 100000 iterations. |
+| WeakKDFConfiguration.ps1:36:8:36:83 | Call to new-object | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:39:8:41:1 | Call to new-object | Rfc2898DeriveBytes uses iteration count of 5000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:39:8:41:1 | Call to new-object | Rfc2898DeriveBytes uses weak hash algorithm SHA1. Use SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:42:8:44:1 | Call to new-object | Rfc2898DeriveBytes uses iteration count of 5000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:42:8:44:1 | Call to new-object | Rfc2898DeriveBytes uses weak hash algorithm SHA1. Use SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:48:8:48:102 | Call to pbkdf2 | Rfc2898DeriveBytes.Pbkdf2 uses iteration count of 1000, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:52:8:52:102 | Call to pbkdf2 | Rfc2898DeriveBytes.Pbkdf2 uses weak hash algorithm SHA1. Use SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:55:8:55:84 | Call to new | Rfc2898DeriveBytes uses iteration count of 500, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:55:8:55:84 | Call to new | Rfc2898DeriveBytes uses the default hash algorithm SHA1. Specify SHA-256 or stronger. |
+| WeakKDFConfiguration.ps1:58:8:58:140 | Call to new | Rfc2898DeriveBytes uses iteration count of 500, which is below the minimum of 100000. |
+| WeakKDFConfiguration.ps1:58:8:58:140 | Call to new | Rfc2898DeriveBytes uses weak hash algorithm SHA1. Use SHA-256 or stronger. |