Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
owen-mc merged 6 commits into from
Mar 14, 2025
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aed5164
Convert to inline expectations test
owen-mc Mar 1, 2025
dc2cbf7
Add tests for always-locked fields
owen-mc Mar 13, 2025
a8e993c
Fix FP for always-locked fields
owen-mc Mar 13, 2025
6ca9a1f
Add change note
owen-mc Mar 13, 2025
5c75888
Fix test output
owen-mc Mar 14, 2025
7702e9d
Address review comments
owen-mc Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor Author

@owen-mc owen-mc Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe we need to add a restriction that Field.getDeclaringType() = thisType. It's probably always true because it would be rare for a field to not be accessed at all in its declaring class.

We are trying to encode the logic that "f is only accessed in synchronized methods on its declaring class (or the equivalent with synchronized statements)". We have already excluded initializers.

My reasoning is:

  • The constructor will be run by one thread.
  • Other threads won't normally get access to the object until after the constructor has finished
    • (In theory you could leak the this reference from the constructor, but I think we can assume that doesn't happen.)
  • Therefore, other threads won't be able to call methods that reference f until the constructor has finished.

So, my conclusion is: the code relating to this probably assumed that f is a field of thisType, and that's probably almost always implied by the code; as it stands, I think the code is okay even if f is not a field of thisType; but if we make the change above to exclude constructors then we probably do want to add the restriction that f is a field of thisType to keep correctness.

Copy link
Copy Markdown
Contributor

@aschackmull aschackmull Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we need to add a restriction that Field.getDeclaringType() = thisType

No, that should not be necessary - it's not completely inconceivable to synchronize on a this from some other class. As long as that's done consistently, then it's fine.

What we might need instead, though, is to check that the Constructor belongs to the same class as the field. And in a similar vein, we probably ought to check the same for the InitializerMethod.
Also, this extension from InitializerMethods to Constructors also applies to the two other disjuncts.
So at this point we should probably factor out a helper predicate identifying field accesses that aren't in constructors or initializers of the declaring type of the field, and use that as the common limitation in all three forexs.

not access.getEnclosingCallable() instanceof InitializerMethod
|
locallySynchronizedOnThis(access, thisType)
)
Expand Down
4 changes: 4 additions & 0 deletions java/ql/src/change-notes/2025-03-13-fix-toctou-false-positive.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
24 changes: 24 additions & 0 deletions java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldAlwaysLocked.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}
28 changes: 28 additions & 0 deletions java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldNotAlwaysLocked.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
}
}
4 changes: 4 additions & 0 deletions java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |
Expand Down
3 changes: 2 additions & 1 deletion java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
Security/CWE/CWE-367/TOCTOURace.ql
query: Security/CWE/CWE-367/TOCTOURace.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
38 changes: 19 additions & 19 deletions java/ql/test/query-tests/security/CWE-367/semmle/tests/Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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)
Expand All @@ -122,7 +122,7 @@ public void act() {
sideEffect();
}
}

public void sideEffect() { }
}
}