Skip to end of metadata
Go to start of metadata

A mutable input has the characteristic that its value may vary; that is, multiple accesses may see differing values. This characteristic enables potential attacks that exploit race conditions. For example, a time-of-check, time-of-use (TOCTOU) vulnerability may result when a field contains a value that passes validation and security checks but changes before use.

Returning references to an object's internal mutable components provides an attacker with the opportunity to corrupt the state of the object. Consequently, accessor methods must return defensive copies of internal mutable objects (see OBJ05-J. Do not return references to private mutable class members for more information).

Noncompliant Code Example

This noncompliant code example contains a TOCTOU vulnerability. Because cookie is a mutable input, an attacker can cause it to expire between the initial check (the hasExpired() call) and the actual use (the doLogic() call).

public final class MutableDemo {
  // java.net.HttpCookie is mutable
  public void useMutableInput(HttpCookie cookie) {
    if (cookie == null) {
       throw new NullPointerException();
    }

    // Check whether cookie has expired
    if (cookie.hasExpired()) {
      // Cookie is no longer valid; handle condition by throwing an exception
    }

    // Cookie may have expired since time of check 
    doLogic(cookie);
  }
}

Compliant Solution

This compliant solution avoids the TOCTOU vulnerability by copying the mutable input and performing all operations on the copy. Consequently, an attacker's changes to the mutable input cannot affect the copy. Acceptable techniques include using a copy constructor or implementing the java.lang.Cloneable interface and declaring a public clone() method (for classes not declared final). In cases such as HttpCookie where the mutable class is declared final—that is, it cannot provide an accessible copy method—perform a manual copy of the object state within the caller (see OBJ04-J. Provide mutable classes with copy functionality to safely allow passing instances to untrusted code for more information). Note that any input validation must be performed on the copy rather than on the original object.

public final class MutableDemo {
  // java.net.HttpCookie is mutable
  public void useMutableInput(HttpCookie cookie) {
    if (cookie == null) {
      throw new NullPointerException();
    }

    // Create copy
    cookie = (HttpCookie)cookie.clone();

    // Check whether cookie has expired
    if (cookie.hasExpired()) {
      // Cookie is no longer valid; handle condition by throwing an exception
    }

    doLogic(cookie);
  }
}

Compliant Solution

Some copy constructors and clone() methods perform a shallow copy of the original instance. For example, invocation of clone() on an array results in creation of an array instance whose elements have the same values as the original instance. This shallow copy is sufficient for arrays of primitive types but fails to protect against TOCTOU vulnerabilities when the elements are references to mutable objects, such as an array of cookies. Such cases require a deep copy that also duplicates the reference objects.

This compliant solution demonstrates correct use both of a shallow copy (for the array of int) and of a deep copy (for the array of cookies):

public void deepCopy(int[] ints, HttpCookie[] cookies) {
  if (ints == null || cookies == null) {
    throw new NullPointerException();
  }

  // Shallow copy
  int[] intsCopy = ints.clone();

  // Deep copy
  HttpCookie[] cookiesCopy = new HttpCookie[cookies.length];
  for (int i = 0; i < cookies.length; i++) {
    // Manually create copy of each element in array
    cookiesCopy[i] = (HttpCookie)cookies[i].clone();
  }
 
  doLogic(intsCopy, cookiesCopy);
}

Noncompliant Code Example

When the class of a mutable input is nonfinal or is an interface, an attacker can write a subclass that maliciously overrides the parent class's clone() method. The attacker's clone() method can subsequently subvert defensive copying. This noncompliant code example demonstrates this weakness:

// java.util.Collection is an interface
public void copyInterfaceInput(Collection<String> collection) {
  doLogic(collection.clone());
}

Compliant Solution

This compliant solution protects against potential malicious overriding by creating a new instance of the nonfinal mutable input, using the expected class rather than the class of the potentially malicious argument. The newly created instance can be forwarded to any code capable of modifying it.

public void copyInterfaceInput(Collection<String> collection) {
  // Convert input to trusted implementation
  collection = new ArrayList(collection);
  doLogic(collection);
}

Some objects appear to be immutable because they have no mutator methods. For example, the java.lang.CharSequence interface describes an immutable sequence of characters. Note, however, that a variable of type CharSequence is a reference to an underlying object of some other class that implements the CharSequence interface; that other class may be mutable. When the underlying object changes, the CharSequence changes. Essentially, the java.lang.CharSequence interface omits methods that would permit object mutation through that interface but lacks any guarantee of true immutability. Such objects must still be defensively copied before use. For the case of the java.lang.CharSequence interface, one permissible approach is to obtain an immutable copy of the characters by using the toString() method. Mutable fields should not be stored in static variables. When there is no other alternative, create defensive copies of the fields to avoid exposing them to untrusted code.

Risk Assessment

