diff --git a/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql b/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql index ca2c948867f8..868085524337 100644 --- a/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql +++ b/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) ) ) diff --git a/java/ql/src/change-notes/2025-03-13-fix-toctou-false-positive.md b/java/ql/src/change-notes/2025-03-13-fix-toctou-false-positive.md new file mode 100644 index 000000000000..fb6fcfaaf1b1 --- /dev/null +++ b/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. diff --git a/java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldAlwaysLocked.java b/java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldAlwaysLocked.java new file mode 100644 index 000000000000..71a463364b84 --- /dev/null +++ b/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); + } + } + } +} diff --git a/java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldNotAlwaysLocked.java b/java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldNotAlwaysLocked.java new file mode 100644 index 000000000000..cdae7f924e58 --- /dev/null +++ b/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(); + } +} diff --git a/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.expected b/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.expected index f68820385401..f2dc9f8aa957 100644 --- a/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.expected +++ b/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 | diff --git a/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref b/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref index 243a56419353..b278242dea6d 100644 --- a/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref +++ b/java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref @@ -1 +1,2 @@ -Security/CWE/CWE-367/TOCTOURace.ql \ No newline at end of file +query: Security/CWE/CWE-367/TOCTOURace.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/java/ql/test/query-tests/security/CWE-367/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-367/semmle/tests/Test.java index c25a699001a6..11287896bbe4 100644 --- a/java/ql/test/query-tests/security/CWE-367/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-367/semmle/tests/Test.java @@ -4,27 +4,27 @@ 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 } } @@ -32,11 +32,11 @@ 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() { } } }