CERT
Skip to end of metadata
Go to start of metadata

There are two ways to synchronize access to shared mutable variables: method synchronization and block synchronization. Methods declared as synchronized and blocks that synchronize on the this reference both use the object as a monitor (that is, its intrinsic lock). An attacker can manipulate the system to trigger contention and deadlock by obtaining and indefinitely holding the intrinsic lock of an accessible class, consequently causing a denial of service (DoS).

One technique for preventing this vulnerability is the private lock object idiom [Bloch 2001]. This idiom uses the intrinsic lock associated with the instance of a private final java.lang.Object declared within the class instead of the intrinsic lock of the object itself. This idiom requires the use of synchronized blocks within the class's methods rather than the use of synchronized methods. Lock contention between the class's methods and those of a hostile class becomes impossible because the hostile class cannot access the private final lock object.

Static methods and state also share this vulnerability. When a static method is declared synchronized, it acquires the intrinsic lock of the class object before any statements in its body are executed, and it releases the intrinsic lock when the method completes. Untrusted code that has access to an object of the class, or of a subclass, can use the getClass() method to gain access to the class object and consequently manipulate the class object's intrinsic lock. Protect static data by locking on a private static final Object. Reducing the accessibility of the class to package-private provides further protection against untrusted callers.

The private lock object idiom is also suitable for classes that are designed for inheritance. When a superclass requests a lock on the object's monitor, a subclass can interfere with its operation. For example, a subclass may use the superclass object's intrinsic lock for performing unrelated operations, causing lock contention and deadlock. Separating the locking strategy of the superclass from that of the subclass ensures that they do not share a common lock and also permits fine-grained locking by supporting the use of multiple lock objects for unrelated operations. This increases the overall responsiveness of the application.

Objects that require synchronization must use the private lock object idiom rather than their own intrinsic lock in any case where untrusted code could:

  • Subclass the class.
  • Create an object of the class or of a subclass.
  • Access or acquire an object instance of the class or of a subclass.

Subclasses whose superclasses use the private lock object idiom must themselves use the idiom. However, when a class uses intrinsic synchronization on the class object without documenting its locking policy, subclasses must not use intrinsic synchronization on their own class object. When the superclass documents its policy by stating that client-side locking is supported, the subclasses have the option to choose between intrinsic locking and using the private lock object idiom. Subclasses must document their locking policy regardless of which locking option is chosen. See rule TSM00-J. Do not override thread-safe methods with methods that are not thread-safe for related information.

When any of these restrictions are violated, the object's intrinsic lock cannot be trusted. But when these restrictions are obeyed, the private lock object idiom fails to add any additional security. Consequently, objects that comply with all of the restrictions are permitted to synchronize using their own intrinsic lock. However, block synchronization using the private lock object idiom is superior to method synchronization for methods that contain nonatomic operations that could either use a more fine-grained locking scheme involving multiple private final lock objects or that lack a requirement for synchronization. Nonatomic operations can be decoupled from those that require synchronization and can be executed outside the synchronized block. Both for this reason and for simplification of maintenance, block synchronization using the private lock object idiom is generally preferred over intrinsic synchronization.

Noncompliant Code Example (Method Synchronization)

This noncompliant code example exposes instances of the SomeObject class to untrusted code.

The untrusted code attempts to acquire a lock on the object's monitor and, upon succeeding, introduces an indefinite delay that prevents the synchronized changeValue() method from acquiring the same lock. Furthermore, the object locked is publicly available via the lookup() method.

Alternatively, an attacker could create a private SomeObject object and make it available to trusted code to use it before the attacker code grabs and holds the lock.

Note that in the untrusted code, the attacker intentionally violates rule LCK09-J. Do not perform operations that can block while holding a lock.

Noncompliant Code Example (Public Non-final Lock Object)

This noncompliant code example locks on a public nonfinal object in an attempt to use a lock other than {{SomeObject}}'s intrinsic lock.

