SEI CERT Oracle Coding Standard for Java

Space shortcuts

Page tree

Classes containing private, confidential, or otherwise sensitive data are best not copied. If a class is not meant to be copied, then failing to define copy mechanisms, such as a copy constructor, is insufficient to prevent copying.

Java's object cloning mechanism allows an attacker to manufacture new instances of a class by copying the memory images of existing objects rather than by executing the class's constructor. Often, this is an unacceptable way of creating new objects. An attacker can misuse the clone feature to manufacture multiple instances of a singleton class, create thread-safety issues by subclassing and cloning the subclass, bypass security checks within the constructor, and violate the invariants of critical data.

Classes that have security checks in their constructors must beware of finalization attacks, as explained in OBJ11-J. Be wary of letting constructors throw exceptions.

Classes that are not sensitive but maintain other invariants must be sensitive to the possibility of malicious subclasses accessing or manipulating their data and possibly invalidating their invariants (see OBJ04-J. Provide mutable classes with copy functionality to safely allow passing instances to untrusted code for more information).

Noncompliant Code Example

This noncompliant code example defines class SensitiveClass, which contains a character array used to hold a file name, along with a Boolean shared variable, initialized to false. This data is not meant to be copied; consequently, SensitiveClass lacks a copy constructor.

class SensitiveClass {
  private char[] filename;
  private Boolean shared = false;

  SensitiveClass(String filename) {
    this.filename = filename.toCharArray();
  }

  final void replace() {
    if (!shared) {
      for(int i = 0; i < filename.length; i++) {
    	filename[i]= 'x' ;}
    }
  }

  final String get() {
    if (!shared) {
      shared = true;
      return String.valueOf(filename);
    } else {
      throw new IllegalStateException("Failed to get instance");
    }
  }

  final void printFilename() {
    System.out.println(String.valueOf(filename));
  }
}

When a client requests a String instance by invoking the get() method, the shared flag is set. To maintain the array's consistency with the returned String object, operations that can modify the array are subsequently prohibited. As a result, the replace() method designed to replace all elements of the array with an x cannot execute normally when the flag is set. Java's cloning feature provides a way to circumvent this constraint even though SensitiveClass does not implement the Cloneable interface.

This class can be exploited by a malicious class, shown in the following noncompliant code example, that subclasses the nonfinal SensitiveClass and provides a public clone() method:

class MaliciousSubclass extends SensitiveClass implements Cloneable {
  protected MaliciousSubclass(String filename) {
    super(filename);
  }

  @Override public MaliciousSubclass clone() {  // Well-behaved clone() method
    MaliciousSubclass s = null;
    try {
      s = (MaliciousSubclass)super.clone();
    } catch(Exception e) {
      System.out.println("not cloneable");
    }
    return s;
  }

  public static void main(String[] args) {
    MaliciousSubclass ms1 = new MaliciousSubclass("file.txt");
    MaliciousSubclass ms2 = ms1.clone(); // Creates a copy
    String s = ms1.get();  // Returns filename
    System.out.println(s); // Filename is "file.txt"
    ms2.replace();         // Replaces all characters with 'x'
    // Both ms1.get() and ms2.get() will subsequently return filename = 'xxxxxxxx'
    ms1.printFilename();   // Filename becomes 'xxxxxxxx'
    ms2.printFilename();   // Filename becomes 'xxxxxxxx'
  }
}

The malicious class creates an instance ms1 and produces a second instance ms2 by cloning the first. It then obtains a new filename by invoking the get() method on the first instance. At this point, the shared flag is set to true. Because the second instance ms2 does not have its shared flag set to true, it is possible to alter the first instance ms1 using the replace() method. This approach obviates any security efforts and severely violates the class's invariants.

Compliant Solution (Final Class)

The easiest way to prevent malicious subclasses is to declare SensitiveClass to be final.

final class SensitiveClass {
  // ...
}

Compliant Solution (Final clone())

Sensitive classes should neither implement the Cloneable interface nor provide a copy constructor. Sensitive classes that extend from a superclass that implements Cloneable (and are cloneable as a result) must provide a clone() method that throws a CloneNotSupportedException. This exception must be caught and handled by the client code. A sensitive class that does not implement Cloneable must also follow this advice because it inherits the clone() method from Object. The class can prevent subclasses from being made cloneable by defining a final clone() method that always fails.

class SensitiveClass {
  // ...
  public final SensitiveClass clone() 
                              throws CloneNotSupportedException {
    throw new CloneNotSupportedException();
  }
}

This class fails to prevent malicious subclasses but does protect the data in SensitiveClass. Its methods are protected by being declared final. For more information on handling malicious subclasses, see OBJ04-J. Provide mutable classes with copy functionality to safely allow passing instances to untrusted code.

