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

[Daniel Fuchs]

On Wed, 26 May 2021 22:11:54 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision:
> 
>  - Merge branch 'master' into 8264859-context-filter-factory
>  - Added test for rejectUndecidedClass array cases
>    Added test for preventing disabling filter factory
>    Test cleanup
>  - Editorial updates to review comments.
>    Simplify the builtin filter factory implementation.
>    Add atomic update to setting the filter factory.
>    Clarify the description of OIS.setObjectInputFilter.
>    Cleanup of the example code.
>  - Editorial updates
>    Updated java.security properties to include jdk.serialFilterFactory
>    Added test cases to SerialFilterFactoryTest for java.security properties and
>    enabling of the SecurityManager with existing policy permission files
>    Corrected a test that OIS.setObjectInputFilter could not be called twice.
>    Removed a Factory test that was not intended to be committed
>  - Moved utility filter methods to be static on ObjectInputFilter
>    Rearranged the class javadoc of OIF to describe the parts of
>    deserialization filtering, filters, composite filters, and the filter factory.
>    And other review comment updates...
>  - Refactored tests for utility functions to SerialFilterFunctionTest.java
>    Deleted confused Config.allowMaxLimits() method
>    Updated example to match move of methods to Config
>    Added test of restriction on setting the filterfactory after a OIS has been created
>    Additional Editorial updates
>  - Move merge and rejectUndecidedClass methods to OIF.Config
>    As default methods on OIF, their implementations were not concrete and not trustable
>  - Review suggestions included;
>    Added @implSpec for default methods in OIF;
>    Added restriction that the filter factory cannot be set after an ObjectInputStream has been created and applied the current filter factory
>  - Editorial javadoc updated based on review comments.
>    Clarified behavior of rejectUndecidedClass method.
>    Example test added to check status returned from file.
>  - Editorial updates to review comments
>    Add filter tracing support
>  - ... and 3 more: https://git.openjdk.java.net/jdk/compare/cad27daf...0930f0f8

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

> 168:  * <p>For an application composed from multiple modules or libraries, the structure
> 169:  * of the application can be used to identify the classes to be allowed or rejected
> 170:  * by each {@linkplain ObjectInputStream} in each context of the application.

Nit: should be `{@link }` here.

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

> 350:      *
> 351:      * @param predicate a predicate to test a non-null Class, non-null
> 352:      * @param otherStatus a Status to use if the predicate is {@code false}

should it be specified that the `otherStatus` must also be non-null?
Is there a blanket statement somewhere for NPE, or are `@throws NPE` clauses missing everywhere?
I'm asking because elsewhere in the JDK we usually specify that "unless otherwise specified, null parameters are not allowed and a NullPointerException will be thrown". But here it seems the opposite direction has been taken (which is fine), but the fact that NPE will be thrown if `null` is passed for a parameter specified as non-null seems to be implicit.

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

> 383:      * @since 17
> 384:      */
> 385:     static ObjectInputFilter merge(ObjectInputFilter filter, ObjectInputFilter anotherFilter) {

Same remark about NPE. Should it `@throws` or is there a blanket statement that makes it unnecessary?

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

> 394:      * are {@code REJECTED}.  Either the class is not {@code ALLOWED} or
> 395:      * if the class is an array and the base component type is not allowed,
> 396:      * otherwise the result is {@code UNDECIDED}.

Is there some part of the sentence missing here? I don't fully understand the "Either, or, otherwise" construct.

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

> 636:             if (filterString != null) {
> 637:                 configLog.log(INFO,
> 638:                         "Creating deserialization filter from {0}", filterString);

Just double checking that you really want an `INFO` message here. With the default logging configuration, `INFO` messages will show up on the console.

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

> 651:             if (factoryClassName != null) {
> 652:                 configLog.log(INFO,
> 653:                         "Creating deserialization filter factory for {0}", factoryClassName);

Same remark about using `INFO` level here.

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

> 717:          * @throws SecurityException if there is security manager and the
> 718:          *       {@code SerializablePermission("serialFilter")} is not granted
> 719:          * @throws IllegalStateException if the filter has already been set {@code non-null}

`* @throws IllegalStateException if the filter has already been set {@code non-null}`

Is there a typo/word missing ?

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

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