Fix insufficient tests and add documentation

java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll

@@ -10,6 +10,7 @@ private import semmle.code.java.dataflow.DataFlow
  * ensuring that they are visible to the taint tracking library.
  */
 private module Frameworks {
+  private import semmle.code.java.JDK
   private import semmle.code.java.frameworks.jackson.JacksonSerializability
   private import semmle.code.java.frameworks.android.AsyncTask
   private import semmle.code.java.frameworks.android.Intent

java/ql/lib/semmle/code/java/environment/SystemProperty.qll

@@ -5,6 +5,8 @@ private import semmle.code.java.frameworks.apache.Lang
 
 /**
  * Gets an expression that retrieves the value of `propertyName` from `System.getProperty()`.
+ *
+ * Note: Expression type is not just `String`.
  */
 Expr getSystemProperty(string propertyName) {
   result = getSystemPropertyFromSystem(propertyName) or
@@ -20,8 +22,7 @@ Expr getSystemProperty(string propertyName) {
 private MethodAccess getSystemPropertyFromSystem(string propertyName) {
   result.(MethodAccessSystemGetProperty).hasCompileTimeConstantGetPropertyName(propertyName)
   or
-  result.getMethod().hasName("lineSeparator") and
-  propertyName = "line.separator"
+  result.getMethod().hasName("lineSeparator") and propertyName = "line.separator"
 }
 
 /**

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

@@ -57,35 +57,45 @@ private class IsWindowsFromSystemProp extends IsWindowsGuard instanceof MethodAc
 
 /**
  * Holds when the Guard is an equality check between the system property with the name `propertyName`
- * and the string or char constant `compareToLiteral`.
+ * and the string or char constant `compareToLiteral`, and the branch evaluates to `branch`.
  */
-private Guard isOsFromSystemPropertyEqualityCheck(string propertyName, string compareToLiteral) {
+private Guard isOsFromSystemPropertyEqualityCheck(
+  string propertyName, string compareToLiteral, boolean branch
+) {
   result
       .isEquality(getSystemProperty(propertyName),
         any(Literal literal |
           (literal instanceof CharacterLiteral or literal instanceof StringLiteral) and
           literal.getValue() = compareToLiteral
-        ), _)
+        ), branch)
 }
 
 private class IsWindowsFromCharPathSeparator extends IsWindowsGuard {
   IsWindowsFromCharPathSeparator() {
-    this = isOsFromSystemPropertyEqualityCheck("path.separator", "\\")
+    this = isOsFromSystemPropertyEqualityCheck("path.separator", ";", true) or
+    this = isOsFromSystemPropertyEqualityCheck("path.separator", ":", false)
   }
 }
 
 private class IsWindowsFromCharSeparator extends IsWindowsGuard {
-  IsWindowsFromCharSeparator() { this = isOsFromSystemPropertyEqualityCheck("file.separator", ";") }
+  IsWindowsFromCharSeparator() {
+    this = isOsFromSystemPropertyEqualityCheck("file.separator", "\\", true) or
+    this = isOsFromSystemPropertyEqualityCheck("file.separator", "/", false)
+  }
 }
 
 private class IsUnixFromCharPathSeparator extends IsUnixGuard {
   IsUnixFromCharPathSeparator() {
-    this = isOsFromSystemPropertyEqualityCheck("path.separator", "/")
+    this = isOsFromSystemPropertyEqualityCheck("path.separator", ":", true) or
+    this = isOsFromSystemPropertyEqualityCheck("path.separator", ";", false)
   }
 }
 
 private class IsUnixFromCharSeparator extends IsUnixGuard {
-  IsUnixFromCharSeparator() { this = isOsFromSystemPropertyEqualityCheck("file.separator", ":") }
+  IsUnixFromCharSeparator() {
+    this = isOsFromSystemPropertyEqualityCheck("file.separator", "/", true) or
+    this = isOsFromSystemPropertyEqualityCheck("file.separator", "\\", false)
+  }
 }
 
 private class IsUnixFromSystemProp extends IsSpecificUnixVariant instanceof MethodAccess {

java/ql/test/library-tests/os/Test.java

@@ -57,7 +57,7 @@ public class Test {
             onlyOnWindows();
         }
 
-        if (System.getProperty("path.separator").equals("\\")) {
+        if (System.getProperty("path.separator").equals(";")) {
             onlyOnWindows();
         }
     }
@@ -94,7 +94,7 @@ public class Test {
             onlyOnUnix();
         }
 
-        if (System.getProperty("path.separator").equals("/")) {
+        if (System.getProperty("path.separator").equals(":")) {
             onlyOnUnix();
         }
     }

java/ql/test/library-tests/os/unix-test.expected

@@ -2,4 +2,8 @@
 | Test.java:66:13:66:95 | contains(...) |
 | Test.java:70:13:70:84 | contains(...) |
 | Test.java:74:13:74:34 | SystemUtils.IS_OS_UNIX |
+| Test.java:81:13:81:41 | ... == ... |
+| Test.java:85:13:85:37 | ... == ... |
+| Test.java:89:13:89:37 | ... == ... |
+| Test.java:93:13:93:33 | ... == ... |
 | Test.java:97:13:97:60 | equals(...) |

java/ql/test/library-tests/os/windows-test.expected

@@ -3,4 +3,8 @@
 | Test.java:24:13:24:75 | contains(...) |
 | Test.java:28:13:28:75 | contains(...) |
 | Test.java:32:13:32:37 | SystemUtils.IS_OS_WINDOWS |
-| Test.java:60:13:60:61 | equals(...) |
+| Test.java:44:13:44:41 | ... == ... |
+| Test.java:48:13:48:37 | ... == ... |
+| Test.java:52:13:52:38 | ... == ... |
+| Test.java:56:13:56:34 | ... == ... |
+| Test.java:60:13:60:60 | equals(...) |

java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected

@@ -60,6 +60,10 @@ edges
 | Test.java:313:38:313:73 | getProperty(...) : String | Test.java:313:29:313:101 | new File(...) : File |
 | Test.java:313:38:313:73 | getProperty(...) : String | Test.java:316:35:316:46 | tempDirChild : File |
 | Test.java:316:35:316:46 | tempDirChild : File | Test.java:316:35:316:55 | toPath(...) |
+| Test.java:322:29:322:101 | new File(...) : File | Test.java:326:35:326:46 | tempDirChild : File |
+| Test.java:322:38:322:73 | getProperty(...) : String | Test.java:322:29:322:101 | new File(...) : File |
+| Test.java:322:38:322:73 | getProperty(...) : String | Test.java:326:35:326:46 | tempDirChild : File |
+| Test.java:326:35:326:46 | tempDirChild : File | Test.java:326:35:326:55 | toPath(...) |
 nodes
 | Files.java:10:24:10:69 | new File(...) : File | semmle.label | new File(...) : File |
 | Files.java:10:33:10:68 | getProperty(...) : String | semmle.label | getProperty(...) : String |
@@ -125,6 +129,10 @@ nodes
 | Test.java:313:38:313:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
 | Test.java:316:35:316:46 | tempDirChild : File | semmle.label | tempDirChild : File |
 | Test.java:316:35:316:55 | toPath(...) | semmle.label | toPath(...) |
+| Test.java:322:29:322:101 | new File(...) : File | semmle.label | new File(...) : File |
+| Test.java:322:38:322:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
+| Test.java:326:35:326:46 | tempDirChild : File | semmle.label | tempDirChild : File |
+| Test.java:326:35:326:55 | toPath(...) | semmle.label | toPath(...) |
 subpaths
 #select
 | Files.java:10:33:10:68 | getProperty(...) | Files.java:10:33:10:68 | getProperty(...) : String | Files.java:15:17:15:23 | tempDir | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Files.java:10:33:10:68 | getProperty(...) | system temp directory |
@@ -146,3 +154,4 @@ subpaths
 | Test.java:260:38:260:73 | getProperty(...) | Test.java:260:38:260:73 | getProperty(...) : String | Test.java:263:33:263:53 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:260:38:260:73 | getProperty(...) | system temp directory |
 | Test.java:294:38:294:73 | getProperty(...) | Test.java:294:38:294:73 | getProperty(...) : String | Test.java:298:35:298:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:294:38:294:73 | getProperty(...) | system temp directory |
 | Test.java:313:38:313:73 | getProperty(...) | Test.java:313:38:313:73 | getProperty(...) : String | Test.java:316:35:316:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:313:38:313:73 | getProperty(...) | system temp directory |
+| Test.java:322:38:322:73 | getProperty(...) | Test.java:322:38:322:73 | getProperty(...) : String | Test.java:326:35:326:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:322:38:322:73 | getProperty(...) | system temp directory |

java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java

@@ -316,4 +316,32 @@ public class Test {
             Files.createDirectory(tempDirChild.toPath());
         }
     }
+
+    void vulnerableBecauseInvertedFileSeparatorCheck() throws IOException {
+        // GIVEN:
+        File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");
+
+        // Oops, this check should be inverted
+        if (File.separatorChar != '\\') {
+            Files.createDirectory(tempDirChild.toPath()); // Creates with permissions 'drwxr-xr-x'
+        }
+    }
+
+    void safeBecauseFileSeparatorCheck() throws IOException {
+        // GIVEN:
+        File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");
+
+        if (File.separatorChar == '\\') {
+            Files.createDirectory(tempDirChild.toPath());
+        }
+    }
+
+    void safeBecauseInvertedFileSeperatorCheck() throws IOException {
+        // GIVEN:
+        File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");
+
+        if (File.separatorChar != '/') {
+            Files.createDirectory(tempDirChild.toPath());
+        }
+    }
 }