RFR 9: JEP 290: Filter Incoming Serialization Data

[Roger Riggs]

Hi Chris,

yes, its in the webrev, but I neglected to include it in the javadoc and 
specdiff updates.

Thanks, Roger


On 7/26/2016 12:20 PM, Chris Hegarty wrote:
> 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
>>>>