MET07-J. Do not invoke overridable methods on the clone under construction

Calling overridable methods from the clone() method is insecure. First, a malicious subclass may override the method and affect the behavior of the clone() method. Second, a trusted subclass can observe (and potentially modify) the cloned object in a partially-initialized state before its construction has concluded. Consequently, the subclass can leave either the clone, or the object being cloned, or both in an inconsistent state.

Noncompliant Code Example

This noncompliant code example shows two classes, BadClone and Sub. BadClone calls an overridable method doSomething(). The overridden method sets the value of the cookies; the overriding method sets the values of the domain names. The doSomething() method of the subclass Sub is erroneously executed at runtime, because of polymorphism. The subclass not only sees the clone in an inconsistent state, but also modifies the clone in a manner that creates inconsistent copies. This is because the deepCopy() method occurs after the call to the doSomething() method and the overriding doSomething() implementation erroneously modifies the object .

class BadClone implements Cloneable {
  HttpCookie[] cookies;
  
  BadClone(HttpCookie[] c) {
    cookies = c;
  }
 
  public Object clone() throws CloneNotSupportedException {		
    final BadClone clone = (BadClone) super.clone();
    clone.doSomething(); // Invokes overridable method
    clone.cookies = clone.deepCopy();
    return clone;
  }

  void doSomething() { // Overridable
    for (int i = 0; i < cookies.length; i++) {
      cookies[i].setValue("" + i);
    }
  }
  
  HttpCookie[] deepCopy() {
    if (cookies == null) {
      throw new NullPointerException();
    }

    // deep copy
    HttpCookie[] cookiesCopy = new HttpCookie[cookies.length];

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

class Sub extends BadClone {
  Sub(HttpCookie[] c) {
    super(c);
  }

  public Object clone() throws CloneNotSupportedException {		
    final Sub clone = (Sub) super.clone();
    clone.doSomething();
    return clone;
  }
   
  void doSomething() { // Erroneously executed
    for (int i = 0;i < cookies.length; i++) {
      cookies[i].setDomain(i + ".foo.com");
    }
  } 
  
  public static void main(String[] args) throws CloneNotSupportedException {
    HttpCookie[] hc = new HttpCookie[20];
    for (int i = 0 ; i < hc.length; i++){	
      hc[i] = new HttpCookie("cookie" + i,"" + i);
    }
    BadClone bc = new Sub(hc);
    bc.clone();
  }
}

If an overridable method is invoked on a shallow copy of the object, the original object is also modified.

Compliant Solution

This compliant solution declares both the doSomething() and the deepCopy() methods final, preventing overriding of these methods.

final void doSomething() {
  // ...
}
final HttpCookie[] deepCopy() {
  // ...
}

Alternative approaches that prevent invocation of overloaded methods include declaring these methods private, declaring the class final, or eliminating the method calls by congregating the code together.

Risk Assessment

Calling overridable methods on the clone under construction can expose class internals to malicious code or violate class invariants by exposing the clone to trusted code in a partially-initialized state, affording the opportunity to corrupt the state either of the clone, or of the object being cloned, or of both.

Automated Detection

Automated detection appears to be straightforward.

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this guideline on the CERT website.

Other Languages

This guideline appears in the C++ Secure Coding Standard as ARR40-CPP. Use a valid ordering rule.

Bibliography

[[Bloch 2008]] Item 11: Override clone judiciously
[[Gong 2003]]

MET06-J. Do not call overridable methods from a privileged block      05. Methods (MET)      MET08-J. Do not use the clone method to copy untrusted method parameters