RFR: 8264859: Implement Context-Specific Deserialization Filters [v4]

Sat May 22 00:43:13 UTC 2021

On Fri, 21 May 2021 17:47:53 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> JEP 415: Context-specific Deserialization Filters extends the deserialization filtering mechanisms with more flexible and customizable protections against malicious deserialization.  See JEP 415: https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and `ObjectInputStream`:
>>     http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Editorial updates to review comments
>   Add filter tracing support

Changes requested by bchristi (Reviewer).

src/java.base/share/classes/java/io/ObjectInputFilter.java line 84:

> 82:  *
> 83:  * <p> When a filter is set on an {@link ObjectInputStream}, the {@link #checkInput checkInput(FilterInfo)}
> 84:  * method is called to validate classes, the length of each array,

This sounds a bit like checkInput() is called at the time that the filter is set.  I'd recommend something like, "If a filter is set on an ObjectInputStream, the checkInput() method is called during deserialization to validate..."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 123:

> 121:  * the depth, number of references, and stream size and return a status
> 122:  * that reflects only one or only some of the values.
> 123:  * This allows a filter to specific about the choice it is reporting and

...to _**be**_ specific ?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 386:

> 384:      * {@link ObjectInputStream#setObjectInputFilter(ObjectInputFilter) ObjectInputStream.setObjectInputFilter}.
> 385:      * If {@code ObjectInputStream.setObjectInputFilter} is called, the filter factory is called a second time
> 386:      * with the initial filter returned from the first call and the requested new filter.

Maybe shorten to, "with the stream's initial filter, and the requested new filter."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 404:

> 402:      * The JVM-wide filter is configured during the initialization of the
> 403:      * {@code ObjectInputFilter.Config} class.
> 404:      * For example, by calling {@link #getSerialFilter() Config.getSerialFilter}.

Can the "For example..." sentence be removed?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 423:

> 421:      * The class must be public, must have a public zero-argument constructor, implement the
> 422:      * {@link BinaryOperator {@literal BinaryOperator<ObjectInputFilter>}} interface, provide its implementation and
> 423:      * be accessible via the {@linkplain ClassLoader#getSystemClassLoader() the application class loader}.

two "the"s

src/java.base/share/classes/java/io/ObjectInputFilter.java line 1258:

> 1256:             /**
> 1257:              * Apply the filter and return the status if not UNDECIDED and checking a class.
> 1258:              * Make an exception for Primitive classes that are implicitly allowed by the pattern based filte.

filte -> filter

src/java.base/share/classes/java/io/ObjectInputStream.java line 200:

> 198:  * and every object read from the stream can be checked.
> 199:  * The {@linkplain #ObjectInputStream() ObjectInputStream constructors} invoke the filter factory
> 200:  * to select the initial filter and it is updated by {@link #setObjectInputFilter}.

In ObjectInputFilter, the `"The deserialization filter for a stream is determined in one of the following ways"` section has a good description of the various pieces and how they work together to determine a stream's filter.  I would consider leaning on that to provide the details, and have a shorter, high-level description here.  For instance:

"Each stream has an optional deserialization filter to check the classes and resource limits during deserialization.  The filter can be set for all streams JVM-wide, or customized on a per-stream basis.  See `<link to "The deserialization for a stream is determined...." section of ObjectInputFilter>` for details on how the filter for a stream is determined.
ObjectInputFilter describes how to use filters and..."

src/java.base/share/classes/java/io/ObjectInputStream.java line 370:

> 368:      *
> 369:      * <p>The serialization filter is initialized to the filter returned
> 370:      * by invoking the {@link Config#getSerialFilterFactory()} with {@code null} for the current filter

`linkplain` to "JVM-wide filter factory" ?

src/java.base/share/classes/java/io/ObjectInputStream.java line 371:

> 369:      * <p>The serialization filter is initialized to the filter returned
> 370:      * by invoking the {@link Config#getSerialFilterFactory()} with {@code null} for the current filter
> 371:      * and {@linkplain  Config#getSerialFilter() static JVM-wide filter} for the requested filter.

"and _**the**_ static JVM-wide filter for the..."

src/java.base/share/classes/java/io/ObjectInputStream.java line 408:

> 406:      * <p>The serialization filter is initialized to the filter returned
> 407:      * by invoking the {@link Config#getSerialFilterFactory()} with {@code null} for the current filter
> 408:      * and {@linkplain  Config#getSerialFilter() static JVM-wide filter} for the requested filter.

Same comments as the other constructor.

src/java.base/share/classes/java/io/ObjectInputStream.java line 1256:

> 1254:     /**
> 1255:      * Set the deserialization filter for the stream.
> 1256:      * The filter must be set and only set once before reading any objects from the stream;

"The filter can only be set once, and must be set before..." ?

src/java.base/share/classes/java/io/ObjectInputStream.java line 1260:

> 1258:      *
> 1259:      * <p>The serialization filter is set to the filter returned
> 1260:      * by invoking the {@link Config#getSerialFilterFactory()}

linkplain to "JVM-wide filter factory" ?

src/java.base/share/classes/java/io/ObjectInputStream.java line 1263:

> 1261:      * with the current filter and the {@code filter} parameter.
> 1262:      * If there is a non-null filter for the stream, one was set in the constructor, and the filter factory
> 1263:      * must return a non-null filter, it is not permitted to remove filtering once established.

New sentence for, "it is not permitted..." ?

src/java.base/share/classes/java/io/ObjectInputStream.java line 1327:

> 1325:      *       if the filter factory returns {@code null} when the
> 1326:      *       {@linkplain #getObjectInputFilter() current filter} is non-null, or
> 1327:      *       if the filter has already been.

if the filter has already been **_set_**

-------------

PR: https://git.openjdk.java.net/jdk/pull/3996