Skip to end of metadata
Go to start of metadata

Returning references to internal mutable members of a class can compromise an application's security, both by breaking encapsulation and by providing the opportunity to corrupt the internal state of the class (whether accidentally or maliciously). As a result, programs must not return references to private mutable classes.

See rule OBJ13-J. Ensure that references to mutable objects are not exposed for details about leaking references to non-private objects.

Noncompliant Code Example

This noncompliant code example shows a getDate() accessor method that returns the sole instance of the private Date object:

class MutableClass {
  private Date d;

  public MutableClass() {
    d = new Date();
  }

  public Date getDate() {
    return d;
  }
}

An untrusted caller can manipulate a private Date object because returning the reference exposes the internal mutable component beyond the trust boundaries of MutableClass.

Compliant Solution (clone())

Returning a reference to a defensive copy of a mutable internal state ensures that the caller cannot modify the original internal state, although the copy remains mutable. This compliant solution returns a clone of the Date object from the getDate() accessor method. Although Date can be extended by an attacker, this approach is safe because the Date object returned by getDate() is controlled by MutableClass and is known to be a nonmalicious subclass.

public Date getDate() {
  return (Date)d.clone();
}

Note that defensive copies performed during execution of a constructor must avoid use of the clone() method when the class could be subclassed by untrusted code. This restriction prevents execution of a maliciously crafted overriding of the clone() method (see OBJ07-J. Sensitive classes must not let themselves be copied for more details).

Classes that have public setter methods, that is, methods whose purpose is to change class fields, must follow the related advice found in OBJ06-J. Defensively copy mutable inputs and mutable internal components. Note that setter methods can (and usually should) perform input validation and sanitization before setting internal fields.

Noncompliant Code Example (Mutable Member Array)

In this noncompliant code example, the getDate() accessor method returns an array of Date objects. The method fails to make a defensive copy of the array before returning it. Because the array contains references to Date objects that are mutable, a shallow copy of the array is insufficient because an attacker can modify the Date objects in the array.

class MutableClass {
  private Date[] date;

  public MutableClass() {
    date = new Date[20];
    for (int i = 0; i < date.length; i++) {
      date[i] = new Date();
    }
  }

  public Date[] getDate() {
    return date; // Or return date.clone()
  }
}

Compliant Solution (Deep Copy)

This compliant solution creates a deep copy of the date array and returns the copy, thereby protecting both the date array and the individual Date objects:

class MutableClass {
  private Date[] date;

  public MutableClass() {
    date = new Date[20];
    for(int i = 0; i < date.length; i++) {
      date[i] = new Date();
    }
  }

  public Date[] getDate() {
    Date[] dates = new Date[date.length];
    for (int i = 0; i < date.length; i++) {
      dates[i] = (Date) date[i].clone();
    }
    return dates;
  }
}

Noncompliant Code Example (Mutable Member Containing Immutable Objects)

In this noncompliant code example, class ReturnRef contains a private Hashtable instance field. The hash table stores immutable but sensitive data (for example, social security numbers [SSNs]). The getValues() method gives the caller access to the hash table by returning a reference to it. An untrusted caller can use this method to gain access to the hash table; as a result, hash table entries can be maliciously added, removed, or replaced. Furthermore, multiple threads can perform these modifications, providing ample opportunities for race conditions.

class ReturnRef {
  // Internal state, may contain sensitive data
  private Hashtable<Integer,String> ht = new Hashtable<Integer,String>();

  private ReturnRef() {
    ht.put(1, "123-45-6666");
  }

  public Hashtable<Integer,String> getValues(){
    return ht;
  }

  public static void main(String[] args) {
    ReturnRef rr = new ReturnRef();
    Hashtable<Integer, String> ht1 = rr.getValues(); // Prints sensitive data 123-45-6666
    ht1.remove(1);                                   // Untrusted caller can remove entries
    Hashtable<Integer, String> ht2 = rr.getValues(); // Now prints null; original entry is removed
  }
}

In returning a reference to the ht hash table, this example also hinders efficient garbage collection.

Compliant Solution (Shallow Copy)

