Secure Java Code IQ Test & Answer Sheet

Even with advanced security architecture and built-in security features, Java isn't immune to security risks. Take this IQ test to see if you are aware of writing a more secure Java code.


1.   Which line of the following code may have a security problem?

1. class BadCode {
2.    int num;
3.    public int getNum() {
4.        return num;
5.    }
6.    public void setNum(int num) {
7.        this.num = num;
8.    }
9.    ...
10. }
A.    line 1
*B.    line 2
C.    line 4
D.    line 7

Answer: The line 2. The variable num should be declared private, because it may be accessed via an object instance without using set & get methods. This provides a potential entry point for an attacher. For example,


class BadCode {
   int num;
   public int getNum() {
       return num;
   }
   public void setNum(int num) {
       this.num = num;
   }
   //...
 }

class Test {
    public static void main(String[] args) {
        BadCode bc = new BadCode();
        bc.num = 5;
        System.out.println(bc.getNum());
    }
}
>javac Test.java
>java Test
5

Rule: Making all variables private is an easy way to secure your data.


2.    Assume Foo has been defined somewhere. Which code below has a security problem?

1. class BadCode {
2.    private Foo foo;
3.    public BadCode (Foo f) {
4.        foo = f;
5.    }
6.    public BadCode() {}
7.    protected void useFoo() {
8.        int num = foo.num;
9.        String name = foo.name;
10.        .... 
11.   }
12.    ....
13. }
A.    line 2
B.    line 3
C.    line 4
*D.    line 8

Answer: line 8. Before you use Foo object, you should check if foo has been initialized or not. It is possible to locate an object of Test without calling the constructor that does initialization. If the foo is not initialized line 8 will cause program to crash or throw a null point exception. For example, the following code will cause the program to crash.

class BadCode {
    private Foo foo;
    public BadCode (Foo f) {
        foo = f;
    }
    public BadCode() {}
    protected void useFoo() {
        int num = foo.num;
        String name = foo.name;
        //.... 
   }
   // ....
 }

class Foo {
   int num = 5;
   String name = "here we go";
}

class Test extends BadCode{
  Test(){}
  public static void main(String[] args){
      Test t = new Test();
      t.useFoo();
  } 
}
>javac Test.java
>java Test
Exception in thread "main" java.lang.NullPointerException
        at BadCode.useFoo(BadCode.java:9)
        at Test.main(BadCode.java:24)

Rule: Make sure each object has been initialized before being used.


3.   Jack wants to design a fully protected class that is not extensible. Which line may have the problem with his initial intent?

1. class MyPrivacy {
2.    private int key = 1234;
3.    //...
4.    protected void setKey(int privateKey) {
5.        this.key = privateKey;
6.    }
7.    protected int getKey() {
8.        return this.key;
9.    }
10.    //...
11. }

*A.    line 1
B.    line 2
C.    line 4
D.    line 8

Answer: line 1. The class should be declared final at least to prevent extensibility. If possible, some methods should be declared final too. For example, the following code will break his initial intent.

class MyPrivacy {
    private int key = 1234;
    //...
    protected void setKey(int privateKey) {
        this.key = privateKey;
    }
    protected int getKey() {
        return this.key;
    }
    //...
}

class Test extends MyPrivacy{
    
    private void changeKey(int key) {
        int newKey = key + 1111;
        setKey(newKey);
    }
    public static void main(String[] args) {
        Test t = new Test();
        t.setKey(5678);
        t.changeKey(5678);
        System.out.println(t.getKey());
    }
}

>javac Test.java
>java Test
6789

Rule: Use final keyword to classes and methods wherever is neccessary to protect malicious inheritance or attacks.


4.    Mike thought the following code would guarantee the private field key initialized by the parameterized constructor, so the set method is not neccessary. Is that true?

class MyPrivacy {
    private int key = 1234;
    //...
    public MyPrivacy() {}
    public MyPrivacy(int k) {
        this.key = k;
    }
    protected int getKey() {
        return this.key;
    }
   
    //...
}
A.    True
*B.    False

Answer: False. The above code is not guaranteed to be safe. An attacker can create a new instance of MyPrivacy class by using a cloneable subclass and by-pass the constructor and leave the field not initialized or steal your data. The following code will prove that:

//easy to be attacked code
class MyPrivacy {
    private int key = 1234;
    //...
    public MyPrivacy() {}
    public MyPrivacy(int k) {
        this.key = k;
    }
    protected int getKey() {
        return this.key;
    }
    
    //...
}

class Test extends MyPrivacy implements Cloneable{
    private int dummy;
    Test(int d) {
        dummy = d;
    } 
    public static void main(String[] args) {
        Test t = new Test(0);
        Object obj = null;
	try {
            obj = t.clone();
        }catch(Exception e) {
            System.out.println("not cloneable");
        }
        if (obj != null)
            System.out.println(((MyPrivacy)obj).getKey());//steal key
    }
}

>javac Test.java
>java Test
1234

Java's object cloning mechanism can allow an attacker to manufacture new instances of classes you defined, without executing any of your constructors. The new instances are made by copying the memory images of the existing objects. To prevent it, you must provide a noncloneable method in each class with the following signature:

public final Object clone() throws java.lang.CloneNotSupportedException {
    throw new java.lang.CloneNotSupportedException();
}

If you add the above method to the MyPrivacy class, the class is protected to be noncloneable:

//improved noncloneable class
class MyPrivacy {
    private int key = 1234;
    //...
    public MyPrivacy() {}
    public MyPrivacy(int k) {
        this.key = k;
    }
    protected int getKey() {
        return this.key;
    }
    public final Object clone() throws java.lang.CloneNotSupportedException {
           throw new java.lang.CloneNotSupportedException();
    }
    //...
}