Failing to create a copy of a mutable input may result in a TOCTOU vulnerability or expose internal mutable components to untrusted code.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

OBJ06-J

Medium

Probable

High

P4

L3

Automated Detection

ToolVersionCheckerDescription
CodeSonar
5.1p0
PMD.Security-Code-Guidelines.ArrayIsStoredDirectly
FB.MALICIOUS_CODE.EI_EXPOSE_STATIC_REP2
Array is stored directly
May expose internal static state by storing a mutable object into a static field
Parasoft Jtest
10.3
SECURITY.EAB.CPCL, SECURITY.EAB.MPT, SECURITY.EAB.SMO, OOP.MUCOPImplemented
SonarQube
6.7
S2384

Mutable members should not be stored or returned directly

Implemented for Arrays, Collections and Dates.

Related Vulnerabilities

CVE-2012-0507 describes an exploit that managed to bypass Java's applet security sandbox and run malicious code on a remote user's machine. The exploit created a data structure that is normally impossible to create in Java but was built using deserialization, and the deserialization process did not perform defensive copies of the deserialized data. See the code examples in SER07-J. Do not use the default serialized form for classes with implementation-defined invariants for more information.

Related Guidelines

Secure Coding Guidelines for Java SE, Version 5.0

Guideline 6-2 / MUTABLE-2: Create copies of mutable output values

Bibliography

[Bloch 2008]

Item 39, "Make Defensive Copies When Needed"

[Pugh 2009]

"Returning References to Internal Mutable State"



8 Comments

  1. TODO: In CS, looks like the cookie should be cloned first and then checked for null (on the copied value). May need some fixing. Alternatively there should be a condition to check if cookie has become null, before the line if(cookie.hasExpired()) { .......

    But check if clone can be invoked on a null value. Not too wise.

  2. Isn't there still a TOCTOU race condition between the beginning of your function to handle the cookie and the invocation of cookie.clone()?

    1. Yes there is a TOCTOU, as I commented (btw, this example is straight out of SUN's secure coding document). clone() cannot be invoked on a null value so again, it is unwise to blindly clone and then check for null.

      Having an if-else (shown below) may provide some temporary relief. Is there a cleaner solution?

      public final class MutableDemo {
      
        // java.net.HttpCookie is mutable
        public void copyMutableInput(HttpCookie cookie) {
          if (cookie == null) {
            throw new NullPointerException();
          }
          else {    //<-----------------------------------------------------
            // create copy
            cookie = cookie.clone();
      
            //check if cookie has expired
            if(cookie.hasExpired()) {
              //cookie is no longer valid, handle condition
            }
          }
      
          doLogic(cookie);
        }
      }
      
      1. I do think the race condition is unavoidable if you don't control the creation of the object. (If you do, you can use the synchronize keyword to restrict access.)

        1. It really doesn't matter to the implementation method if you modify the argument before the copy is made (or even during). Just so long as it has a consistent copy, it is fine. However, if you are calling the implementation method by another trusted method that does rely on the argument not changing, then the calling method must also make a clone in exactly the same manner.

          The explicit null-check is just to be explicit. Calling clone would throw an NPE anyway, but does not make clear then intended contract of the method. On the other hand, any argument checking on data that can be modified must go after the copy. So, for instance, if the method required a particular domain, then it'd need to be written something like:

          public final class MutableDemo {
            private final String domain;
            ...
            public void copyMutableInput(HttpCookie cookie) {
              if (cookie == null) {
                 throw new NullPointerException();
              }
          
              // create copy
              cookie = cookie.clone();
          
              // check copied cookie acceptable
              if (!this.domain.equals(cookie.getDomain())) {
                throw new IllegalArgumentException(
                  "Cookie from illegal domain"
                );
              }
              ...
            }
          }
          
  3. This rule should probably reference & discuss FIO04-J. Canonicalize path names before validating, as they are about about mutable object references.

  4. Some architectures probably do this by default. For example in Java EE, quoting from Java EE Tutorials:

    The parameters of remote calls are more isolated than those of local calls. With remote calls, the client and bean operate on different copies of a parameter object. If the client changes the value of the object, the value of the copy in the bean does not change. This layer of isolation can help protect the bean if the client accidentally modifies the data.

    This might qualify as an exception.

  5. Add this new (well old) related vulnerability:

    G. McGraw and E. W. Felten, Securing Java. Wiley, 1999.
    From SecDev 17: Empirical Studies on the Security and Usability Impact of Immutability:

    For example, in an early release of Java, the method Class.getSigners() returned an array containing the signers of a class. Unfortunately, what was returned was a reference to the actual internal array containing the signers, not a copy of it. Since arrays are mutable, malicious code could alter the system's knowledge of code signers. Thus the concern about mutability affecting security is more broad than just TOCTTOU attacks.