Explicit Serialization API and Security

[Brian Goetz]

Thanks Peter(s): I think it's just about time to re-sync on the goals, 
since there are an infinitude of issues with serialization, only some of 
which we can address.

To that end, let me toss out some of the results of our recent 
explorations with improving serialization, where we started out on 
something not unlike what Peter is suggesting, and dialed back.

One thing we can do to positively improve the serialization experience 
is to reduce the set of situations where users have to write explicit 
readObject() methods.  In the JDK, we end up having to do so for all 
manner of security-motivated reasons, and such code is hard to write 
correctly, hard to validate, hard to audit, and hard to evolve.

After an audit of our codebase (thanks Chris!), we identified some of 
the biggest offenders on this score:

  - Validating invariants
  - Ensuring confinement

The latter comes up in cases where there's a field:

class Foo {
     private final Bar bar = new BarImpl();
}

which implicitly has an invariant: the Bar referenced by Foo.bar is a) 
exactly of type BarImpl and not some other subclass and b) there is 
exactly one reference to that BarImpl (assuming it doesn't escape Foo.)

Note that both of these still use the default deserialization; having to 
override readObject() just to do some extra checking is error-prone and 
messy.  So I think the goal of a separate validation method which is 
called automatically is a good one, since it means we can ditch some 
percentage of otherwise-needless readObject implementations (which then 
have to jump through hoops to set final fields.)

I agree that being able to play nicely with final fields is also an 
important goal here.

On 1/5/2015 9:11 AM, Peter Levart wrote:
> Hi Peter and others,
>
> A assume your approach described here (to use constructors for
> deserialization) is motivated mainly by the fact that only constructors
> are allowed to set the final fields. Otherwise the explicit features of
> your ReadSerial API are more or less accessible even now by using
> "serialPersistentFields " static field for declaration of "serialized"
> fields, combined with ObjectOutputStream.PutField and
> ObjectInputStream.GetField API, accessible from writeObject/readObject
> methods. readObject() method already acts like a constructor, whith the
> following notable differences:
>
> 1 - it is not chained explicitly to superclass (either in source or
> automatically by javac) like constructor, but is invoked by
> (de)serialization infrastructure in correct order for each class in the
> hierarchy that declares it.
> 2 - it is a normal method, so it can be called at any time by code in
> the declaring class or inner classes.
> 3 - it can't set final fields (without using clumsy reflection)
>
>
> The 1st point is actually in favor of readObject() method, since the
> (de)serialization API need not be caller-sensitive. The correct
> per-class context is established before each readObject call-back.
> 2nd and 3rd points are readObject drawbacks and are both interconnected.
> If readObject was not universally callable, then the restriction that it
> can't set final fields could be lifted.
>
> Perhaps it is too late from compatibility perspective to change the
> readObject's 2nd and 3rd point now, but we could compatibly change the
> 1st point of a special kind of constructor.
>
> Say that a constructor declared with exactly one parameter of type
> ObjectInputStream is treated specially by javac and reflection:
> - constructor code can't chain to superclass constructor
> - javac does not automatically generate default constructor chaining code
> - javac does not allow calling such constructor from any code
> - this is optional: javac does not allow calling instance methods and
> explicit use of 'this' keyword in deserialization constructor code
> (preventing escape of such object before de-serialization ends) - to be
> really safe, this would probably have to be accompanied with verifier
> checks too.
> - public reflection API does not allow invoking such constructor (only
> deserialization infrastructure can - like it can invoke the default
> constructor of a non-Serializable superclass on an already allocated
> instance of a sublclass)
>
> No additional changes to VM are apparently necessary (except verifier).
> Declaring such constructor as an alternative to readObject() method is
> then possible to make deserialization more safe.
>
> Regarding finalizability (and finalizer attacks):
>
> The serialization specifies that for 1st (most specific)
> non-Serializable superclass of a Serializable subclass, default
> constructor is invoked to initialize the non-Serializable superclass
> state before deserializing subclass state. As each Serializable class
> has such a non-Serializable superclass (at least in the form of Object
> class) and normal constructors are chained, it is apparent that Object's
> default constructor is called before any deserialization of state
> begins. As soon as Object's constructor completes, the instance becomes
> eligible for finalization:
>
>   "An object o is not finalizable until its constructor has invoked the
> constructor for Object on o and that invocation has completed
> successfully (that is, without throwing an exception).”
>
> The rule of calling non-Serializable superclass default constructor
> could be weakened. If 1st (most specific) non-Serializable superclass of
> a Serializable subclass is Object, then it's constructor is not called.
> Obviously it is not needed since Object has no state to initialize. What
> we achieve by this is that such object does not become eligible for
> finalization just yet. Serialization infrastructure must then explicitly
> register such objects for finalization, but only after their
> de-serialization completes normally. This would prevent finalization
> attacks.
>
> Regards, Peter
>
>
> On 01/05/2015 01:01 PM, Peter Firmstone wrote:
>> My mistake, thank you.
>>
>> Peter.
>>
>> On 5/01/2015 9:57 PM, David Holmes wrote:
>>> Hi Peter,
>>>
>>> Did you send this to the list? I haven't seen it turn up yet.
>>>
>>> David
>>>
>>> On 5/01/2015 6:51 PM, Peter Firmstone wrote:
>>>> Thanks Dave,
>>>>
>>>> That's another way of checking invariants, you could expose A's check
>>>> method but you'd also need a couple of additional static methods to
>>>> obtain integers upper and lower from ReadSerial, so that B can ensure
>>>> its invariant.
>>>>
>>>> ReadSerial is caller sensitive so B can't obtain A's stream values via
>>>> ReadSerial and must do so via A's API.  This prevents the
>>>> publication of
>>>> A's implementation, you don't wan't B depending on A's serial form.
>>>> Instead A can reorder and rename it's fields and change their values,
>>>> have multiple serial forms and be able to remain compatible with all
>>>> forms relatively easily.
>>>>
>>>> There are some benefits to using a temporary object instance of A; it
>>>> reduces the amount of code required, eg it would be a beneficial for
>>>> RMI
>>>> to minimise code downloads, the JVM is free to inline access to these
>>>> fields and the temporary instance of A never leaves the cpu cache, so
>>>> it's not likely to become a performance issue.  Well it might on real
>>>> time java :)
>>>>
>>>> If circular relationships are mutable, or effectively mutable after
>>>> publication, then we could eventually deprecate the requirement to
>>>> support special treatment of final fields for Serializable.
>>>>
>>>> Cheers,
>>>>
>>>> Peter.
>>>>
>>>> On 5/01/2015 10:38 AM, David Holmes wrote:
>>>>>
>>>>>>>       - I don't see how this invariant-checking mechanism can
>>>>>>> enforce
>>>>>>> invariants between superclass fields and subclass fields.   For
>>>>>>> example:
>>>>>>>
>>>>>>> class A {
>>>>>>>             int lower, upper;   // invariant: lower <= upper
>>>>>>> }
>>>>>>>
>>>>>>> class B extends A {
>>>>>>>             int cur;   // invariant: lower <= cur <= upper
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> The serialization framework should only construct an objects fields
>>>>>> and make these available from ReadSerial, each class then calls a
>>>>>> static method before calling a superclass constructor that's
>>>>>> responsible for an objects creation, to prevent construction of the
>>>>>> object, if necessary.
>>>>>>
>>>>>> Eg, some complexity, but bullet proof:
>>>>>>
>>>>>> public class A (
>>>>>>
>>>>>>      public final int lower, upper;
>>>>>>
>>>>>>       private static boolean check(ReadSerial rs){
>>>>>>          if (rs.getInt("lower") > rs.getInt("upper"))
>>>>>>               throw new IllegalArgumentException(
>>>>>>                  "lower bound must be less than or equal to upper");
>>>>>>         return true;
>>>>>>      }
>>>>>>
>>>>>>      public A(ReadSerial rs){
>>>>>>          this(check(rs), rs);
>>>>>>      }
>>>>>>
>>>>>>      private A(boolean checked, ReadSerial rs){
>>>>>>          super();
>>>>>>          lower = rs.getInt("lower");
>>>>>>          upper = rs.getInt("upper");
>>>>>>      }
>>>>>>
>>>>>> // other constructors omitted must also check invarients
>>>>>> }
>>>>>>
>>>>>> class B extends A {
>>>>>>
>>>>>>      public final int cur;
>>>>>>
>>>>>>      private static ReadSerial check(ReadSerial rs) {
>>>>>>          A a = new A(rs);
>>>>>
>>>>>
>>>>> It doesn't seem practical in general to have to create an instance of
>>>>> your supertype to validate the passed in serialized form. Why not
>>>>> expose the check method?
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>          int cur = rs.getInt("cur");
>>>>>>          if ( a.lower > cur || cur > a.upper )
>>>>>>               throw new IllegalArgumentException(
>>>>>>                   "cur outside lower and upper bounds");
>>>>>>          return rs;
>>>>>>      }
>>>>>>
>>>>>>      public B(ReadSerial rs) {
>>>>>>          super(check(rs));
>>>>>>          cur = rs.getInt("cur");
>>>>>>      }
>>>>>> }
>>>>>>
>>>>>>
>>>>
>>
>