On Thu, 20 May 2021 16:10:11 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 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
