as securecoding site explains:
Blockquote This noncompliant code example shows a class Ser with a private constructor, indicating that code external to the class should be unable to create instances of it. The class implements java.io.Serializable and defines public readObject() and writeObject() methods. Consequently, untrusted code can obtain the reconstituted objects by using readObject() and can write to the stream by using writeObject().
public class Ser implements Serializable {
private final long serialVersionUID = 123456789;
private Ser() {
// initialize
}
public static void writeObject(final ObjectOutputStream stream)
throws IOException {
stream.defaultWriteObject();
}
public static void readObject(final ObjectInputStream stream)
throws IOException, ClassNotFoundException {
stream.defaultReadObject();
}
}
as you know writeObject and readObject methods should be defined as private (and also without static keyword!) and these methods not invoked by JVM.
my question is: why these methods are unsafe. these methods not even invoke by JVM!! i want a sample code that shows me this code can be unsafe and an attacker can access our data.
any help will be appreciated.
The code shown here certainly has a bug in it, but it's not really a security vulnerability, it's just a programming error. And unfortunately the secure coding site is incorrect in some statements it makes about how to fix it. See the comments below that article; they discuss some of the errors.
First, as you noted, since these methods are declared static they won't be called at all by the serialization mechanism. (Serialization is mostly library-based, not JVM-based.) The bug is that if you wanted to customized the serialization format using these methods, it simply won't work. The default serialization format will be used no matter what you put in these methods.
(It's also somewhat odd that the custom readObject and writeObject methods do nothing but call the default read/write routines, providing no customization at all, but this was probably done for the sake of example.)
Is this code unsafe? No, not really. It just won't work the way it was intended.
User Powerlord asked whether an attacker couldn't just call Ser.readObject(myInputStream). That won't work, because the defaultReadObject method is only allowed to be called during the deserialization of an object. If it's not, it will throw NotActiveException.
The fix for this code is to change the methods so that they are private instance methods instead of public static methods. This will cause the serialization mechanism to call these methods so that they can implement some format customizations.
However, the referenced article is incorrect when it states that this will prevent attackers from creating unwanted instances. It is correct that the private constructor will be bypassed by deserialization. But making readObject and writeObject private will not prevent attackers from deserializing as many instances of this class as they want. This is probably what Powerlord was concerned about. I could easily concoct my own stream of bytes and deserialize objects from it, as many times as I want, with any contents that I choose:
ObjectInputStream ois = new ObjectInputStream(myInputStream);
Ser anotherSerInstance = (Ser)ois.readObject();
To prevent this, the Ser class would have to implement some checking in readObject and throw something like InvalidObjectException if it wanted to prevent deserialization. Another approach would be for it to implement readResolve and return an already-created instance (such as a singleton) instead of creating a new one.
For further discussion, see Bloch, Effective Java, items 74-78. Note that it recommends using an enum instead of readResolve if you want a singleton.