RFR 9: JEP 290: Filter Incoming Serialization Data

[Chris Hegarty]

Another final thought that just occurred to me…

java.io.SerializablePermission will need its class-level javadoc updated to
include the new permission target name.

-Chris.

> On 25 Jul 2016, at 19:55, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> 
> 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 
>>> 
>