Risk Assessment

Failure to make sensitive classes noncopyable can permit violations of class invariants and provide malicious subclasses with the opportunity to exploit the code to create new instances of objects, even in the presence of the default security manager (in the absence of custom security checks).

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

OBJ07-J

Medium

Probable

Medium

P8

L2

Automated Detection

ToolVersionCheckerDescription
CodeSonar
8.1p0

JAVA.CLASS.CLONE.CNC
JAVA.CLASS.CLONE.SCNC
JAVA.CLASS.CLONE.NF

clone Non-cloneable (Java)
clone Subclass of Non-clonable (Java)
clone not final (Java)

Parasoft Jtest
2024.1
CERT.OBJ07.MCNCMake your classes noncloneable

Related Guidelines

MITRE CWE

CWE-498, Cloneable Class Containing Sensitive Information
CWE-491, Public cloneable() Method without Final (aka "Object Hijack")

Bibliography

[McGraw 1998]

"Twelve Rules for Developing More Secure Java Code"

[Wheeler 2003]

Section 10.6, "Java"


25 Comments

  1. David Svoboda

    This rule is technically valid, but needs a lot of filling out on details:

    • Title ID should be MSC05-J
    • When should classes be cloneable?
    • Need at least one non-compliant code example illustrating why a clonable class is bad. Prob need one compliant solution to illustrate why making the class non-clonable is good. Also need a compliant solution (perhaps with a non-compliant example) demonstrating how to protect a clonable class.
    • Need 'Risk Assessment' and References section.

  2. Thomas Hawtin

    When overriding clone to make the class non-cloneable, it should be left protected and not added to the public API of the class.

    An example of this technique is in java.lang.Enum.

    Making clone final in deliberately cloneable class is not a sensbile idea. By default clone is shallow, which is wrong for the vast majority of classes.

    A better aproach is to arrange for the class to be non-subclassable.

  3. Lokesh Agarwal

    But what if the class needs to be further extended?

    1. David Svoboda

      The JLS has good advice on restricting subclass-ability to just one package (and other available restrictions) that is relevant: http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.8.10

  4. Dhruv Mohindra

    Lokesh,

  5. David Svoboda

    Lokesh, this rule is still generally valid, but your example code doesn't support it. If your secret key is accessible by a protected getKey() method, then the exploit code merely subclasses your class and calls getKey() on your object (which is passed in as a parameter). Game over; no cloning necessary.

    Your introductory paragraph contains a better example for you to flesh out; the reason for preventing cloning is so that malicious code cannot instantiate the class without calling one of its constructors. So your example code should have a class that does something critical in its constructor, such as authenticating a password.

    I just saw SEC12-J. Do not grant untrusted code access to classes existing in forbidden packages, which comes dangerously close to what your rule claims, but studying the rule, it seems to be more focused on preventing instantiations of ClassLoader and other classes directly related to Java Security. So I don't think that rule overlaps with yours (and it should probably get a new title). But you should discuss it and how it relates to your rule.

    1. David Svoboda

      • Lokesh, the non-compliant code has many compiler errors. I think you need to actually build and run the exploit in order to have a solid noncompliant code example + compliant solution.
      • Each solution should have its own 'Compliant Solution' section, even if it has just pseudocode.

  6. Dhruv Mohindra

    It seems the problem is that cloning can leak sensitive information from your class as it does not require executing the constructor that may have security checks built-in it. REF: http://cwe.mitre.org/data/definitions/498.html

    Perfectly valid rule. The title contradicts SEC35-J's recommendation which was my main concern. Perhaps it can be changed to something like - "Make sensitive classes non cloneable"? (Actually non-subclassable might suit it more in this context as Thomas recommended)

  7. Dhruv Mohindra

    A statement in the rule suggests that by implementing the cloneable interface, the subclass can create the instance of the base class MyPrivacy. This might not be completely true the way it is shown currently.

    1. If a superclass does not implement cloneable, it's clone method cannot be overridden by a subclass to get it's instance. So I am unsure on how the current NCCE supports cloning the superclass.

    2. If we do not make the superclass cloneable, the subclass still has to execute a superclass constructor during initialization. Given our authentication requirements, there is no default constructor (without arguments) in the superclass. Thus, the MyPrivacy(passwd) constructor will need to be invoked with the real password and the superclass constructor will eventually block access. Cloning of the class would be performed only after this step.

    3. It also assumes that somefunction() is passed a MyPrivacy obj which would mean that the constructor for it has to get executed first.

    One thing that can be done is to override Object.clone() (see code snippet below) in the 'Test' class and have it call cloned.use() which should be usually forbidden until authentication has concluded. But this is only contingent upon the success of Point 1.

    class CloneCaller extends MyPrivacy implements Cloneable {

 

 public CloneCaller(String passwd) {
        super(passwd);
 }


 protected Object clone() throws CloneNotSupportedException {
        SecurityManager sm = System.getSecurityManager();
        if (sm != null) {
          //check permissions
        }
        final MyPrivacy cloned = (MyPrivacy)super.clone();
        cloned.use();   //invoke method use() without authenticating
        return cloned;
 }


 public static void main(String[] args) throws CloneNotSupportedException {
	CloneCaller c = new CloneCaller("password!");
	c.clone();
 }
}

    The NCCE also needs to catch the usual exceptions (to compile cleanly) but again in doing so one has to be careful about OBJ32-J. Do not allow partially initialized objects to be accessed as throwing exceptions in constructors of non-final classes might backfire.

    Recommendation: Change the NCCE so that it implements cloneable and demonstrate why this can be dangerous.

    1. Dhruv Mohindra

      I couldn't come up with a reasonable example for exploiting the fact that cloning does not require executing the constructor. Two cases worth mentioning -

      • Case 1: For a non-final base class, a subclass requires the base's object to do a clone of the Base, within it. If it already has the (sensitive) Base object, it is not harmful to provide that code with a second copy unless someone does not know that the second object is a shallow copy and the original data can be modified with it. (IMO a separate issue)
      • Case 2: If the superclass forgets to do a super.clone() in its clone method, its (sensitive) instance would be erroneously retrieved instead of the subclass'. But to return the instance, the superclass's clone() method would itself have to invoke one of its own constructors. (security check in constructor is still invoked and the initialization too, making this rule moot)

      Finally, the example CWE 498 doesn't quite impress me. (see case 1; also the shallow copying ensures that the invariants of the new copy are preserved)

      Anyone has any ideas for a non-contrived example?

      1. Thomas Hawtin

        An example would be defeating intrinsic synchronization on this. Create a (subclassed) legitimate object. Then clone it. You now have two copies of essentially the same object, but now it is not thread-safe.

        As above I mentioned above, I don't think singling out the clone method is a very good idea. General solutions include forbidding subclassing and forwarding to an implementation object (much like the C++ pimpl idiom).

        1. David Svoboda

          Here is the simplest program I could generate from the NCCE that does use clone() to bypass the password:

          import java.io.*;

