Centralize command argument safety checks

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: owen-mc

java/ql/lib/semmle/code/java/security/CommandArguments.qll

@@ -13,66 +13,10 @@ import semmle.code.java.Collections
  * i.e. it's not in a position that selects a command to execute.
  */
 predicate isSafeCommandArgument(Expr ex) {
-  exists(ArrayInit ai, int i |
-    ex = ai.getInit(i) and
+  exists(CommandArguments arguments, int i |
     i > 0 and
-    not isShell(ai.getInit(0)) and
-    (
-      not exists(delegatedCommandIndex(ai.getInit(0)))
-      or
-      exists(int commandIndex |
-        commandIndex = delegatedCommandIndex(ai.getInit(0)) and
-        (
-          i < commandIndex
-          or
-          i > commandIndex and
-          not isShell(ai.getInit(commandIndex)) and
-          not exists(delegatedCommandIndex(ai.getInit(commandIndex)))
-        )
-      )
-    )
-  )
-  or
-  exists(CommandArgumentList cal, int i |
-    not cal.isShell() and
-    i > 0 and
-    ex = cal.getArgument(i) and
-    (
-      not exists(delegatedCommandIndex(cal.getArgument(0)))
-      or
-      exists(int commandIndex |
-        commandIndex = delegatedCommandIndex(cal.getArgument(0)) and
-        (
-          i < commandIndex
-          or
-          i > commandIndex and
-          exists(cal.getArgument(commandIndex)) and
-          not isShell(cal.getArgument(commandIndex)) and
-          not exists(delegatedCommandIndex(cal.getArgument(commandIndex)))
-        )
-      )
-    )
-  )
-  or
-  exists(CommandArgArrayImmutableFirst caa, int i |
-    not caa.isShell() and
-    i > 0 and
-    ex = caa.getAWrite(i) and
-    (
-      not exists(delegatedCommandIndex(caa.getFirstElement()))
-      or
-      exists(int commandIndex |
-        commandIndex = delegatedCommandIndex(caa.getFirstElement()) and
-        (
-          i < commandIndex
-          or
-          i > commandIndex and
-          exists(caa.getAnElement(commandIndex)) and
-          not isShell(caa.getAnElement(commandIndex)) and
-          not exists(delegatedCommandIndex(caa.getAnElement(commandIndex)))
-        )
-      )
-    )
+    ex = arguments.getArgumentUse(i) and
+    arguments.hasSafeArgument(i)
   )
 }
 
@@ -116,6 +60,66 @@ private int delegatedCommandIndex(Expr ex) {
   )
 }
 
+private newtype TCommandArguments =
+  TLiteralArguments(ArrayInit init) or
+  TListArguments(CommandArgumentList list) or
+  TMutableArguments(CommandArgArrayImmutableFirst array)
+
+/** An ordered sequence of command arguments. */
+private class CommandArguments extends TCommandArguments {
+  string toString() { result = this.getArgument(0).toString() }
+
+  /** Gets a possible value at the given index. */
+  private Expr getArgument(int index) {
+    exists(ArrayInit init | this = TLiteralArguments(init) | result = init.getInit(index))
+    or
+    exists(CommandArgumentList list |
+      this = TListArguments(list) and
+      result = list.getArgument(index)
+    )
+    or
+    exists(CommandArgArrayImmutableFirst array |
+      this = TMutableArguments(array) and
+      result = array.getAnElement(index)
+    )
+  }
+
+  /** Gets an expression whose value is stored at the given index. */
+  Expr getArgumentUse(int index) {
+    exists(ArrayInit init | this = TLiteralArguments(init) | result = init.getInit(index))
+    or
+    exists(CommandArgumentList list |
+      this = TListArguments(list) and
+      result = list.getArgument(index)
+    )
+    or
+    exists(CommandArgArrayImmutableFirst array |
+      this = TMutableArguments(array) and
+      result = array.getAWrite(index)
+    )
+  }
+
+  predicate hasSafeArgument(int index) {
+    exists(this.getArgumentUse(index)) and
+    not isShell(this.getArgument(0)) and
+    (
+      not exists(delegatedCommandIndex(this.getArgument(0)))
+      or
+      exists(int commandIndex |
+        commandIndex = delegatedCommandIndex(this.getArgument(0)) and
+        (
+          index < commandIndex
+          or
+          index > commandIndex and
+          exists(this.getArgument(commandIndex)) and
+          not isShell(this.getArgument(commandIndex)) and
+          not exists(delegatedCommandIndex(this.getArgument(commandIndex)))
+        )
+      )
+    )
+  }
+}
+
 /**
  * A type that could be a list of strings. Includes raw `List` types.
  */
@@ -184,9 +188,6 @@ private class CommandArgumentList extends SsaExplicitWrite {
 
   /** Gets the argument added at the given index. */
   Expr getArgument(int index) { result = this.getAnAddAtIndex(index).getArgument(0) }
-
-  /** Holds if the first element of this list is a shell command. */
-  predicate isShell() { isShell(this.getArgument(0)) }
 }
 
 /**
@@ -252,9 +253,6 @@ private class CommandArgArrayImmutableFirst extends CommandArgumentArray {
       result = arrayElementOf(this.getDefiningExpr(), index)
     )
   }
-
-  /** Holds if the first element of this array is a shell command. */
-  predicate isShell() { isShell(this.getFirstElement()) }
 }
 
 /** Gets an element of an immutable array of strings. */