Explicit Serialization API and Security

[Peter Levart]

Hi Brian,

On 01/05/2015 09:51 PM, Brian Goetz wrote:
> 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.)

readObject() can be used on both ends of the spectrum or anywhere 
in-between. It can be used for explicit deserialization or just for 
validation of default deserialized state. Would separate validation 
method give us anything more or simplify things? readObject() can be 
used just as:

private void readObject(ObjectInputStream in) throws IOException, 
ClassNotFoundException {
     in.defaultReadObject();
     ...
     just validation
     ...
}

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

Explicit deserialization and final fields don't play nicely together 
currently, yes. Sometimes data-racey-publication thread-safety is 
sacrificed just because explicit deserialization would complicate code 
too much by using reflection.

Regards, Peter

>
> 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");
>>>>>>>      }
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>
>>>
>>