CON32-J. Synchronize access to static fields that may be modified by untrusted code

Class methods that can be invoked from untrusted code to modify a static field must synchronize access to that field. This is necessary because there is no guarantee that untrusted clients will externally synchronize when accessing the field. Because a static field is shared by all clients, untrusted clients may violate the contract by failing to provide suitable locking.

According to Joshua Block [[Bloch 08]]:

If a method modifies a static field, you must synchronize access to this field, even if the method is typically used only by a single thread. It is not possible for clients to perform external synchronization on such a method because there can be no guarantee that unrelated clients will do likewise.

This guideline also applies to classes that explicitly document their lack of thread-safety. Documented design intent is irrelevant when dealing with untrusted code because an attacker can always chose to ignore the documentation.

Noncompliant Code Example

This noncompliant code example does not synchronize access to the static field counter.

/** This class is not thread-safe! */
public final class CountHits {
  private static int counter;
  
  public void incrementCounter() {
    counter++;
  }
}

This class definition does not violate CON01-J. Ensure that compound operations on shared variables are atomic which only applies to classes that promise thread-safety. However, this class has a mutable static field counter that is modified by the publicly accessible incrementCounter() method. Consequently, this class cannot be securely used by untrusted client code that may (purposely) fail to externally synchronize access to the field.

Compliant Solution

This compliant solution uses a static private final lock to protect the counter field and consequently, does not depend on any external synchronization. This solution also complies with CON04-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

/** This class is thread-safe */
public final class CountHits {
  private static int counter;
  private static final Object lock = new Object();

  public void incrementCounter() {
    synchronized (lock) {
      counter++;
    }
  }
}

Risk Assessment

Failing to protect classes containing accessible static members can result in unexpected results when a client fails to obey the classes' synchronization policy.

Rule	Severity	Likelihood	Remediation Cost	Priority	Level
CON32- J	low	probable	medium	P4	L3

Automated Detection

TODO

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

References

[[API 06]]
[[Bloch 08]] Item 67: "Avoid excessive synchronization"

Issue Tracking

85%

Review List

handler

and specifies its lack of thread-safety in the documentation. => rephrase to : and documents its lack of thread-safety. <---- FIXED ~DS

Priority MEDIUM

Assigned To svoboda

Created Apr 01, 2010

1:KJSXM2LFO4QEY2LTOQ_n547935532 MEDIUM 1270153006582 true 1270216402886 false svoboda
handler

protecting fields is a weak word. Should be changed to synchronizing access to fields.

Priority MEDIUM

Assigned To rcs

Created Apr 01, 2010

1:KJSXM2LFO4QEY2LTOQ_n2024465319 MEDIUM 1270152984383 true 1270219726077 false rcs
handler

and not because it documents its lack of thread-safety; suggest removing this

Priority MEDIUM

Assigned To rcs_mgr

Created Apr 01, 2010

1:KJSXM2LFO4QEY2LTOQ_n1521926222 MEDIUM 1270152995003 true 1270327832435 false rcs_mgr
handler

The title can also be Ensure classes containing publicly accessible mutable static fields are thread-safe

Priority MEDIUM

Assigned To rcs_mgr

Created Apr 01, 2010

1:KJSXM2LFO4QEY2LTOQ_n1073687404 MEDIUM 1270153016807 true 1270327838669 false rcs_mgr
handler

"Classes that use static fields that are both publicly accessible and mutable must always protect accesses to their fields, regardless of their documentation." => A class that is internally providing synchronization would not document that it is not thread-safe. So I don't understand why you added the words in bold.

Priority MEDIUM

Assigned To rcs_mgr

Created Apr 02, 2010

1:KJSXM2LFO4QEY2LTOQ_n1464687817 MEDIUM 1270215734760 true 1270327843870 false rcs_mgr
handler

"The class relies on clients to externally synchronize the object and this class documents its lack of thread-safety" => "this class" occurs twice in the sentence. Should be "and documents its lack of thread safety".

Priority MEDIUM

Assigned To rcs_mgr

Created Apr 02, 2010

1:KJSXM2LFO4QEY2LTOQ_p1571147287 MEDIUM 1270215935982 true 1270327856033 false rcs_mgr
handler

"any assurances that the class is safe-thread." => safe-thread should be thread-safe!

Priority MEDIUM

Assigned To dmohindr

Created Apr 03, 2010

1:KJSXM2LFO4QEY2LTOQ_n1625903486 MEDIUM 1270325608947 false 0 false dmohindr

VOID CON06-J. Do not defer a thread that is holding a lock 11. Concurrency (CON)

Space shortcuts

Page tree