[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-core-libs-dev
Subject:    Re: RFR: 8277322: Document that setting an invalid property jdk.serialFilter disables deserializatio
From:       Roger Riggs <rriggs () openjdk ! java ! net>
Date:       2021-11-30 20:46:07
Message-ID: 8XD8crGyZQwUraBd6zeaOzDq-HwXf1K0pn04dgrZEzI=.0aca26c8-8c05-4f4a-93aa-8fb70e93f1cf () github ! com
[Download RAW message or body]

On Wed, 24 Nov 2021 15:39:13 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

> > If the intent is to disable serialization entirely, then this state should be \
> > represented explicitly. Having things throw `NoClassDefFoundError` looks like a \
> > mistake and a bug that needs to be fixed. In addition, it requires that all code \
> > paths to deserialization use `OIF.Config` in order to provoke the NCDFE. This \
> > might be true today, but under maintenance a different code path might be \
> > introduced that happens not to use that class, allowing deserialization to \
> > proceed. 
> > An alternative policy might be to disallow any deserialization that would use the \
> > default filter. This could be accomplished by having a "fail-safe" or "fallback" \
> > filter that always returns REJECT. This would be at least as restrictive and thus \
> > no less safe than any valid filter that could be installed. In addition, it would \
> > allow things that don't use the global filter to proceed, such as, 
> > var ois = new ObjectInputStream(...);
> > ois.setObjectInputFilter(new ObjectInputFilter() { ... });
> > 
> > or
> > 
> > var ois = new ObjectInputStream(...);
> > ois.setObjectInputFilter(ObjectInputFilter.Config.createFilter(...));
> > 
> > There could be a reasonable policy discussion about whether it's preferable to \
> > disable all deserialization or just deserialization that depends on the \
> > (invalidly specified) global filter.
> 
> This is about the second line of defense; what happens when the developer \
> deliberately ignores the first error. If the command line parameters are invalid it \
> might be an option to call `System.exit(1)` but there is no precedent for that and \
> it seems undesirable. 
> Any and all deserialization is only possible after the command line or security \
> properties, if any, are successfully applied. In the examples above, the \
> constructors for `ObjectInputStream` do not succeed if either the serial filter or \
> filter factory are not valid.  The builtin filter factory applies the default \
> filter regardless of the setting of an `ObjectInputFilter` set on the stream.  The \
> only way to completely control the filter combinations is to provide a valid filter \
> factory on the command line; but that is not the case raising the issue here. 
> The initialization of both could be re-specified and re-implemented to allow the \
> initialization of `Config` to complete successfully but defer throwing an exception \
> (or Error) until either filter or filter factory was requested from \
> `Config.getSerialFilter` or `Config.getSerialFilterFactory`. Both of them are \
> called from the `ObjectInputStream` constructors.  At present, it is incompletely \
> specified and implemented to throw `IllegalStateException` for \
> `getSerialFilterFactory`. 
> The `NCDFE` is a more reliable backstop against misuse from any source, including \
> reflection, than the alternative.

The use of `ExceptionInInitializerError` can be completely replaced for  invalid \
values of `jdk.serialFilter` and `jdk.serialFilterFactory` with:

- If either property is supplied and is invalid; deserialization is disabled by:
- `OIF.Config.getSerialFilter()` and `OIF.Config.setSerialFilter(...)` throw \
IllegalStateException with the exception message thrown from \
                `Config.createFilter(pattern)`
- `OIF.Config.getSerialFilterFactory()` and `OIF.Config.setSerialFilterFactory(...)` \
throw IllegalStateException with the exception message from the attempt to construct \
                the filter factory.
- Both `new ObjectInputStream(...)` constructors call both \
`OIF.Config.getSerialFilter()` and `OIF.Config.getSerialFilterFactory()` and so will \
                throw the appropriate `IllegalStateException` for invalid values of \
                the properties.
- The static initialization of `OIF.Config` does not throw any exceptions (so no \
`ExceptionInInitializerError`) but hold the state so that the method above can throw \
                `IllegalStateException` with the informative message.
- The `IllegalStateException`s will be thrown just slightly later than previously, \
                now after the `Config` class is initialized instead of during \
                initialization.
- The javadoc of the `Config` class will replace the descriptions of the previous \
error with descriptions of `ISE` and each of the methods mentioned above will have an \
added `IllegalStateException` documented referring to the property values.

Its not strictly compatible with the previous behavior but occurs only in the case of \
badly formed parameters on the command line.

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

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


[prev in list] [next in list] [prev in thread] [next in thread]