Why shouldn't clone() be used for defensive copying?

Question

In Effective Java (Chapter 7), it says

Note also that we did not use Date’s clone method to make the defensive copies. Because Date is nonfinal, the clone method is not guaranteed to return an object whose class is java.util.Date: it could return an instance of an untrusted subclass specifically designed for malicious mischief. Such a subclass could, for example, 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. This would give the attacker free reign over all instances. To prevent this sort of attack, do not use the clone method to make a defensive copy of a parameter whose type is subclassable by untrusted parties.

I don't quite understand its explanation. Why does clone() not return a Date object? How can the instance be of untrusted subclass?

Answer 1

clone() is widely regarded to have been a failed experiment for a number of reasons. In this case, someone passing in a Date could have passed in an EvilDate extends Date whose clone() method sneakily returned a copy that was still mutable by someone else.

Answer 2

Consider this code:

public class MaliciousDate extends Date { /** malicious code here **/ }

public class SomeClass {
    public static void main(String[] args) {
        MaliciousDate someDate = new MaliciousDate();
        Date copyOfMaliciousDate = someDate;
        Date anotherDate = copyOfMaliciousDate.clone();
    }
}

Since copyOfMaliciousDate is of type Date, you can call clone() and it will return a Date object, but calling clone on copyOfMaliciousDate executes the code written in the MaliciousDate class' clone() because the instance stored in copyOfMaliciousDate is a MaliciousDate.

Answer 3

I haven't read the book you quoted from, but that paragraph gives a poor justification and offers no protection against any sort of attack.

The quote mentions that an attacker with the ability to load code into your program could potentially submit a Date subclass with malicious methods, for example returning a subclass of Date from clone.

But that's only a minor way an attacker with the ability to load code can cause harm. They could also:

• Use reflection to get read+write access to pretty much anything marked private,
• Mess with the class loaders to load their own versions of classes,
• Call System.exit() to stop your program, and
• Do anything your program could do, like spawn other programs or access files.

If the attacker is running code in your process, the game's over and your process is compromised, and this silly little guard is not going to help.

Maybe you think that clone is bad from a design standpoint, and that's fine, but please don't pretend that not using it will protect you from some security threat, because it won't.

Answer 4

Since Date class is non-final, it can be extended and the clone method can be overridden to store the instances which later can be accessed to modify the state.

MaliciousDate.java

import java.util.ArrayList;
import java.util.Date;
import java.util.List;

public class MaliciousDate extends Date {

    private static List<MaliciousDate> instances = new ArrayList<>();

    @Override
    public Object clone() {
        instances.add(this);
        return super.clone();
    }

    public List<MaliciousDate> getInstances() {
        return instances;
    }
}

Period.java

import java.util.Date;

public class Period {
    private static Date start;
    private static Date end;

    public Period(Date start, Date end) {
        this.start = (Date) start.clone();
        this.end = (Date) end.clone();
        if (start.getTime() > end.getTime()) {
            throw new IllegalArgumentException("start is greater than end");
        }
    }

    public static void main(String[] args) {
        MaliciousDate start = new MaliciousDate();
        MaliciousDate end = new MaliciousDate();
        Period p = new Period(start, end);
        //...
    }

}