Skip to end of metadata
Go to start of metadata

Serializable class can overload the readObject() method, which is called when an object of that class is being deserialized.  Both this method and the readResolve() method should refrain from performing potentially dangerous operations.  

 A class that performs dangerous operations in the constructor must not be Serializable. This is because SER07-J. Do not use the default serialized form for classes with implementation-defined invariants would require that its readObject() method perform the same dangerous operation as the constructor. As an alternative, such a class  could be Serializable if readObject() always throws an exception.

This guideline is related to rule SER12-J. Prevent deserialization of untrusted data.

Non-Compliant Code Example

In the following non-compliant code example, the class OpenedFile opens a file during deserialization.  Operating systems typically impose a limit on the number of open file handles per process. Usually, this limit is not large (e.g., 1024).  Consequently, deserializing a list of OpenedFile objects can consume all file handles available to the process and consequently cause the program to malfunction if it attempts to open another file before the deserialized OpenedFile objects get garbage-collected.

import java.io.*;

class OpenedFile implements Serializable {
  String filename;
  BufferedReader reader;

  public OpenedFile(String filename) throws FileNotFoundException {
    this.filename = filename;
    init();
  }

  private void init() throws FileNotFoundException {
    reader = new BufferedReader(new FileReader(filename));
  }
    
  private void writeObject(ObjectOutputStream out) throws IOException {
    out.writeUTF(filename);
  }

  private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
    filename = in.readUTF();
    init();
  }
} 

Compliant Solution

In this compliant solution, potentially dangerous operations are moved outside of deserialization, and users of the class are required to make a separate call to init() after deserializing.

import java.io.*;
 
class OpenedFile implements Serializable {
  String filename;
  BufferedReader reader;
  boolean isInitialized;

  public OpenedFile(String filename) {
    this.filename = filename;
    isInitialized = false;
 }

  public void init() throws FileNotFoundException {
    reader = new BufferedReader(new FileReader(filename));
    isInitialized = true;
 }
     
  private void writeObject(ObjectOutputStream out) throws IOException {
    out.writeUTF(filename);
  }

  private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
    filename = in.readUTF();
    isInitialized = false;
 }
}

Compliant Solution

In this compliant solution, we assume that OpenedFile must be Serializable because it inherits from a serializable superclass. Because OpenedFile must perform dangerous operations in the constructor, it intentionally forbids deserialization by throwing an exception in readObject().

 

import java.io.*;
 
class Unchangeable implements Serializable {
  // ...
}
class OpenedFile extends Unchangeable { // Serializable, unfortunately
  String filename;
  BufferedReader reader;
  boolean isInitialized;

  public OpenedFile(String filename) {
    this.filename = filename;
    isInitialized = false;
 }

  public void init() throws FileNotFoundException {
    reader = new BufferedReader(new FileReader(filename));
    isInitialized = true;
 }
     
  private void writeObject(ObjectOutputStream out) throws IOException {
    out.writeUTF(filename);
  }

  private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
    throw new NotSerializableException(OpenedFile.getClass().getName());
 }
}

Related Vulnerabilities

CERT Vulnerability #576313 describes a family of exploitable vulnerabilities that arise from violating this rule.

Risk Assessment

The severity of violations of this rule depend on the nature of the potentially dangerous operations performed.  If only mildly dangerous operations are performed, the risk might be limited to denial-of-service (DoS) attacks.  At the other extreme, remote code execution is possible if attacker-supplied input is supplied to methods such as Runtime.exec (either directly or via reflection).

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

SEC58-J

High

LikelyHighP9L2

Automated Detection

Tool
Version
Checker
Description

ysoserial

  Useful for developing exploits that detect violation of this rule

Related Guidelines

MITRE CWE

CWE-502, Deserialization of Untrusted Data

Bibliography

  

 

3 Comments

  1. Good first draft for a rule. Comments:

    • The 1st NCCE is similar to CVE-2011-2894, should at least cite the CVE. IIRC the vul in the CVE also allowed attackers to create arbitrary files; we should list that as a potential hazard. (To do that you have to open the files for writing, not reading).
    • The 1st NCCE needs to comply with ERR00-J. Do not suppress or ignore checked exceptions
    • The CS fixes the problem by whitelisting the classes deserialized, which is recommended by SEC58-J. Deserialization methods should not perform potentially dangerous operations. Should be replaced by a CS that fixes the readObject() definition. Perhaps by ensuring validity of the deserialized objects before opening files. Perhaps by catching and handling a 'no more file descriptors' exception.
    • Need a NCCE/CS pair that showcases the Apache Commons Collection library vulnerability. aka CVE-2015-3253.
  2. I just did some minor grammar-smithing.  Beyond that, I concur with David's comments for additional NCCE/CS work.

  3. Updated comments:

    • 1st CS should just check that there is a whitelist and explain why, as we discussed.
    • Add CS that opens the file after deserialization, using an 'initialized' flag, as we discussed.

    This comment still applies:

    • Need a NCCE/CS pair that showcases the Apache Commons Collection library vulnerability. aka CVE-2015-3253.

    Both this method and the method readResolve() must treat the serialized data as potentially malicious and must refrain from performing potentially dangerous operations, unless the programmer has expressly indicated for the particular deserialization at hand that the class should be permitted to perform potentially dangerous operations.

    • I don't like that last clause. If your deserializer can perform potentially dangerous operations, it could be paired with a class that doesn't whitelist its serialized classes, violating SER12-J. Then an attacker can use your class for evil.
    • A large part of the problem is the InputDataStream.defaultReadObject() method, which can deserialize a huge graph of objects. This can cause no end of mischief (imagine an array of 1024 OpenedFile objects). The NCCE doesn't use defaultReadObject(), so it doesn't showcase that liability. Maybe I'm greedy, but I would like to see a NCCE that uses defaultReadObject(), along with a CS that is more judicious (perhaps dispensing with it or validaing ahead of time).
    • Needs some simple text in the Risk Assessment (trusting deserialized data can result in executing code, such as shell commands, at the behest of attackers.