class Test extends MyPrivacy implements Cloneable{
    private int dummy;
    Test(int d) {
        dummy = d;
    } 
    public static void main(String[] args) {
        Test t = new Test(0);
        Object obj = null;
	try {
            obj = t.clone();
        }catch(Exception e) {
            System.out.println("not cloneable");
        }
        if (obj != null)
            System.out.println(((MyPrivacy)obj).getKey());
    }
}

>javac Test.java
>java Test
not cloneable

If you want your class cloneable, but prevent it to be rewritten or redefined, use the following code signature in each of your classes.

public final Object clone() throws java.lang.CloneNotSupportedException {
    super.clone();
}

Rule: To protect your class aggressively by blocking cloneability.


5.   Aaron doesn't want his class to be serialized in anyway, how to do that?
A.    Make his class final
B.    Make his class static
C.    Define a readObject() method in his class
*D.    Define a writeObject() method in his class

Answer: Aaron needs special handling for his class, so he has to declare a writeObject() method with the following signature

private void writeObject(ObjectOutputStream out)
                           throws java.io.IOException {
        throw new java.io.NotSerializableException();
}

According to Java API, the writeObject() method is responsible for writing the state of the object for its particular class so that the corresponding readObject method can restore it. The method does not need to concern itself with the state belonging to the object's superclasses or subclasses. State is saved by writing the individual fields to the ObjectOutputStream using the writeObject method or by using the methods for primitive data types supported by DataOutput.

Classes that do not implement java.io.Serializable will not have any of their state serialized or deserialized. But its subclass may be serialized and is assumed to take care of the non-serialized super class. For example:

//subclass is serialized
import java.io.*;

class MyPrivacy {
    private int key = 1234;
    //...
    public MyPrivacy() {}
    public MyPrivacy(int k) {
        this.key = k;
    }
    protected int getKey() {
        return this.key;
    }
    protected void setKey(int key) {
        this.key = key;
    }
    private void writeObject(ObjectOutputStream out)
                           throws IOException {
        throw new NotSerializableException();
    }
    //...
}

class Test extends MyPrivacy implements java.io.Serializable{
    Test(){}
    Test(int i){
        super(i);
    }
    public static void main(String[] args) {

        //write an object to a file
        try{
            ObjectOutputStream os = new ObjectOutputStream
                    (new FileOutputStream("test.txt"));
        
            os.writeObject(new Test(9999));
            System.out.println("Writing an object to a file with key value: 9999");
        }catch(Exception e) {
            e.printStackTrace();
            System.exit(1);
        }

        //read back the object
        try{
            ObjectInputStream is = new ObjectInputStream
                    (new FileInputStream("test.txt"));
        
            Test t = (Test)is.readObject();
            System.out.println("Read an object, but its key value is: "+ t.getKey());
        }catch(Exception e) {
            e.printStackTrace();
            System.exit(1);
        }
    }
}

>javac Test.java
>java Test
Writing an object to a file with key value: 9999
Read an object, but its key value is: 1234

Note that the state you wrote to the file is supposed to be key 9999, but you got key value 1234, which is a default key value. because the super class is not serialized, its state is not written out to a file or a stream.

If you want to protect your class to be deserialized, add readObject() method to your class.

private void readObject(java.io.ObjectInputStream stream)
                         throws IOException, ClassNotFoundException;

Rule: You can prevent your class from serializing or deserializing by defining specific methods


6.   Jerry wants to compare two classes to see whether they are the same. He wrote a piece of code as follows. Is the code below OK?

public boolean match(Object o) {
    return this.getName() == ((SomeClass)o).getName();
}
A.    The code is OK.
*B.    The code is not OK.

Answer: It is not OK because it is possible that there are many different classes carrying same name in JVM. The better way to do so is to compare them by its class like the following:

public boolean match(Object o) {
    return this.getClass() == o.getClass();
}

Rule: Don't compare class objects by their names.


7.   Betty wants to have a field that stores sensitive data like ID or SSN, which of the following data types is the best to choose?
A.    String
B.    StringBuffer
C.    char
*D.    char array

Answer: Use char array, which is easy to encrypt or decrypt.

You cannot use a char type, because it can only hold a single character.

Java strings are immutable, you cannot write to them, and it is also difficult to clear them. If you use String instance to store data, it is possible to give an attacher a chance to track the data in memory. Therefore, for the sensitive data, you should not use String to store them.

A string buffer is like a String, but can be modified. Its length and content of the sequence can be changed through certain method calls, so it may not be used to store sensitive data in this case.

Rule: For important data, don't store it in a string, use char array instead.


8.   The following code is very commonly exampled, but there is a less secure issue. Which line of code is affected? Assume the Foo is a class defined somewhere.

//not perfect code
1. public class Test {
2.     private Foo foo = null;
3.     ....
4.     public void setFoo(Foo foo) {
5.         this.foo = foo;          
6.     }
7.     public Foo getFoo() {
8.         return foo;
9.     }
10.
11.    ....
12. }
A.    line 1
B.    line 2
C.    line 5
*D.    line 8

Answer: Line 8 is a less secure code.

The foo is a self-defined object here. Its state might be changed by the caller. If so, it would be possible for some other internal methods of the class to pick up a different state, especially in multi-threading environment. For safe, make a clone of the foo and return to the caller. A possible solution:

//improved code
 public class Test {
     private Foo foo = null;
     ....
     public void setFoo(Foo foo) {
         this.foo = foo;          
     }
     public Foo getFoo() {
         return new Foo(foo);
     }

    ....
 }

So, your self-defined class Foo should have a constructor to take itself instance as a parameter and return a clone object. If the caller change the returned Foo object, it wouldn't affect the internals of Foo class.

Rule: Don't Return References to Mutable Objects. Note that arrays are mutable too.


Bug Report