class MyPrivacy {

    //define class member fields
    //...

    public MyPrivacy(String passwd) {
	System.out.println("MyPrivacy with password");
    }

    public void use(){
	System.out.println("MyPrivacy.use()");
    }
};


class Baz extends MyPrivacy implements Cloneable {
    Baz(String passwd) {super(passwd);}
    static Baz instance = new Baz("password");

    public static void main(String[] args) {
	try {
            Baz t = (Baz)instance.clone();
	    if (t != instance)
		t.use(); // Another object instantiated without knowing the password.....
	}catch(Exception e) {
            System.out.println("not cloneable");
        }
    }
}

          The program does create a Baz object, supplying the password, and then clones this object. Ergo, it gets two MyPrivacy objects via one password. One can conceive of a NCCE scenario where the 1st Baz object is done by privileged code with the password, and the 2nd Baz object is cloned by unprivileged code.

          Without adding other ctors to MyPrivacy, I can't see any way to get an initial Baz object without supplying the password. So I can't create a malicious subclass to MyPrivacy without the password.

          The exploit described in the NCCE does not compile, simply because the obj.clone() method is protected (obj is a MyPrivacy which is not explicitly cloneable).

          Bottom line, I now think this rule is very weak. Specifically, if you have a class with a 'gateway' constructor, and you have an object of a subclass that is cloneable, you can bypass the gateway constructor by cloneing the object. The best fix for such a scenario is prob not to prevent cloning of your gateway class but rather to prevent anyone who wants to subclass the gateway class from getting the password. Or make your gateway class non-subclassable (eg final). Don't we have rules for that already?

          So I now recommend we move this rule to the void. Comments?

          1. Dhruv Mohindra

            1st Baz object is done by privileged code with the password, and the 2nd Baz object is cloned by unprivileged code.

            That might require the privileged code to be within the same class as malicious code (which sounds slightly contrived). If it's not, then for the exploit to succeed we'll need to pass privileged objects around to untrusted code (more of a violation of SEC02-J. Guard doPrivileged blocks against untrusted invocation and leakage of sensitive data than this rule). This condition falls under Case 1 of my comment.

            I do feel the guideline is redundant. The best I can do is to use this example in the SCP01-J. Do not increase the accessibility of overridden or hidden methods which does need more examples. The other option is to describe the thread-safety (shallow copy etc.) issue that Mr. Hawtin hinted at. For the latter, this rule will need a small makeover.

            1. Dhruv Mohindra

              Rewrote the rule using a doPrivileged example.

              1. Thomas Hawtin

                I'm not keen on bringing the access controller mechanism into this.

                As an example of an issue: Consider the StringBuffer implementation from 1.4 and earlier. When you created a String with a StringBuffer the char[]. When the StringBuffer was subsequently modified, it would copy the char[]. StringBuffer consisted of a reference to the char[], a shared flag and some other stuff.

                Consider the consequences of StringBuffer not being final (although perhaps all of its methods were). An adversary could subclass StringBuffer and implement final. Load the StringBuffer subclass instance with some data. Clone the StringBuffer. Create a String from one of the StringBuffers. Both StringBuffers and also the String share the same char[]. Only one of the StringBufffers has the shared flag set. The StringBuffer without the flag set can be used to alter the String.

                class MaliciousStringBuffer extends StringBuffer implements Cloneable {
    public StringBuffer klone() {
        try {
            return clone();
        } catch (CloneNotSupportedException exc) {
            throw new Error(exc);
        }
    }
}

