![](/confluence/download/attachments/88042725/wiki_logo.gif)
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
Tool | Version | Checker | Description |
---|---|---|---|
CodeSonar | 8.1p0 | JAVA.CLASS.CLONE.CNC | clone Non-cloneable (Java) |
Parasoft Jtest | 2023.1 | CERT.OBJ07.MCNC | Make your classes noncloneable |
Related Guidelines
CWE-498, Cloneable Class Containing Sensitive Information |
Bibliography
"Twelve Rules for Developing More Secure Java Code" | |
Section 10.6, "Java" |
25 Comments
David Svoboda
This rule is technically valid, but needs a lot of filling out on details:
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.
Lokesh Agarwal
But what if the class needs to be further extended?
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
Dhruv Mohindra
Lokesh,
See https://www.securecoding.cert.org/confluence/display/java/SEC34-J.+Do+not+allow+the+unauthorized+construction+of+sensitive+classes (compliant solution in particular)
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.
David Svoboda
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)
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.
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.
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 -
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?
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).
David Svoboda
Here is the simplest program I could generate from the NCCE that does use clone() to bypass the password:
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?
Dhruv Mohindra
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.
Dhruv Mohindra
Rewrote the rule using a doPrivileged example.
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.
Fortunately StringBuffer is final so cannot be cloned. Also the buffer share "optimisation" was removed in 1.5.
Dhruv Mohindra
Thank you for the example. Quick questions -
StringBuffer
non-final at some point? Oddly enough, if one goes by JDK 1.1 API and subsequent releases, the class seems to befinal
.Thomas Hawtin
I believe StringBuffer has always been final.
Dhruv Mohindra
From Sun's secure coding doc Guideline 4-1:
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 theclone
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 theclone
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'sclone
method, the security provided by the two-arg form will be ineffective. This still requires the instance of X to be available in thedoPrivileged()
block to be able to invokeclone()
.Robert Seacord
Most likely we would need to consider this guideline to be non-normative, unless we can define what makes a class "sensitive".
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?
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.
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,
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)](/confluence/s/-vqierp/9012/m4ydwl/_/images/icons/emoticons/sad.svg)
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.
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.