On Wed, 26 May 2021 22:11:54 GMT, Roger Riggs <rri...@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
