Merge pull request #19015 from owen-mc/java/toctou-sync-methods

Java: Fix FP in "Time-of-check time-of-use race condition" (`java/toctou-race-condition`)

Author: owen-mc

java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql

@@ -54,30 +54,34 @@ class PossiblyConcurrentCallable extends Callable {
   }
 }
 
+private VarAccess getANonInitializationAccess(Field f) {
+  result = f.getAnAccess() and
+  exists(Callable c | c = result.getEnclosingCallable() |
+    not (
+      c = f.getDeclaringType().getACallable() and
+      (c instanceof Constructor or c instanceof InitializerMethod)
+    )
+  )
+}
+
 /**
  * Holds if all accesses to `v` (outside of initializers) are locked in the same way.
  */
 predicate alwaysLocked(Field f) {
   exists(Variable lock |
-    forex(VarAccess access |
-      access = f.getAnAccess() and not access.getEnclosingCallable() instanceof InitializerMethod
-    |
+    forex(VarAccess access | access = getANonInitializationAccess(f) |
       locallySynchronizedOn(access, _, lock)
     )
   )
   or
   exists(RefType thisType |
-    forex(VarAccess access |
-      access = f.getAnAccess() and not access.getEnclosingCallable() instanceof InitializerMethod
-    |
+    forex(VarAccess access | access = getANonInitializationAccess(f) |
       locallySynchronizedOnThis(access, thisType)
     )
   )
   or
   exists(RefType classType |
-    forex(VarAccess access |
-      access = f.getAnAccess() and not access.getEnclosingCallable() instanceof InitializerMethod
-    |
+    forex(VarAccess access | access = getANonInitializationAccess(f) |
       locallySynchronizedOnClass(access, classType)
     )
   )

java/ql/src/change-notes/2025-03-13-fix-toctou-false-positive.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed a false positive in "Time-of-check time-of-use race condition" (`java/toctou-race-condition`) where a field of a non-static class was not considered always-locked if it was accessed in a constructor.

java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldAlwaysLocked.java

@@ -0,0 +1,24 @@
+package test.cwe367.semmle.tests;
+
+import java.util.Enumeration;
+import java.util.Hashtable;
+
+class FieldAlwaysLocked {
+
+        Hashtable field;
+
+        public FieldAlwaysLocked() {
+                field = new Hashtable();
+        }
+
+        protected synchronized void checkOut() {
+                Object o;
+                if (field.size() > 0) {
+                        Enumeration e = field.keys();
+                        while (e.hasMoreElements()) {
+                                o = e.nextElement();
+                                field.remove(o);
+                        }
+                }
+        }
+}

java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldNotAlwaysLocked.java

@@ -0,0 +1,28 @@
+package test.cwe367.semmle.tests;
+
+import java.util.Enumeration;
+import java.util.Hashtable;
+
+class FieldNotAlwaysLocked {
+
+        Hashtable field;
+
+        public FieldNotAlwaysLocked() {
+                field = new Hashtable();
+        }
+
+        protected synchronized void checkOut() {
+                Object o;
+                if (field.size() > 0) {
+                        Enumeration e = field.keys(); // $ Alert
+                        while (e.hasMoreElements()) {
+                                o = e.nextElement();
+                                field.remove(o); // $ Alert
+                        }
+                }
+        }
+
+        protected void modifyUnlocked() {
+                field = new Hashtable();
+        }
+}

java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.expected

@@ -1,3 +1,5 @@
+| FieldNotAlwaysLocked.java:17:41:17:52 | keys(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | FieldNotAlwaysLocked.java:8:19:8:23 | field | field | FieldNotAlwaysLocked.java:16:21:16:32 | size(...) | is checked at a previous call |
+| FieldNotAlwaysLocked.java:20:33:20:47 | remove(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | FieldNotAlwaysLocked.java:8:19:8:23 | field | field | FieldNotAlwaysLocked.java:16:21:16:32 | size(...) | is checked at a previous call |
 | Test.java:13:4:13:10 | act(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | Test.java:10:32:10:41 | r | r | Test.java:12:7:12:18 | getState(...) | is checked at a previous call |
 | Test.java:20:4:20:10 | act(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | Test.java:17:32:17:42 | r | r | Test.java:19:7:19:18 | getState(...) | is checked at a previous call |
 | Test.java:27:4:27:10 | act(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | Test.java:24:19:24:28 | r | r | Test.java:26:7:26:18 | getState(...) | is checked at a previous call |

java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref

@@ -1 +1,2 @@
-Security/CWE/CWE-367/TOCTOURace.ql
+query: Security/CWE/CWE-367/TOCTOURace.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

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

@@ -4,39 +4,39 @@
 
 class Test {
 	public final Object lock = new Object();
-	
+
 	public volatile boolean aField = true;
-	
+
 	public synchronized void bad1(Resource r) {
 		// probably used concurrently due to synchronization
 		if (r.getState()) {
-			r.act();
+			r.act(); // $ Alert
 		}
 	}
 
 	public synchronized void bad2(Resource2 r) {
 		// probably used concurrently due to synchronization
 		if (r.getState()) {
-			r.act();
+			r.act(); // $ Alert
 		}
 	}
 
 	public void bad3(Resource r) {
 		// probably used concurrently due to use of volatile field
 		if (r.getState() && aField) {
-			r.act();
+			r.act(); // $ Alert
 		}
 	}
 
 	public void bad4(Resource r) {
 		// probably used concurrently due to synchronization
 		synchronized(this) {
 			if (r.getState() && aField) {
-				r.act();
+				r.act(); // $ Alert
 			}
 		}
 	}
-	
+
 	public void good1(Resource r) {
 		// synchronizes on the same monitor as the called methods
 		synchronized(r) {
@@ -45,15 +45,15 @@ public void good1(Resource r) {
 			}
 		}
 	}
-	
+
 	public Resource rField = new Resource();
-	
+
 	public void someOtherMethod() {
 		synchronized(lock) {
 			rField.act();
 		}
 	}
-	
+
 	public void good2() {
 		// r is always guarded with the same lock, so okay
 		synchronized(lock) {
@@ -77,43 +77,43 @@ public void good3(Resource r) {
 			r.act();
 		}
 	}
-	
+
 	class Resource {
 		boolean state;
-		
+
 		public synchronized void setState(boolean newState) {
 			this.state = newState;
 		}
-		
+
 		public synchronized boolean getState() {
 			return state;
 		}
-		
+
 		public synchronized void act() {
 			if (state)
 				sideEffect();
 			else
 				sideEffect();
 		}
-		
+
 		public void sideEffect() { }
 	}
 
 	class Resource2 {
 		boolean state;
-		
+
 		public void setState(boolean newState) {
 			synchronized(this) {
 				this.state = newState;
 			}
 		}
-		
+
 		public boolean getState() {
 			synchronized(this) {
 				return state;
 			}
 		}
-		
+
 		public void act() {
 			synchronized(this) {
 				if (state)
@@ -122,7 +122,7 @@ public void act() {
 					sideEffect();
 			}
 		}
-		
+
 		public void sideEffect() { }
 	}
 }