Make defensive copies of private internal mutable object state. For mutable fields that contain immutable data, a shallow copy is sufficient. Fields that refer to mutable data generally require a deep copy.

This compliant solution creates and returns a shallow copy of the hash table containing immutable SSNs. Consequently, the original hash table remains private, and any attempts to modify it are ineffective.

class ReturnRef {
  // ...
  private Hashtable<Integer,String> getValues(){
    return (Hashtable<Integer, String>) ht.clone(); // Shallow copy
  }

  public static void main(String[] args) {
    ReturnRef rr = new ReturnRef();
    // Prints nonsensitive data
    Hashtable<Integer,String> ht1 = rr.getValues(); 
    // Untrusted caller can only modify copy
    ht1.remove(1); 
    // Prints nonsensitive data
    Hashtable<Integer,String> ht2 = rr.getValues(); 
  }
}

When a hash table contains references to mutable data such as Date objects, each of those objects must also be copied by using a copy constructor or method (refer to OBJ06-J. Defensively copy mutable inputs and mutable internal components and OBJ04-J. Provide mutable classes with copy functionality to safely allow passing instances to untrusted code for further details).

Note that making deep copies of the keys of a hash table is unnecessary; shallow copying of the references suffices because a hash table's contract dictates that its keys must produce consistent results to the equals() and hashCode() methods. Mutable objects whose equals() or hashCode() method results may be modified are not suitable keys.

Exceptions

OBJ05-J-EX0: When a method is called with only an immutable view of an object, that method may freely use the immutable view without defensive copying. This decision should be made early in the design of the API. Note that new callers of such methods must also expose only immutable views.

Risk Assessment

Returning references to internal object state (mutable or immutable) can render an application susceptible to information leaks and corruption of its objects' states, which consequently violates class invariants. Control flow can also be affected in some cases.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

OBJ05-J

High

Probable

Medium

P12

L1

Automated Detection

Sound automated detection is infeasible; heuristic checks could be useful.

ToolVersionCheckerDescription
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

Pugh [Pugh 2009] cites a vulnerability discovered by the Findbugs static analysis tool in the early betas of JDK 1.7 in which the sun.security.x509.InvalidityDateExtension class returned a Date instance through a public accessor without creating defensive copies.

Related Guidelines

Bibliography

[API 2014]

Method clone()

[Bloch 2008]

Item 39, "Make Defensive Copies When Needed"

[Goetz 2006]

Section 3.2, "Publication and Escape: Allowing Internal Mutable State to Escape"

[Gong 2003]

Section 9.4, "Private Object State and Object Immutability"

[Haggar 2000]

Practical Java Praxis 64. Use clone for immutable objects when passing or receiving object references to mutable objects

[Pugh 2009]

[Security 2006]




