From aed51644ba49f3582d6583028acaab618bde505c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Sat, 1 Mar 2025 21:22:26 +0000 Subject: [PATCH 1/6] Convert to inline expectations test --- .../CWE-367/semmle/tests/TOCTOURace.qlref | 3 +- .../security/CWE-367/semmle/tests/Test.java | 38 +++++++++---------- 2 files changed, 21 insertions(+), 20 deletions(-) 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() { } } } From dc2cbf74020f7c80c2f4638f5a2a3c7649f64bec Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 13 Mar 2025 15:02:26 +0000 Subject: [PATCH 2/6] Add tests for always-locked fields --- .../semmle/tests/FieldAlwaysLocked.java | 24 ++++++++++++++++ .../semmle/tests/FieldNotAlwaysLocked.java | 28 +++++++++++++++++++ .../CWE-367/semmle/tests/TOCTOURace.expected | 4 +++ 3 files changed, 56 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldAlwaysLocked.java create mode 100644 java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldNotAlwaysLocked.java 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..7f676650dcff --- /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(); // $ SPURIOUS: Alert + while (e.hasMoreElements()) { + o = e.nextElement(); + field.remove(o); // $ SPURIOUS: Alert + } + } + } +} 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..e1cd7942ebef 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,7 @@ +| FieldAlwaysLocked.java:17:41:17:52 | keys(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | FieldAlwaysLocked.java:8:19:8:23 | field | field | FieldAlwaysLocked.java:16:21:16:32 | size(...) | is checked at a previous call | +| FieldAlwaysLocked.java:20:33:20:47 | remove(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | FieldAlwaysLocked.java:8:19:8:23 | field | field | FieldAlwaysLocked.java:16:21:16:32 | size(...) | is checked at a previous call | +| 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 | From a8e993c942aa1ce90634af053a470701df354766 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 13 Mar 2025 15:03:32 +0000 Subject: [PATCH 3/6] Fix FP for always-locked fields --- java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql | 4 +++- .../security/CWE-367/semmle/tests/FieldAlwaysLocked.java | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql b/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql index ca2c948867f8..4c71e1066071 100644 --- a/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql +++ b/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql @@ -68,7 +68,9 @@ predicate alwaysLocked(Field f) { or exists(RefType thisType | forex(VarAccess access | - access = f.getAnAccess() and not access.getEnclosingCallable() instanceof InitializerMethod + access = f.getAnAccess() and + not access.getEnclosingCallable() instanceof Constructor and + not access.getEnclosingCallable() instanceof InitializerMethod | locallySynchronizedOnThis(access, thisType) ) 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 index 7f676650dcff..71a463364b84 100644 --- 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 @@ -14,10 +14,10 @@ public FieldAlwaysLocked() { protected synchronized void checkOut() { Object o; if (field.size() > 0) { - Enumeration e = field.keys(); // $ SPURIOUS: Alert + Enumeration e = field.keys(); while (e.hasMoreElements()) { o = e.nextElement(); - field.remove(o); // $ SPURIOUS: Alert + field.remove(o); } } } From 6ca9a1ff9ad38edb60e36caa592f654e682e0272 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 13 Mar 2025 15:05:32 +0000 Subject: [PATCH 4/6] Add change note --- .../src/change-notes/2025-03-13-fix-toctou-false-positive.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/src/change-notes/2025-03-13-fix-toctou-false-positive.md 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. From 5c7588822d21e972ad09dec8e16887e06344c3fb Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 14 Mar 2025 11:39:50 +0000 Subject: [PATCH 5/6] Fix test output --- .../security/CWE-367/semmle/tests/TOCTOURace.expected | 2 -- 1 file changed, 2 deletions(-) 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 e1cd7942ebef..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,5 +1,3 @@ -| FieldAlwaysLocked.java:17:41:17:52 | keys(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | FieldAlwaysLocked.java:8:19:8:23 | field | field | FieldAlwaysLocked.java:16:21:16:32 | size(...) | is checked at a previous call | -| FieldAlwaysLocked.java:20:33:20:47 | remove(...) | This uses the state of $@ which $@. But these are not jointly synchronized. | FieldAlwaysLocked.java:8:19:8:23 | field | field | FieldAlwaysLocked.java:16:21:16:32 | size(...) | is checked at a previous call | | 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 | From 7702e9da7d5555c0908f8bbfd59ff4e6c74efb89 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 14 Mar 2025 11:36:43 +0000 Subject: [PATCH 6/6] Address review comments --- .../ql/src/Security/CWE/CWE-367/TOCTOURace.ql | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql b/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql index 4c71e1066071..868085524337 100644 --- a/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql +++ b/java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql @@ -54,32 +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 Constructor 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) ) )