This change fails to protect against malicious code. For example, untrusted or malicious code could disrupt proper synchronization by changing the value of the lock object.

Noncompliant Code Example (Publicly Accessible Non-final Lock Object)

This noncompliant code example synchronizes on a publicly accessible but nonfinal field. The lock field is declared volatile so that changes are visible to other threads.

Any thread can modify the field's value to refer to a different object in the presence of an accessor such as setLock(). That modification might cause two threads that intend to lock on the same object to lock on different objects, thereby permitting them to execute two critical sections in an unsafe manner. For example, if the lock were changed when one thread was in its critical section, a second thread would lock on the new object instead of the old one and would enter its critical section erroneously.

A class that lacks accessible methods to change the lock is secure against untrusted manipulation. However, it remains susceptible to inadvertent modification by the programmer.

Noncompliant Code Example (Public Final Lock Object)

This noncompliant code example uses a public final lock object.

This noncompliant code example also violates rule OBJ01-J. Limit accessibility of fields.

Compliant Solution (Private Final Lock Object)

Thread-safe public classes that may interact with untrusted code must use a private final lock object. Existing classes that use intrinsic synchronization must be refactored to use block synchronization on such an object. In this compliant solution, calling changeValue() obtains a lock on a private final Object instance that is inaccessible to callers that are outside the class's scope.

A private final lock object can be used only with block synchronization. Block synchronization is preferred over method synchronization because operations without a requirement for synchronization can be moved outside the synchronized region, reducing lock contention and blocking. Note that it is unnecessary to declare the lock field volatile because of the strong visibility semantics of final fields. When granularity issues require the use of multiple locks, declare and use multiple private final lock objects to satisfy the granularity requirements rather than using a mutable reference to a lock object along with a setter method.

Noncompliant Code Example (Static)

This noncompliant code example exposes the class object of SomeObject to untrusted code.

The untrusted code attempts to acquire a lock on the class object''s monitor and, upon succeeding, introduces an indefinite delay that prevents the synchronized changeValue() method from acquiring the same lock.

A compliant solution must also comply with rule LCK05-J. Synchronize access to static fields that can be modified by untrusted code.
In the untrusted code, the attacker intentionally violates rule LCK09-J. Do not perform operations that can block while holding a lock.

Compliant Solution (Static)

Thread-safe public classes that both use intrinsic synchronization over the class object and may interact with untrusted code must be refactored to use a static private final lock object and block synchronization.

In this compliant solution, changeValue() obtains a lock on a private static Object that is inaccessible to the caller.

Exceptions

LCK00-J-EX0: A class may violate this rule when all of the following conditions are met:

  • It sufficiently documents that callers must not pass objects of this class to untrusted code.
  • The class cannot invoke methods, directly or indirectly, on objects of any untrusted classes that violate this rule.
  • The synchronization policy of the class is documented properly.

Clients are permitted to use a class that violates this rule when all of the following conditions are met:

  • Neither the client class nor any other class in the system passes objects of the violating class to untrusted code.
  • The violating class cannot invoke methods, directly or indirectly, from untrusted classes that violate this rule.

LCK00-J-EX1: When a superclass of the class documents that it supports client-side locking and synchronizes on its class object, the class can support client-side locking in the same way and document this policy.

LCK00-J-EX2: Package-private classes that are never exposed to untrusted code are exempt from this rule because their accessibility protects against untrusted callers. However, use of this exemption should be documented explicitly to ensure that trusted code within the same package neither reuses the lock object nor changes the lock object inadvertently.

Risk Assessment

Exposing the lock object to untrusted code can result in DoS.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

LCK00-J

low

probable

medium

P4

L3

Automated Detection

ToolVersionCheckerDescription
The Checker Framework

2.1.3

Lock CheckerConcurrency and lock errors (see Chapter 6)
Parasoft Jtest9.5TRS.SOPFImplemented
SonarQube Java Plugin4.11S2445 