6 Comments

  1. This rule is way too strong. It is standard practice to return objects from methods, and given OBJ00-J. Declare data members as private and provide accessible wrapper methods, it is likely that many objects returned are private data members. Some valid instances of returning private data members:

    • Immutable objects, such as String. If you can't change the state of an object, what's the harm in giving it to untrusted code?
    • Some classes, such as Vector and other containers, hold objects intended to be manipulated by untrusted code
    • Some classes maintain a hierarchy of objects (GUI widgets, for instance) that are publicly editable. GUI widgets are advertised as not being thread-safe, and you should never assume a method or object is thread-safe, unless it explicitly claims to be.

    I suspect we'll have to demote this to a recommendation.

    1. To substantiate my claim wrt vectors, here is code from Sun's OpenJVM:

      From java-6-openjdk/src.zip java/util/Vector.java:

          protected Object[] elementData;
      //...
          public synchronized E elementAt(int index) {
              if (index >= elementCount) {
                  throw new ArrayIndexOutOfBoundsException(index + " >= " + elementCount);
              }
      
              return elementData(index);
          }
      

      Here's another example, from java/util/logging/LogManager.java:

          private static LogManager manager;
      //...
          public static LogManager getLogManager() {
              if (manager != null) {
                  manager.readPrimordialConfiguration();
              }
              return manager;
          }
      

      After today's discussion, my current thinking on this rule is that it is still valid, with the following qualifications:

      • The private item must be an Object, not a primitive type (because primitive types get copied when returned from a method).
      • The private item must be mutable; fixed private objects like String introduce no vuls from becoming world-readable.
      • Encapsulation is not transitive in Java; ergo it is ok to return a reference to an object accessible from the private object as long as the two references are distinct. Best illustrated by code:
      class B {
      		public C c;
      };
      
      class A {
      		private B b;
      		public getB() {return b;}  // should violate rule
      		public getC() {return b.c;}  // should not violate rule
      };
      

      This permits a Vector the ability to hand out references to its array's elements, while denying it the ability to hand out a ref to the array itself.

      This does not excuse the LogManager example, however.

      In that example, there is a global static LogManager object, marked private. Which forces everyone to use getLogManager to access it; thereby negating the 'private' keyword.

      As was argued at lunch today, this 'faux-public' access technique has advantages over simply declaring the data to be public. Making a data member private and providing getter access allows you to finely control how easily outside code can access or modify the data, whereas providing public data denies you the ability to restrict getting/setting the data completely. Furthermore, providing getter/setter access makes it easy to change said access in the future if necessary.

      I suspect the Java community will prevail and declare that 'faux-public' access (that is, making a data member private and providing a public getter method) is a valid mechanism and not a vulnerability. The vulnerability comes in providing access to data not meant to be 'faux-public' (eg data that is 'sensitive'). And the Java language does not support the concept of 'sensitive', so such a rule would not be automatically enforceable. (perhaps you could solve this problem with a @sensitive annotation?)

      1. Here are some of my thoughts:

        • Primitive types should also violate this guideline. Arrays are treated as objects in Java. So if the caller modifies the array elements, the original values are also modified. The difference is that shallow copying (as opposed to deep copying) before returning is enough for primitive types.
        • Returning immutable objects without creating copies is not a problem
        • Logmanager appears to be a (non-stateful) singleton so it doesn't require a copy to be returned. In fact, the goal is quite the opposite i.e. not creating multiple copies.
        • Having many accessor methods indicates bad design. A class must implement as much functionality as possible within itself. The goal should be to achieve immutability.
        • When the above is not possible and copying/cloning is too expensive, an unmodifiable view of the object can be exposed.
        • A violation of this rule would occur if the caller of the accessor method actually changes the contents of the private object. But, it is good defensive programming to assume that objects may fall into the hands of untrusted code. As far as possible one should take appropriate steps to mitigate these risks (by cloning etc.).
        • It can be argued that if there is a public setter method, this guideline is useless. But again, setter methods can perform functions such as validating data before storing it. If you return references to private data, the untrusted code can even enter invalid input into a trusted object.
        • Lastly, wrt vectors I'll quote Sun's secure coding doc:

          Clearly, mutable (including non-final) inputs and outputs place a significant burden on method implementations. To minimize this burden, favor immutability when designing new classes 8. In addition, if a class merely serves as a container for mutable inputs or outputs (the class does not directly operate on them), it may not be necessary to create defensive copies. For example, arrays and the standard collection classes do not create copies of caller-provided values. If a copy is desired so updates to a value do not affect the corresponding value in the collection, the caller must create the copy before inserting it into the collection, or after receiving it from the collection.

        This guideline clearly needs a lot more work. Thanks for raising the questions...comments are welcome.

  2. I would like to see the reference to the Secure Coding Guidelines, V 2.0 changed into a reference to Secure Coding Guidelines, V 3.0. Here are some candidates:

    1. Guideline 2-2 Create copies of mutable outputs
    2. Guideline 2-2a Create safe copies of mutable and subclassable inputs
    3. Guideline 2-2b Create copies of mutable objects passed to untrusted objects

    rCs

  3. I can't understand what OBJ05-EX0 means.
    what does it mean "callers expose ... to a method"?
    anyone please rewrite this to more plain english?

    1. I've rewritten the exception's sentence more clearly.
      OBJ05-EX0 is just an identifier for this exception. If this rule had another exception, it would be OBJ05-EX1. We often have to refer to exceptions in other rules (eg this code example does not violate OBJ05-J because it falls under OBJ05-EX0.)