On Tue, 23 Nov 2021 04:40:04 GMT, Stuart Marks <smarks@openjdk.org> wrote:

...

The effects of an invalid `jdk.serialFilter` property are not completely documented. If the value of the system property jdk.serialFilter is invalid, deserialization should not be possible and it should be clear in the specification.

Specify an implementation specific exception is thrown in the case where deserialization is invoked after reporting the invalid jdk.serialFilter.

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

...

527: * if the filter string is invalid, an {@link ExceptionInInitializerError} is thrown 528: * and the initialization fails; subsequent attempts to use the configuration or 529: * serialization will fail with an implementation specific exception.

I'm confused about exactly what happens after `ExceptionInInitializerError`.

...

Subsequent attempts to use the configuration or serialization will fail....

Which configuration? I thought OIF.Config is a utility class and thus has no instances. If its class initialization fails, then other code cannot use `Config.setSerialFilter` to set a global filter (which might be desirable, but throws NCDFE instead of `IllegalStateException`) and other code can't use `Config.createFilter` to create individual filters. Is that right? It seems like there ought to be a better arrangement than to have the system come up in some dysfunctional way, where any subsequent reference to `OIF.Config` results in NCDFE.

And surely this affects deserialization, not serialization?

Most configurations of `jdk.serialFilter` and` jdk.serialFilterFactory` will be valid. If they are not valid, the cause must be clear and useful suggestion made to correct the command line or security properties. It must not be possible to do any deserialization if the command line (or security) properties are invalid. Since the states are held in static fields of `ObjectInputFilter.Config`, the initialization is done in a static initializer. Exceptions/errors in the static initializer cause `ExceptionInInitializerError` to be thrown. In most cases, that will be sufficient to report the invalid properties and the program will be terminated. The message on the error should be sufficient correct the filter However, there is a chance that `ExceptionInInitializerError` will be caught and ignored. Ignoring the exception should not allow deserialization. At the moment, this comes for free because when the initialization of `ObjectInputFilter.Config` fails any subsequent reference to the class causes `NoClassDefFoundError`. Neither calls to `ObjectInputFilter.Config` get or set methods will succeed. Deserialization is prevented because `ObjectInputStream` constructors call `Config.getSerialFilterFactorySingleton` which will fail with `NoClassDefFoundError`. The question that has been raised is how much of that should be included in the specification of `ObjectInputFilter.Config`. The CSR proposes that after `ExceptionInInitializerError` is thrown, it is specified that an implementation specific exception/error is thrown when attempting to use `Config`. There is no additional value in being specific about the error that is thrown. ------------- PR: https://git.openjdk.java.net/jdk/pull/6508

On Tue, 23 Nov 2021 23:07:08 GMT, Stuart Marks <smarks@openjdk.org> wrote:

...

Most configurations of `jdk.serialFilter` and` jdk.serialFilterFactory` will be valid. If they are not valid, the cause must be clear and useful suggestion made to correct the command line or security properties.

It must not be possible to do any deserialization if the command line (or security) properties are invalid. Since the states are held in static fields of `ObjectInputFilter.Config`, the initialization is done in a static initializer. Exceptions/errors in the static initializer cause `ExceptionInInitializerError` to be thrown. In most cases, that will be sufficient to report the invalid properties and the program will be terminated. The message on the error should be sufficient correct the filter

However, there is a chance that `ExceptionInInitializerError` will be caught and ignored. Ignoring the exception should not allow deserialization. At the moment, this comes for free because when the initialization of `ObjectInputFilter.Config` fails any subsequent reference to the class causes `NoClassDefFoundError`. Neither calls to `ObjectInputFilter.Config` get or set methods will succeed. Deserialization is prevented because `ObjectInputStream` constructors call `Config.getSerialFilterFactorySingleton` which will fail with `NoClassDefFoundError`.

The question that has been raised is how much of that should be included in the specification of `ObjectInputFilter.Config`. The CSR proposes that after `ExceptionInInitializerError` is thrown, it is specified that an implementation specific exception/error is thrown when attempting to use `Config`. There is no additional value in being specific about the error that is thrown.

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. ------------- PR: https://git.openjdk.java.net/jdk/pull/6508

On Tue, 30 Nov 2021 20:43:23 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

...

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.

With the change in scope of the solution, a new PR has been created: https://github.com/openjdk/jdk/pull/6645 Please review that instead. ------------- PR: https://git.openjdk.java.net/jdk/pull/6508