RFR 9: JEP 290: Filter Incoming Serialization Data

[Roger Riggs]

Hi Chris,

Thanks for the review and comments,

Updates in place:

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-serial-filter-jdk9-8155760/

SpecDiff:
http://cr.openjdk.java.net/~rriggs/filter-diffs/overview-summary.html

Javadoc (subset)
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputStream.html 


http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputFilter.html 



On 7/25/2016 10:54 AM, Chris Hegarty wrote:
> Roger,
>
> Mainly looks good. Some comments on the spec:
>
>  - Use the Present Simple Tense consistently, e.g.
>     "Return*S* an ObjectInputFilter computed from a string of patterns."
ok
>
>  - ObjectInputFilter. Was there a comment already on the use of links?
>     For example the following is showing in the javadoc:
> "ObjectInputStream.setObjectInputFilter(java.io.ObjectInputFilter)"
>      when "setObjectInputFilter" would be better.
ok, will fix  (It would be nice if javadoc had a @linksimple that didn't 
supply the whole signature).
>
>     Same comment applies to the links to ObjectInputFilter.Status, and
>     other places.
>
>  - ObjectInputFilter class description. "If set on an
>     ObjectInputStream, the method*(s)* are called ..."
ok
>
>  - Looking at the example in the ObjectInputFilter class description
>     makes me think that maybe the default process-wide filter should
>     be a filter that simply returns UNDECIDED, rather than being null.
>     Is it important to discern whether, or not, it has been set?
When writing a customized filter, it is useful to know whether the 
process-wide filter is has been configured.
Usually it is not and there will be a (slight) performance improvement 
in not calling it and checking the return.

>
>  - ObjectInputFilter.Config
>     The initial sentence in the class description should describe the
>     class itself, so maybe " A utility class for ..."
ok
>
>  - "process-wide" is this an agreed upon term? I'm just curious where
>     it came from. Is there a more common term for this?
It is useful to distinguish between the filter applied by default to all 
ObjectInputStreams from
one set for a particular stream.  I initially used 'global' but that 
seemed overly broad.
The description is used sparingly but I'm open to suggestions.
>
>  - Config.setSerialFilter:  SecurityException - if there is security
>     manager and the SerializablePermission("serialFilter") is not
>     granted or if there is no securityManager set and the process-wide
>     filter has already been set non-null
>
>     It is a little odd to throw a SE if there is no SM, no ?
True, that would be better as IllegalStateException;   updated
>
>  - Is there a class/package level statement covering null, or should
>     it be covered for each applicable method?
Added one.
>
>  - ObjectInputStream
>     "... the serialization filter for the stream." ->
>     "... the serialization filter for THIS stream."
ok
>
>  - setObjectInputFilter: "The checkInput method is called for each
>     class and reference in the stream". Does this apply to back
>     references too?
yes, a reference in the stream, as opposed to a new instance in the stream,
refers to back references.
>
>  - setObjectInputFilter: "... when the ObjectInputStream is constructed
>     and CANNOT be re-set until an object has been deserialized."
The intent was to prevent it from being modified during deserialization.
But I think it will be clearer if it can only be set non-null once and 
only if the previous
value was the pre-configured process-wide filter.

I've also had the recommendation that ObjectInputStream.setObjectInputFilter
should be protected by the same permission as configuring the 
process-wide filter.
(SerializablePermission("serialFilter"))

Roger



>
> -Chris.
>
> On 19/07/16 15:02, Roger Riggs wrote:
>> Please review the design, implementation, and tests of JEP 290: Filter
>> Incoming Serialization Data[1]
>>
>> It allows incoming streams of object-serialization data to be filtered
>> in order to improve both security and robustness.
>> The JEP[1] has more detail on the background and scope.
>>
>> The core mechanism is a filter interface implemented by serialization
>> clients and set on an |ObjectInputStream|. The filter is called during
>> the deserialization process to validate the classes being deserialized,
>> the sizes of arrays being created, and metrics describing stream length,
>> stream depth, and number of references as the stream is being decoded.
>>
>> A process-wide filter can be configured that is applied to every
>> ObjectInputStream.
>> The API of ObjectInputStream can be used to set a custom filter to
>> supersede or augment the process-wide filter.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-serial-filter-jdk9-8155760/
>>
>> SpecDiff:
>> http://cr.openjdk.java.net/~rriggs/filter-diffs/overview-summary.html
>>
>> Javadoc (subset)
>> http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputStream.html 
>>
>>
>> http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputFilter.html 
>>
>>
>>
>> Comments appreciated, Roger
>>
>> [1] JEP 290:   https://bugs.openjdk.java.net/browse/JDK-8154961
>>