Every serializable class that has private mutable instance variables must defensively copy them in the readObject() method. An attacker can tamper with the serialized form of such a class, appending extra references to the byte stream. When deserialized, this byte stream could allow the creation of a class instance whose internal variable references are controlled by the attacker. Consequently, the class instance can mutate and violate its class invariants.

This rule is an instance of OBJ06-J. Defensively copy mutable inputs and mutable internal components, which applies to constructors and to other methods that accept untrusted mutable arguments. This rule applies the same principle to deserialized mutable fields.

Noncompliant Code Example

This noncompliant code example fails to defensively copy the mutable Date object date. An attacker might be able to create an instance of MutableSer whose date object contains a nefarious subclass of Date and whose methods can perform actions specified by an attacker. Any code that depends on the immutability of the subobject is vulnerable.

class MutableSer implements Serializable {
  private static final Date epoch = new Date(0);
  private Date date = null; // Mutable component
  
  public MutableSer(Date d){
    date = new Date(d.getTime()); // Constructor performs defensive copying
  }

  private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
    ois.defaultReadObject();
    // Perform validation if necessary
  }
}

Compliant Solution

This compliant solution creates a defensive copy of the mutable Date object date in the readObject() method. Note the use of field-by-field input and validation of incoming fields. Additionally, note that this compliant solution is insufficient to protect sensitive data (see SER03-J. Do not serialize unencrypted sensitive data for additional information).

private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
  ObjectInputStream.GetField fields = ois.readFields();
  Date inDate = (Date) fields.get("date", epoch);
  // Defensively copy the mutable component
  date = new Date(inDate.getTime());
  // Perform validation if necessary
}

There is no need to copy immutable subobjects. Also, avoid using the subobject's clone() method because it can be overridden when the subobject's class is not final and produces only a shallow copy. The references to the subobjects themselves must be nonfinal so that defensive copying can occur. It is also inadvisable to use the writeUnshared() and readUnshared() methods as an alternative [Bloch 2008].

Risk Assessment

Failure to defensively copy mutable components during deserialization can violate the immutability contract of an object.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

SER06-J

Low

Probable

Medium

P4

L3

Automated Detection

Tool
Version
Checker
Description
CodeSonar
8.1p0

JAVA.CLASS.SER.ND

Serialization Not Disabled (Java)

Coverity7.5UNSAFE_DESERIALIZATIONImplemented

Related Guidelines

MITRE CWE

CWE-502, Deserialization of Untrusted Data

Bibliography

[API 2014]


[Bloch 2008]

Item 76, "Write readObject Methods Defensively"

[Sun 2006]

Serialization Specification, A.6, Guarding Unshared Deserialized Objects



2 Comments

  1. Mr. Bloch is usually a pretty safe bet for good examples, however both his Effective Java example and the CS are vulnerable to an attack similar I commented in SER04-J.

    One could craft serialized data where the object stored in the date field of the MutableSer class is a Date subclass, MutableDate, which holds a reference to the MutableSer instance (so MutableSer references MutableDate which references MutableSer). When the readObject method calls getTime() on the MutableDate class, it has a fully functional instance of the MutableSer class, with itself (a mutable date) in the date field.

    As an example of how to create such serialized data and the auxiliary MutableDate:

    public class CreateEvilSerializedData {
      public static void main(String[] args) throws Exception {
        MutableSer ser = new MutableSer(new Date());
    		
        MutableDate mutable = new MutableDate();
        mutable.captured = ser;
        // reflection to set a desired value
        Field field = MutableSer.class.getDeclaredField("date");
        field.setAccessible(true);
        field.set(ser, mutable);
    		
        FileOutputStream fos = new FileOutputStream("ser07.ser");
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(ser);
        oos.close();
      }
    
      static class MutableDate extends Date {
        private static final long serialVersionUID = 1L;
        Object captured;
    
        public long getTime() {
          // at this point we have a MutableSer object that's initialized and has 
          // a MutableDate as the date
          System.out.println(captured);
          super.setTime(0);
          System.out.println(captured);
          return super.getTime();
        }
      }
    }
    

    I propose the same fix as I did for SER04-J.

    1. Improved as suggested. Also added the observation that even this improvement is insufficient to protect sensitive data.