Related Guidelines

MITRE CWE

CWE-412. Unrestricted externally accessible lock

 

CWE-413. Improper resource locking

Bibliography

[Bloch 2001]

Item 52. Document Thread Safety

 

            

32 Comments

  1. In your noncompliant code example, the instance of importantObj is available to the "malicious client". While this is possible through other means, you may want to declare the class public to build your argument in an easy way. This might also give you ideas for exceptions to the guideline (an object whose instances are inaccessible to untrusted code may not comply considering that we are synchronizing on a thread-safe object. Again, trusted code has no unforeseen motive to cause a denial of service). There is also relevant advice in VOID CON06-J. Do not defer a thread that is holding a lock. Good luck!

    • Need a citation for the following quote:

    There is some performance impact associated with using block synchronization instead of method synchronization but the difference is usually negligible.

    • This is a fine rule. I'm betting there are exceptions. But my main point, which I also made in CON01-J is that this rule seems to forbid method synchronization in favor of block synchronization. So all code samples should synchronize on blocks and private locks, not methods.
    1. But my main point, which I also made in CON01-J is that this rule seems to forbid method synchronization in favor of block synchronization.

      Only for public classes and when there is the possibility of interaction with untrusted code.

      1. David,

        This idiom is only useful for objects that may be accessible to hostile code.

        ... and trusted code but untrusted programmers (smile) . Legit code may cause all sorts of problems by subclassing this class and so on or using the lock directly, causing deadlocks, long waits etc.

        A private object gains no benefits from synchronizing on a private lock object, rather than synchronizing on its own intrinsic lock.

        I think I understand what you want to say here, however, the statement is too strong. This technique is almost always better even in the absence of untrusted code. This is like encapsulating to ensure stupid mistakes don't happen - so it does have some benefits.

        I was hoping that the exceptions would cover the allowable cases...

        Btw, I can't find a reasonable citation so I think it is best to remove that line about performance you mentioned.

        1. I think another exception would then be - if the class is final, then this guideline may be violated. This means that classes in CON01 must be declared final to be in compliance with this guideline.

          1. Refined the scope of when this rule is applicable, as we discussed.

  2. I believe java.util.Vector and java.util.Hashtable violate this guideline.

    1. My source for java.util.Vector doesn't violate this guideline. It uses a transient array, and array operations to control it. Can you be more specific?

      1. The classes are public and the methods are synchronized.

        1. Let me first quickly dash off a note: this rule's title should be changed to: "CON04-J. Use the private lock object idiom instead of method synchronization" It's less accurate than the current title, but also shorter. Now on to Vector and Hashtable...

          We have two choices here, and they are not mutually exclusive:

          • Declare Vector & Hashtable to violate this rule
          • Modify the rule to allow their design

          Having gone over the Javadoc API, I will state that while both classes claim to be synchronized, they give no details about how this thread-safety is implemented, nor any indication of what methods are synchronized. Ditto for the Enumerator class, which presumbly is involved in the thread-safety of both classes. Which signifies to me that the synchronization mechanism could change without notice in future versions of Java.

          I think it's safe to say that if we allow a publicy-accessible object to synchronize on itself, it should at least document this and indicate when it's safe for other classes to use its intrinsic lock. My current feeling is that this rule should not change and that they should remain violations. (they could easily be modified to comply with this rule w/o violating any documented API)

          1. I have shortened the title with regard to your request. Hopefully it is still accurate enough. I am thinking we could allow classes to violate this guideline if they sufficiently document the potential insecurity. The reason is that trusted code can always restrict the vector/hashtable object from being passed to untrusted code; or refuse to use an untrusted object that is vulnerable to DoS. IOW, even if a class violates this guideline, other guidelines may enable the application to remain secure. Appropriate documentation then serves as a warning to implementers.

            1. I would be fine with a documented exception as you suggest. (keeping in mind that the API docs for Vector and Hashtable are not sufficient to justify their noncompliance)

              1. I added the exception.

  3. I found a citation for your comment on Jan 28 regarding why method synchronization might be faster than block synchronization Doug Lea's Coding Standard:

    Prefer synchronized methods to synchronized blocks.
    Rationale: Better encsapsulation; less prone to subclassing snags; can be more efficient.

    Note that this is opposite of what this guideline recommends. I doubt this provides better encapsulation than an internal private lock object though.

    EDIT: The reference does not seem to be up-to-date (last updated in 2000).

  4. I wasn't sure about making this guideline as specific as this, but encourage private final locks even for the usual case. What do you think?

    1. I think we should be clear about what is required and what is not. We can easily add a line towards the end of the introduction which states: Because classes may be used in unexpected ways, block synchronization on private final lock object is generally recommended.

  5. The are a few security implications to public lock objects: wait() can be used overridden methods to release locks unexpectedly, notify() can be used if code is not safe against spurious wakeups and various techniques can make a theoretical-only race lazy.

    Also nested classes make it easy to lock the wrong object, even by experts reviewed by experts. For instance, from the JCiP errata: p.154: In Listing 7.15, the last two synchronized (this) lines should read synchronized (LogService.this).

    1. Thanks!

  6. The current title is easily misinterpreted...can we change it to:

    CON04-J. Use private final lock objects to synchronize classes that may interact with untrusted code

    1. Yes, I believe it can be very easily done.

  7. In headings such as, "Noncompliant Code Example (Public Final Lock Object)"

    should public final be capitalized or not? Code font?

  8. David, I don't think the previous change is required. In fact, we might want to get rid of the sleep() because while(true) is enough and the there is no try-catch to catch interrupted exception.

    1. OR

      Replace:

      with:

      Btw Noncompliant Code Example (Static) does not require any of this.
      You just need to use SomeObject.class.getClass() in the synchronized block.

      1. Dhruv: WRT this code:

        I'll agree it's overkill, but the point is to delay someObject infinitely long, not just for a long time.

        I fixed the other points.

        1. I thought the while(true) would do the needful. Anyway, I assume that the method throws InterruptedException (it needn't be shown). Thanks for fixing the others!

  9. Though this page has been up a while, I see a couple of problems here.

    1. Untrusted code examples above that instantiate the non-compliant class are actually benign. The private instance of the class is just that - it's not visible to anyone but the code attempting to perform the attack vector, including the application itself. The problem arises when a previously initialized instance is provided via dependency injection, factory method or simply a getter on another object. I think the examples should be changed to reflect that fact.
    2. I may be missing something but I fail to see how declaring the class package-private actually protects against malicious use by untrusted code. Accessibility restrictions imposed by the language are orthogonal to deployment-time possibilities of exploits; here are just a couple points that make that apparent:
      1. I can create a class in the same package as the "trusted" class that violates this rule, deploy it alongside the code I am attacking, and gain visibility into all package-private methods and fields of the instance.
      2. I do not need to reference the package-private class by its declared name in order to gain access to its instance; if class Foo is package-private but it implements a public interface Bar, and I know that an accessible instance of Bar is actually an instance of Foo, then I can proceed with the exploit relying on my knowledge of Foo's internals.
    1. 1. The 'untrusted' examples serve to show how any trusted lock that they can access can be used to attack the program (by engaging the lock and never releasing it). Ultimately this is an access control problem, which is largely addressed by the "private" keyword, as well as by package-private classes, fields, and methods. It is certainly possible for trusted code to leak a private lock object to untrusted code, and we cover the most common case here: a public getter method.

      2. Java's reflection mechanisms are designed to respect access restrictions. While you can create a class with the same qualified (package) name as the trusted class, if your class is loaded by a different class loader, then the packages are different (even if their names match). If the trusted class is part of an application that uses the same class loader to load your untrusted class (say via a plugin mechanism), then it is vulnerable as you suggest. But that would be an insecure design, and not the fault of the class with a private-package lock. See SEC05-J. Do not use reflection to increase accessibility of classes, methods, or fields for further details.

      1. David Svoboda: I think you have misunderstood the gist of both of my points.

        The first issue is that the "untrusted", or malicious code examples as they are shown above are not actually harming anything. The reason being is that you are synchronizing on an object, instantiated locally to the client (presumably attacker's) code - so this not going to block the execution of trusted code by holding the lock on this instance of the trusted class, precisely because the object is "private" to you. A more convincing and realistic illustration would look like so:

        The larger picture behind my second point is that the exception to this rule relating to making your class package private does not generally hold true, in the presence of a simple arrangement like this:

        Finally (and I just noticed this), the non-compliant code example that claims to immediately release the lock on a publicly accessible lock object is wrong too, specifically this snippet will cause an IllegalMonitorStateExeption thrown immediately, instead of releasing the lock held by the trusted code (this example also suffers from the same issue I pointed out in my point 1):

        The reason for this is you cannot invoke wait() on an object's monitor without owning it (i.e. do it outside of a synchronized block). It's also the reason why it is generally not possible for untrusted code to release a scoped lock taken by trusted code, unless the untrusted code is on the call stack from the trusted code itself, inside the lock scope - but in that case, you're dead in the water anyway, as all sorts of nasty things are possible.

        1. The first issue is that the "untrusted", or malicious code examples as they are shown above are not actually harming anything. The reason being is that you are synchronizing on an object, instantiated locally to the client (presumably attacker's) code - so this not going to block the execution of trusted code by holding the lock on this instance of the trusted class, precisely because the object is "private" to you. A more convincing and realistic illustration would look like so:

          Agreed, I tweaked the 1st NCCE's exploit code to be more illustrative 
          The larger picture behind my second point is that the exception to this rule relating to making your class package private does not generally hold true, in the presence of a simple arrangement like this:
          I restricted the exception to only apply to package-private classes that are never exposed to untrusted code.
          Finally (and I just noticed this), the non-compliant code example that claims to immediately release the lock on a publicly accessible lock object is wrong too, specifically this snippet will cause an IllegalMonitorStateExeption thrown immediately, instead of releasing the lock held by the trusted code (this example also suffers from the same issue I pointed out in my point 1):
          Argh, that's true. I removed that attack code from the NCCE; the first attack code can apply to all NCCEs.
          1. I restricted the exception to only apply to package-private classes that are never exposed to untrusted code

            Thanks for incorporating my suggestions, I still think this one though has nothing to do with accessibility of trusted class. In Java, if one has access to your bytecode, they have already seen your source (enough of it in this case to mount an attack anyway). And as we both agree at this point, making your class package-private or just private will then not be an acceptable substitute for following this rule.

            1. I am assuming that an attacker can *read* your bytecode (prob via your .class file) but not modify it. Or, perhaps they modify it, but their modified bytecode is no longer your responsibility, as long as it is not mistaken for your pristine code.

              The two concerns are that (1) an object of your class could be leaked to untrusted code, and (2) external code (not necessarily untrusted, but not under your control) could create their own class object and get DOSed because of the non-private lock. The clause I added to EX3 prevents (1), while making the class package-private prevents (2).

              1. If you take a closer look at the second code snippet in my earlier comment, it should become apparent that making the class package private does not prevent the exploit from occurring. I maintain that runtime or compile-time accessibility of the package the declared class of an object belongs to has nothing to do with your ability to attempt to acquire the object's intrinsic lock, as long as the object itself is accessible to you via some public means (see the code example). Making your class package-private does not prevent decompilation of its bytecode, which would immediately reveal any critical sections that use intrinsic locks - there is no need to modify anything in the original class.