final MaliciousStringBuffer a = new MaliciousStringBuffer("/tmp");
final StringBuffer b = (StringBuffer)(a.klone());
final String s = a.toString();
// a.shared is true; b.shared is false;
// s is equal to "/tmp"
b.replace(0, 4, "/usr");
// s is equal to "/usr"

                Fortunately StringBuffer is final so cannot be cloned. Also the buffer share "optimisation" was removed in 1.5.

                1. Dhruv Mohindra

                  Thank you for the example. Quick questions -

                  • Was StringBuffer non-final at some point? Oddly enough, if one goes by JDK 1.1 API and subsequent releases, the class seems to be final.
                  • Is there any resource that we can add as a reference?

                  1. Thomas Hawtin

                    I believe StringBuffer has always been final.

  8. Dhruv Mohindra

    From Sun's secure coding doc Guideline 4-1:

    Also enforce checks at points where an instance of a class can be created without the use of a constructor. Specifically, enforce a check inside ... the clone method of a cloneable class.

    To invoke the Clone method you have to create an instance of the class => executing its constructor. If the constructor has the security check why should the clone method need one too? I assume that the class protects against partially initialized instances and even if it doesn't, it is vulnerable through the finalizer attack which doesn't need the clone method to obtain an instance, anyhow.

    One way I can imagine is when you use the two arg form of doPrivileged() to strip the permissions required to create some class X. But because that code section may have an instance of the class already available (as all other code of the class is permitted to create the instance of class X), if an attacker can trick the doPrivileged() code into invoking the class's clone method, the security provided by the two-arg form will be ineffective. This still requires the instance of X to be available in the doPrivileged() block to be able to invoke clone().

  9. Robert Seacord

    Most likely we would need to consider this guideline to be non-normative, unless we can define what makes a class "sensitive".

  10. Robert Seacord

    I'm not too crazy about the vagueness in the second sentence:

    Java's object cloning mechanism allows an attacker to manufacture new instances of a class by copying the memory images of existing objects rather than by executing the class's constructor. Often this is an unacceptable way of creating new objects.

    Can we identify the exact conditions under which this approach is unacceptable?

    1. David Svoboda

      I think the danger is when a class's constructor serves as a security measure; eg it only allows 'legit' objects to be constructed. If a ctor provides some security mechanism (eg requires a username/password), then clone() can bypass the mechanism. (So can serialization.) The implementation details of clone() are actually not important.

  11. I Gede Panca Sutresna

    Hi there..

    again..

    1. OBJ05-J. Do not allow access to partially initialized objects should changed to OBJ11-J. Be wary of letting constructors throw exceptions and

    2. OBJ08-J. Provide mutable classes with copy functionality to allow passing instances to untrusted code safely should change to OBJ04-J. Provide mutable classes with copy functionality to allow passing instances to untrusted code safely

    Best Regards,

    1. David Svoboda

      Sorry for the repetition. We will have someone traverse the Java wiki and fix all links like this. The bad links seem to be caused by editing the wiki using 'Rich Text' format (sad)

  12. James Ahlborn

    It would seem that the ultimate implication of this rule is that all sensitive classes must be synchronized (or otherwise made thread-safe).  Even if the SensitiveClass is made final, multiple threads calling the replace() and get() methods simultaneously could still successfully get the char[] as a String value and cause it to be replaced with 'x's.

    1. Yitzhak Mandelbaum

      Seems to be the case, although its hard to reason in the abstract about the interaction between class invariants and thread-safety. Still, I think it safe to say that any class invariant that relies on mutation of state will be at high risk for invariant-violating race conditions and, hence, must be made thread safe.