Update OS Check from Review Feedback

java/ql/lib/semmle/code/java/JDK.qll

@@ -37,6 +37,27 @@ class StringLengthMethod extends Method {
   StringLengthMethod() { this.hasName("length") and this.getDeclaringType() instanceof TypeString }
 }
 
+/**
+ * The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char.
+ */
+class StringPartialMatchMethod extends Method {
+  StringPartialMatchMethod() {
+    this.hasName([
+        "contains", "startsWith", "endsWith", "matches", "indexOf", "lastIndexOf", "regionMatches"
+      ]) and
+    this.getDeclaringType() instanceof TypeString
+  }
+
+  /**
+   * The index of the parameter that is being matched against.
+   */
+  int getMatchParameterIndex() {
+    if not this.hasName("regionMatches")
+    then result = 0
+    else this.getParameterType(result) instanceof TypeString
+  }
+}
+
 /** The class `java.lang.StringBuffer`. */
 class TypeStringBuffer extends Class {
   TypeStringBuffer() { this.hasQualifiedName("java.lang", "StringBuffer") }

java/ql/lib/semmle/code/java/StringCheck.qll

@@ -1,14 +0,0 @@
-/**
- * Provides classes and predicates for reasoning about checking characterizations about strings.
- */
-
-import java
-
-/**
- * Holds if `ma` is a call to a method that checks a partial string match.
- */
-predicate isStringPartialMatch(MethodAccess ma) {
-    ma.getMethod().getDeclaringType() instanceof TypeString and
-    ma.getMethod().getName() =
-      ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"]
-  }

java/ql/lib/semmle/code/java/os/OSCheck.qll

@@ -6,7 +6,6 @@ import java
 import semmle.code.java.controlflow.Guards
 private import semmle.code.java.frameworks.apache.Lang
 private import semmle.code.java.dataflow.DataFlow
-private import semmle.code.java.StringCheck
 
 /**
  * A guard that checks if the current platform is Windows.
@@ -21,34 +20,36 @@ abstract class IsUnixGuard extends Guard { }
 /**
  * Holds when the MethodAccess is a call to check the current OS using either the upper case `osUpperCase` or lower case `osLowerCase` string constants.
  */
-bindingset[osUpperCase, osLowerCase]
-private predicate isOsFromSystemProp(MethodAccess ma, string osUpperCase, string osLowerCase) {
-  exists(MethodAccessSystemGetProperty sgpMa |
+bindingset[osString]
+private predicate isOsFromSystemProp(MethodAccess ma, string osString) {
+  exists(MethodAccessSystemGetProperty sgpMa, Expr sgpFlowsToExpr |
     sgpMa.hasCompileTimeConstantGetPropertyName("os.name")
   |
-    DataFlow::localExprFlow(sgpMa, ma.getQualifier()) and // Call from System.getProperty to some partial match method
-    ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().matches(osUpperCase)
-    or
-    exists(MethodAccess toLowerCaseMa |
-      toLowerCaseMa.getMethod() =
-        any(Method m | m.getDeclaringType() instanceof TypeString and m.hasName("toLowerCase"))
-    |
-      DataFlow::localExprFlow(sgpMa, toLowerCaseMa.getQualifier()) and // Call from System.getProperty to toLowerCase
-      DataFlow::localExprFlow(toLowerCaseMa, ma.getQualifier()) and // Call from toLowerCase to some partial match method
-      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().matches(osLowerCase)
+    DataFlow::localExprFlow(sgpMa, sgpFlowsToExpr) and
+    ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase().matches(osString) and // Call from System.getProperty to some partial match method
+    (
+      sgpFlowsToExpr = ma.getQualifier()
+      or
+      exists(MethodAccess caseChangeMa |
+        caseChangeMa.getMethod() =
+          any(Method m |
+            m.getDeclaringType() instanceof TypeString and m.hasName(["toLowerCase", "toUpperCase"])
+          )
+      |
+        sgpFlowsToExpr = caseChangeMa.getQualifier() and // Call from System.getProperty to case-switching method
+        DataFlow::localExprFlow(caseChangeMa, ma.getQualifier()) // Call from case-switching method to some partial match method
+      )
     )
   ) and
-  isStringPartialMatch(ma)
+  ma.getMethod() instanceof StringPartialMatchMethod
 }
 
 private class IsWindowsFromSystemProp extends IsWindowsGuard instanceof MethodAccess {
-  IsWindowsFromSystemProp() { isOsFromSystemProp(this, "Window%", "window%") }
+  IsWindowsFromSystemProp() { isOsFromSystemProp(this, "window%") }
 }
 
 private class IsUnixFromSystemProp extends IsUnixGuard instanceof MethodAccess {
-  IsUnixFromSystemProp() {
-    isOsFromSystemProp(this, ["Mac%", "Linux%", "LINUX%"], ["mac%", "linux%"])
-  }
+  IsUnixFromSystemProp() { isOsFromSystemProp(this, ["mac%", "linux%"]) }
 }
 
 bindingset[fieldNamePattern]
@@ -70,7 +71,7 @@ private class IsUnixFromApacheCommons extends IsUnixGuard instanceof FieldAccess
 /**
  * A guard that checks if the `java.nio.file.FileSystem` supports posix file permissions.
  * This is often used to infer if the OS is unix-based.
- * Looks for calls to `contains("posix")` on the `supportedFileAttributeViews` method returned by `FileSystem`.
+ * Looks for calls to `contains("posix")` on the `supportedFileAttributeViews()` method returned by `FileSystem`.
  */
 private class IsUnixFromPosixFromFileSystem extends IsUnixGuard instanceof MethodAccess {
   IsUnixFromPosixFromFileSystem() {

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

@@ -104,17 +104,17 @@ private class FileCreateTempFileSink extends FileCreationSink {
 }
 
 /**
- * A guard that checks what OS the program is running on.
+ * A guard that checks whether or not the the program is running on the Windows OS.
  */
-abstract private class OsBarrierGuard extends DataFlow::BarrierGuard { }
+abstract private class WindowsOsBarrierGuard extends DataFlow::BarrierGuard { }
 
-private class IsUnixBarrierGuard extends OsBarrierGuard instanceof IsUnixGuard {
+private class IsNotUnixBarrierGuard extends WindowsOsBarrierGuard instanceof IsUnixGuard {
   override predicate checks(Expr e, boolean branch) {
     this.controls(e.getBasicBlock(), branch.booleanNot())
   }
 }
 
-private class IsWindowsBarrierGuard extends OsBarrierGuard instanceof IsWindowsGuard {
+private class IsWindowsBarrierGuard extends WindowsOsBarrierGuard instanceof IsWindowsGuard {
   override predicate checks(Expr e, boolean branch) { this.controls(e.getBasicBlock(), branch) }
 }
 
@@ -147,7 +147,7 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
   }
 
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof OsBarrierGuard
+    guard instanceof WindowsOsBarrierGuard
   }
 }
 

java/ql/test/library-tests/JDK/PrintAst.expected

@@ -60,6 +60,75 @@ jdk/A.java:
 #   28|             0: [ArrayTypeAccess] ...[]
 #   28|               0: [TypeAccess] String
 #   28|       5: [BlockStmt] { ... }
+jdk/StringMatch.java:
+#    0| [CompilationUnit] StringMatch
+#    1|   1: [Class] StringMatch
+#    2|     3: [FieldDeclaration] String STR;
+#    2|       -1: [TypeAccess] String
+#    2|       0: [StringLiteral] "the quick brown fox jumps over the lazy dog"
+#    4|     4: [Method] a
+#    4|       3: [TypeAccess] void
+#    4|       5: [BlockStmt] { ... }
+#    5|         0: [ExprStmt] <Expr>;
+#    5|           0: [MethodAccess] matches(...)
+#    5|             -1: [VarAccess] STR
+#    5|             0: [StringLiteral] "[a-z]+"
+#    8|     5: [Method] b
+#    8|       3: [TypeAccess] void
+#    8|       5: [BlockStmt] { ... }
+#    9|         0: [ExprStmt] <Expr>;
+#    9|           0: [MethodAccess] contains(...)
+#    9|             -1: [VarAccess] STR
+#    9|             0: [StringLiteral] "the"
+#   12|     6: [Method] c
+#   12|       3: [TypeAccess] void
+#   12|       5: [BlockStmt] { ... }
+#   13|         0: [ExprStmt] <Expr>;
+#   13|           0: [MethodAccess] startsWith(...)
+#   13|             -1: [VarAccess] STR
+#   13|             0: [StringLiteral] "the"
+#   16|     7: [Method] d
+#   16|       3: [TypeAccess] void
+#   16|       5: [BlockStmt] { ... }
+#   17|         0: [ExprStmt] <Expr>;
+#   17|           0: [MethodAccess] endsWith(...)
+#   17|             -1: [VarAccess] STR
+#   17|             0: [StringLiteral] "dog"
+#   20|     8: [Method] e
+#   20|       3: [TypeAccess] void
+#   20|       5: [BlockStmt] { ... }
+#   21|         0: [ExprStmt] <Expr>;
+#   21|           0: [MethodAccess] indexOf(...)
+#   21|             -1: [VarAccess] STR
+#   21|             0: [StringLiteral] "lazy"
+#   24|     9: [Method] f
+#   24|       3: [TypeAccess] void
+#   24|       5: [BlockStmt] { ... }
+#   25|         0: [ExprStmt] <Expr>;
+#   25|           0: [MethodAccess] lastIndexOf(...)
+#   25|             -1: [VarAccess] STR
+#   25|             0: [StringLiteral] "lazy"
+#   28|     10: [Method] g
+#   28|       3: [TypeAccess] void
+#   28|       5: [BlockStmt] { ... }
+#   29|         0: [ExprStmt] <Expr>;
+#   29|           0: [MethodAccess] regionMatches(...)
+#   29|             -1: [VarAccess] STR
+#   29|             0: [IntegerLiteral] 0
+#   29|             1: [StringLiteral] "fox"
+#   29|             2: [IntegerLiteral] 0
+#   29|             3: [IntegerLiteral] 4
+#   32|     11: [Method] h
+#   32|       3: [TypeAccess] void
+#   32|       5: [BlockStmt] { ... }
+#   33|         0: [ExprStmt] <Expr>;
+#   33|           0: [MethodAccess] regionMatches(...)
+#   33|             -1: [VarAccess] STR
+#   33|             0: [BooleanLiteral] true
+#   33|             1: [IntegerLiteral] 0
+#   33|             2: [StringLiteral] "FOX"
+#   33|             3: [IntegerLiteral] 0
+#   33|             4: [IntegerLiteral] 4
 jdk/SystemGetPropertyCall.java:
 #    0| [CompilationUnit] SystemGetPropertyCall
 #    3|   1: [Class] SystemGetPropertyCall

java/ql/test/library-tests/JDK/StringMatch.expected

@@ -0,0 +1,8 @@
+| jdk/StringMatch.java:5:9:5:29 | matches(...) | jdk/StringMatch.java:5:21:5:28 | "[a-z]+" |
+| jdk/StringMatch.java:9:9:9:27 | contains(...) | jdk/StringMatch.java:9:22:9:26 | "the" |
+| jdk/StringMatch.java:13:9:13:29 | startsWith(...) | jdk/StringMatch.java:13:24:13:28 | "the" |
+| jdk/StringMatch.java:17:9:17:27 | endsWith(...) | jdk/StringMatch.java:17:22:17:26 | "dog" |
+| jdk/StringMatch.java:21:9:21:27 | indexOf(...) | jdk/StringMatch.java:21:21:21:26 | "lazy" |
+| jdk/StringMatch.java:25:9:25:31 | lastIndexOf(...) | jdk/StringMatch.java:25:25:25:30 | "lazy" |
+| jdk/StringMatch.java:29:9:29:41 | regionMatches(...) | jdk/StringMatch.java:29:30:29:34 | "fox" |
+| jdk/StringMatch.java:33:9:33:47 | regionMatches(...) | jdk/StringMatch.java:33:36:33:40 | "FOX" |

java/ql/test/library-tests/JDK/StringMatch.ql

@@ -0,0 +1,5 @@
+import java
+
+from MethodAccess ma, StringPartialMatchMethod m
+where ma.getMethod() = m
+select ma, ma.getArgument(m.getMatchParameterIndex())

java/ql/test/library-tests/JDK/jdk/StringMatch.java

@@ -0,0 +1,35 @@
+public class StringMatch {
+    private static String STR = "the quick brown fox jumps over the lazy dog";
+
+    void a() {
+        STR.matches("[a-z]+");
+    }
+
+    void b() {
+        STR.contains("the");
+    }
+
+    void c() {
+        STR.startsWith("the");
+    }
+
+    void d() {
+        STR.endsWith("dog");
+    }
+
+    void e() {
+        STR.indexOf("lazy");
+    }
+
+    void f() {
+        STR.lastIndexOf("lazy");
+    }
+
+    void g() {
+        STR.regionMatches(0, "fox", 0, 4);
+    }
+
+    void h() {
+        STR.regionMatches(true, 0, "FOX", 0, 4);
+    }
+}