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

[Daniel Fuchs]

On Thu, 20 May 2021 16:10:11 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:
> 
>   Simplify factory interface to BinaryOperator<ObjectInputFilter> and cleanup the example

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

> 408:      * The class must have a public zero-argument constructor, implement the
> 409:      * {@link BinaryOperator} interface,
> 410:      * and provide its implementation.

I wonder if some more explanation is needed to explain how the filter factory class is located. I mean - I expect it will be looked up using the System ClassLoader - doesn't this need to be specified? Or if it's already specified elsewhere then maybe a link should be provided?

Maybe the spec should also say that the class should be public. And I guess that if it is in another module then it should be in an exported package - or possibly in a package opened to reflection? Or does this only work if the class is in the unnamed module (deployed on the classpath)?

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

> 429:          * Used as a system property and a java.security.Security property.
> 430:          */
> 431:         private final static String SERIAL_FILTER_PROPNAME = "jdk.serialFilter";

should be `private static final` (in canonical order).

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

> 587:         /**
> 588:          * Returns the JVM-wide deserialization filter factory.
> 589:          * If the filter factory has been {@link #setSerialFilterFactory(BinaryOperator) set} it is returned,

Should be `{@linkplain ...}`

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

> 590:          * otherwise, a builtin deserialization filter factory is returned.
> 591:          * The filter factory provides a filter for every ObjectInputStream when invoked from
> 592:          * {@link ObjectInputStream#ObjectInputStream(InputStream) ObjectInputStream constructors}

Should be {@linkplain ...}

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

> 599:          * {@link ObjectInputStream#ObjectInputStream(InputStream) ObjectInputStream constructors}.
> 600:          * When invoked {@link ObjectInputStream#setObjectInputFilter(ObjectInputFilter)
> 601:          * to set the stream-specific filter} the requested filter replaces the static JVM-wide filter,

These three should be {@linkplain ...} too.

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

> 601:          * to set the stream-specific filter} the requested filter replaces the static JVM-wide filter,
> 602:          * unless it has already been set. The stream-specific filter can only be set once,
> 603:          * if it has already been set, an {@link IllegalStateException} is thrown.

I am assuming the `IllegalStateException` exception is thrown by `ObjecInputStream::setObjectInputFilter` - maybe that should be clarified here.

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

> 607:          *
> 608:          * @return the JVM-wide deserialization filter factory; non-null
> 609:          * @since TBD

Need to replace TBD before pushing ;-)

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

> 630:          * The factory determines the filter to be used for {@code ObjectInputStream} streams based
> 631:          * on its inputs, and any other filters, context, or state that is available.
> 632:          * The factory may throw runtime exceptions to signal incorrect use or invalid parameters.

What happens if the factory throws an exception?

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

> 637:          * @throws SecurityException if there is security manager and the
> 638:          *       {@code SerializablePermission("serialFilter")} is not granted
> 639:          * @since TBD

... TBD ...

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

> 754:          * <pre><code>
> 755:          *     ObjectInputFilter f = allowFilter(cl -> cl.getClassLoader() == ClassLoader.getPlatformClassLoader()
> 756:          *                                          || cl.getClassLoader() == null);

The example here seems to be missing the `otherStatus` parameter.

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

> 783:          * <pre><code>
> 784:          *     ObjectInputFilter f = rejectFilter(cl -> cl.getClassLoader() == ClassLoader.ClassLoader.getSystemClassLoader());
> 785:          * </code></pre>

Same here, the example is missing the `otherStatus` parameter.

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

> 828:          *
> 829:          */
> 830:         final static class Global implements ObjectInputFilter {

IIRC the canonical order is `static final` not `final static`.

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

> 1077:                 }
> 1078:                 // There are no classes to check and none of the limits have been exceeded.
> 1079:                 return Status.ALLOWED;

Should this be addressed outside of this JEP and pushed separately?

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

> 1125:             public ObjectInputFilter.Status checkInput(FilterInfo info) {
> 1126:                 return (info.serialClass() != null &&
> 1127:                         predicate.test(info.serialClass())) ? ifTrueStatus : ifFalseStatus;

Maybe the result of info.serialClass() should be stored in a local var to avoid calling the method twice.

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

> 1137:          * and not classes.
> 1138:          */
> 1139:         private static class AllowMaxLimitsFilter implements ObjectInputFilter {

This class is maybe misnamed. If limitCheck == REJECTED it will not allow max limits. Or am I missing something?

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

> 1236:          * {@link #getSerialFilter static serial filter} when invoked from
> 1237:          * {@link ObjectInputStream#ObjectInputStream(InputStream) ObjectInputStream constructors}.
> 1238:          * When invoked from {@link ObjectInputStream#setObjectInputFilter(ObjectInputFilter)

These three links should be {@linkplain ...}

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

> 1268:              *       is not {@code null} and is not the JVM-wide filter
> 1269:              * @throws SecurityException if there is security manager and the
> 1270:              *       {@code SerializablePermission("serialFilter")} is not granted

Where is this thrown? I don't see it in the implementation of `apply` below.

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

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