Why defensive copying with clone represents a security issue?

Question

These days I am reading the second edition of Effective Java by Joshua Bloch. In the item 39 he mentions that it is a good idea to make defensive copies of mutable objects passed as arguments, say in constructors of a given class Foo, if these objects are later on used to represent the state of the class Foo. In the same context he mentions to avoid using the clone() method of non-final classes, as it could return an instance of an untrusted subclass designed to perform malicious operations.

Here is what I don't clearly get. As an example of a malicious subclass he mentions a class that could "record a reference to each instance in a private static list at the time of its creation and allow the attacker to access this list".

My doubts:

1. Does he mean that this malicious class can actually record references of all private/protected/package/public instances of the encapsulating class?

2. If so, how could that be possible?. Could you please provide me an example?

Thx!

Methyl

Answer 1

As is typical with security, it's important to set out the context this applies to. We're interested in cases where potentially malicious code can access the attacked trusted class. For instance within the browser Java PlugIn trusted libraries can be accessed by untrusted code. It used to be the case that RMI loaded remote code, however it has now fallen into line with policy of secure by default.

The issue with mutable arguments is that they can be changed between the time they are checked for validity and they are used. This is known as a Time Of Check/Time Of Use vulnerability, TOCTOU (or TOC2TOU). In practice, this can be two uses rather than one use specifically being a check. Other badly designed classes that appear immutable but are subclassable (for instance java.io.File), can be subclassed to be mutable as part of their ability to execute arbitrary code when invoked.

The specific attack scenario being discussed here is where the clone is overridden to to thwart the attempt at copying. The reference to a static is irrelevant in this context (it's important in finalizer attacks, but mostly reflects that attack code is rarely designed to be clean).

class MaliciousDate {
    private final List<MaliciousDate> dates;
    public MaliciousDate(List<MaliciousDate> dates) {
        this.dates = dates;
    }
    @Override public MaliciousDate clone() {
        MalicousDate other = (MalicousDate)super.clone(); // Or new MalicousDate
        synchronized (dates) {
            dates.add(other);
        }
        return other; // Or return this;
    }
}

To modify the example from the book.

public Period(Date start, Date end) {
    // Failing defensive copy.
    start = (Date)start.clone();
    end   = (Date)end  .clone();

    if (start.compareTo(end) > 0)
        throw new IllegalArgumentExcpetion();
    this.start = start;
    this.end = end;
} 

Then attack with:

List<MaliciousDate> dates = new ArrayList<>()
Date start = new MaliciousDate(dates);
Date end = new MaliciousDate(dates);
Period p = new Period(start, end);
dates.get(1).setYear(78); // Modifies internals of p!

Conclusion: Make your value types robustly immutable. More information in the completely awesome Secure Coding Guidelines for the Java Programming Language.

Leatherwood

Answer 2

The malicious subclass's clone method can access its superclass's private members via the getDeclaredFields method - this returns all of the superclass's fields, even the ones that were declared private.

I believe what the book is referring to is that the clone method could also store a list of all instances instantiated via the clone method.

class MaliciousClass extends LegitimateClass {
  public static ArrayList privateData = new ArrayList()
  public static ArrayList clonedInstances = new ArrayList();
  protected Object clone() {
    Fields[] fields = this.getSuperclass().getDeclaredFields();
    for(Field field: Fields) {
      privateData.add(field.get(this));
    }
    Object clonedObject = // perform clone, returning an instance of MaliciousClass
    clonedInstances.add(clonedObject);
    return clonedObject;
  }
}

Unintentional

Surely it could do that anyway, without clone? And if a SecurityManager is installed getDeclaredFields will throw a SecurityException (unless explicitly allowed). – Aeromechanic

Yeah, reflection is more or less limited to what you could do anyway. It's when trusted code uses reflection for some dodgy purpose that vulnerabilities are introduced. It's just really basic features of the Java programming language itself (and how it is used) that causes the issues here. – Leatherwood