Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.
Comment: scraped doprivileged NCE and added one based on comment

...

Noncompliant Code Example

This noncompliant example derives some functional behavior from the implementation of the class java.lang.StringBuffer, prior to JDK v1.5. A SensitiveClass class is defined which contains a character array used to internally hold a filename, and a Boolean shared variable. When a client requests a String instance by invoking the get() method, the shared flag is set. Operations that can modify the array are subsequently prohibited in order to be consistent with the returned String object. Therefore, 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 illegally work around this constraint even though SensitiveClass does not implement the Cloneable interface.

Here, a malicious class subclasses the non-final SensitiveClass and provides a public clone() method. It proceeds to create its own instance (ms1) and produces a second one (ms2), by cloning the first. It subsequently obtains a new String filename object by invoking the get() method on the first instance. At this point, the shared flag is set to true. Since 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 downplays any security efforts and severely violates the object's invariants.Consider the following noncompliant class definition. Unless someone knows the secret password, objects cannot be created. This is because the constructor for the class checks the input against the expected password before allowing object construction. It also performs a security check. The calling code uses a doPrivileged block to create an instance of SensitiveClass. Object creation beyond this block is prohibited by the security manager. However, since the SensitiveClass advertises a public clone() method, it is possible to circumvent this restriction. This clearly violates the principle of least privilege. Yet another blemish is the inconsistent security checking.

Code Block
bgColor#ffcccc
class SensitiveClass implements Cloneable { {
  private char[] filename;
  private Boolean shared = false;
 
  protected SensitiveClass(String passwdfilename) {
    // perform security manager check
    System.out.println("SensitiveClass construction done!this.filename = filename.toCharArray();
  }

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

  protected String get(){
    if(!shared){	
      shared = true;
     return String.valueOf(filename);
    } else
     throw new Error("Error getting instance");
  }
  
  protected void useprintFilename(){
    System.out.println("In method use()"String.valueOf(filename));
  }
}

class MaliciousSubclass extends SensitiveClass implements Cloneable {	
  protected MaliciousSubclass(String filename) {
    super(filename);
  }
  
  public SensitiveClass Clone() {  // well-behaved clone() method
    SensitiveClass s = null;
    try {
      s = (SensitiveClass)super.clone();	        
    }catch(Exception e) { System.out.println("not cloneable"); }
    return s;
  }
}

class Foo {
public static protected void privilegedmain(String[] args) {
    finalMaliciousSubclass SensitiveClass[]ms1 sc = new SensitiveClass[2];

    AccessController.doPrivileged(new PrivilegedAction() {  MaliciousSubclass("file.txt");
    MaliciousSubclass ms2 public Object= run() {
        sc[0] = new SensitiveClass("password"MaliciousSubclass) ms1.Clone(); // objectcreates creationa withcopy the password
    String s =  sc[0].usems1.get();  //allowed
 returns filename
      return nullSystem.out.println(s); // filename is "file.txt"
    ms2.replace();  }		
    });// replaces all characters with x'
  
  //  sc[1] = sc[0].Clone(); // object creation without the passwordboth ms1.get() and ms2.get() will subsequently return filename = 'xxxxxxxx'
    sc[1].usems1.printFilename();  // thisfilename shouldbecomes not be allowed'xxxxxxxx' 
  }

  public static void main(String[] args) {
    Foo f = new Foo();
    f.privileged();ms2.printFilename(); // filename becomes 'xxxxxxxx'
  }
}

Compliant Solution

Sensitive classes should not implement the Cloneable interface. If the class extends from a superclass that implements Cloneable (and is therefore cloneable), it's clone() method should throw a CloneNotSupportedException. This exception must be caught and handled by the client code.

...

bgColor#ccccff

...

A sensitive class that does not implement Cloneable must also follow this advice.

It is also required to declare SensitiveClass final so as to avoid malicious subclassing. This will stop an artful attacker from subclassing the sensitive class and creating several copies of the subclass, with the intention of introducing thread-safety issues.

Code Block
bgColor#ccccff

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

Risk Assessment

Recommendation

Severity

Likelihood

Remediation Cost

Priority

Level

MSC05-J

medium

probable

medium

P8

L2

...