Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264859: Implement Context-Specific Deserialization Filters #3996

Closed

Conversation

RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented May 12, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264859: Implement Context-Specific Deserialization Filters

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3996/head:pull/3996
$ git checkout pull/3996

Update a local copy of the PR:
$ git checkout pull/3996
$ git pull https://git.openjdk.java.net/jdk pull/3996/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3996

View PR using the GUI difftool:
$ git pr show -t 3996

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3996.diff

@RogerRiggs
Copy link
Contributor Author

@RogerRiggs RogerRiggs commented May 12, 2021

/csr

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 12, 2021

👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added rfr csr labels May 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@RogerRiggs this pull request will not be integrated until the CSR request JDK-8264860 for issue JDK-8264859 has been approved.

@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@RogerRiggs The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label May 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 12, 2021

src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputStream.java Outdated Show resolved Hide resolved
* used for each {@link ObjectInputStream} that does not set its own filter.
* A utility class to set and get the JVM-wide deserialization filter factory,
* the static JVM-wide filter, or to create a filter from a pattern string.
* If a JVM-wide filter factory or static JVM-wide filter is set, it will determine the filter
Copy link
Member

@ChrisHegarty ChrisHegarty May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concerns me, "A JVM-wide filter factory". I was going to suggest that it should be "The ..", but then realised that there can only ever be one present at a time, but in the lifetime of a JVM there can be two (since getSerialFilterFactory if invoked before setSerialFilterFactory will subsequently return a different JVM-wide factory). Is this intentional? It would great if this could be "The ..", so that setXXX can only be invoked successfully if getXXX has not been. This may seen somewhat insignificant, but the fact that the JVM-wide factory can change make the model harder understand.

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is reasonable to require that the factory be set before any OIS is constructed.
Similar to the restriction that the filter on a stream cannot be changed after the first call to readObject.
So an IllegalStateException added to Config.setSerialFilterFactory.

Copy link
Member

@ChrisHegarty ChrisHegarty May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great. So setSerialFilterFactory cannot be successfully invoked after any of i) getSerialFilterFactory, or ii) an OIS is constructed. I don't yet see this in the code.

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec/code is forthcoming.
ii) is sufficient to prevent ambiguity in which filter is used throughout the Java runtime;
though it requires a bit of package-private plumbing.

i) is too limiting. It should be possible for an application to check whether a filter factory has been provided on the command line (by calling getSerialFilterFactory) and if not setting the factory itself. It may also want to install its own filter factory that delegates to the builtin factory without needed to re-implement the builtin behavior.

Copy link
Member

@ChrisHegarty ChrisHegarty May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i) is too limiting. It should be possible for an application to check whether a filter factory has been provided on the command line (by calling getSerialFilterFactory) and if not setting the factory itself. It may also want to install its own filter factory that delegates to the builtin factory without needed to re-implement the builtin behavior.

How is this supposed to work in practice? getSerialFilterFactory always returns a non-null factory, so how does one know whether or not the returned factory is the built-in factory, a factory set by the command line (or security property) ? (without resorting to implementation assumptions)

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the app does not recognize the filter factory (by its class or module) then try to replace it.
If it succeeds, the app is good to go.
If it throws an exception then your app is not likely to run and should fail.
Newer APIs generally make it possible to detect the state of things without throwing but in this case it would add extra API surface.

Some alternatives, check the module of the filter returned, if it is java.base then replacing it is ok to replace. (yes an implementation assumption).
An alternative would be to add a static method to return the builtin filter; creating it if has not already been created.

Add filter tracing support
@RogerRiggs
Copy link
Contributor Author

@RogerRiggs RogerRiggs commented May 21, 2021

To avoid spamming folks with individual emails, I marked as 'resolved' comments that didn't need a response other than 'ok, will fix'. Feel free to re-open them or re-comment if you don't see the fix.
Thanks for the comments.

src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputStream.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputStream.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputStream.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
Clarified behavior of rejectUndecidedClass method.
Example test added to check status returned from file.
RogerRiggs added 2 commits May 24, 2021
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
As default methods on OIF, their implementations were not concrete and not trustable
RogerRiggs added 2 commits May 25, 2021
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...
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
Copy link
Member

@bchristi-git bchristi-git left a comment

More suggestions for ObjectInputFilter.java. I hope they're helpful.

RogerRiggs added 3 commits May 26, 2021
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.
Added test for preventing disabling filter factory
Test cleanup
public void current(ObjectInputFilter current) {
this.current = current;
}
Copy link
Member

@bchristi-git bchristi-git May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this current() used anywhere ?

public void next(ObjectInputFilter next) {
this. next = next;
}
Copy link
Member

@bchristi-git bchristi-git May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this next() used anywhere ?

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither is used, IntellJ was happy to create getters and setters... Can be removed.

* classes may be sufficient to manage the risk of deserializing unexpected classes.
* <p>For an application composed from multiple modules or libraries, the structure
* of the application can be used to identify the classes to be allowed or rejected
* by each {@linkplain ObjectInputStream} in each context of the application.
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should be {@link } here.

* are {@code REJECTED}. Either the class is not {@code ALLOWED} or
* if the class is an array and the base component type is not allowed,
* otherwise the result is {@code UNDECIDED}.
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra "if" at line 395.
it should be a more readable version of the list below implementing checkfilter.
If it does not aid in understanding, it can be removed.

* </code></pre>
*
* @param predicate a predicate to test a non-null Class, non-null
* @param otherStatus a Status to use if the predicate is {@code false}
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of FIlterInputStream javadoc, there is the blanket statement.

 * Unless otherwise noted, passing a {@code null} argument to a
 * method in this interface and its nested classes will cause a
 * {@link NullPointerException} to be thrown.``` 
including non-null on the @ param line reinforces the point

* @return an {@link ObjectInputFilter} that merges the status of the filter and another filter
* @since 17
*/
static ObjectInputFilter merge(ObjectInputFilter filter, ObjectInputFilter anotherFilter) {
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

configLog.log(INFO,
"Creating deserialization filter from {0}", filterString);
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is unchanged in the PR, though DEBUG might be more appropriate.

configLog.log(INFO,
"Creating deserialization filter factory for {0}", factoryClassName);
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark about using INFO level here.

*
* @param filter the serialization filter to set as the system-wide filter; not null
* @param filter the deserialization filter to set as the JVM-wide filter; not null
* @throws SecurityException if there is security manager and the
* {@code SerializablePermission("serialFilter")} is not granted
* @throws IllegalStateException if the filter has already been set {@code non-null}
Copy link
Member

@dfuch dfuch May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Is there a typo/word missing ?

Copy link
Contributor Author

@RogerRiggs RogerRiggs May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-null is unnecessary.

* or based on a {@linkplain Predicate predicate of a class} to
* {@linkplain #allowFilter(Predicate, Status) allow} or
* {@linkplain #rejectFilter(Predicate, Status) reject} classes.
*.
Copy link
Member

@bchristi-git bchristi-git May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra '.' on this line

RogerRiggs added 2 commits May 30, 2021
ensure effective filtering and compatibility with previous releases.
Fixed a bug in allow/rejectFilter()
Cleanup of error stages and messages related setting filter factory
with Config.setSerialFilterFactory.
Updated tests to match.
dfuch
dfuch approved these changes May 31, 2021
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
Added javadoc to describe throwing of ExceptionInInitializerError if the class
named by system property jdk.serialFilterFactory is not valid.
Added description of jdk.serialFilterFactory to java.security file.
src/java.base/share/classes/java/io/ObjectInputFilter.java Outdated Show resolved Hide resolved
* If the filter factory constructor is not invoked successfully, an {@link ExceptionInInitializerError}
* is thrown.
Copy link
Member

@dfuch dfuch Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you also say that later attempts to create an ObjectInputStream or to call ObjectInputStream::setObjectInputFilter will result in an IllegalStateException?

Copy link
Contributor Author

@RogerRiggs RogerRiggs Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and setObjectInputFilter should throw ISE if the initialization from the system property has failed.

     * If the filter factory constructor is not invoked successfully, an {@link ExceptionInInitializerError}
     * is thrown and subsequent use of the filter factory for deserialization fails with
     * {@link IllegalStateException}.

dfuch
dfuch approved these changes Jun 8, 2021
@openjdk openjdk bot removed the csr label Jun 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@RogerRiggs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264859: Implement Context-Specific Deserialization Filters

Reviewed-by: bchristi, dfuchs, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 178 new commits pushed to the master branch:

  • 07108c9: 8268241: deprecate JVM TI Heap functions 1.0
  • c9dbc4f: 8266891: Provide a switch to force the class space to a specific location
  • 2cc1977: 8268424: JFR tests fail due to GC cause 'G1 Preventive Collection' not in the valid causes after JDK-8257774
  • 58a59e3: 8240997: Remove more "hack" word in security codes
  • 1c3932f: 8264766: ClassCastException during template compilation (Variable cannot be cast to Param)
  • f6f82c3: 8266421: Deadlock in Sound System
  • bcaa2cb: 8264144: Add handling of "--about-url" CLI parameter for RPM/DEB packages
  • ae16052: 8268088: Clarify Method::clear_jmethod_ids() related comments in ClassLoaderData::~ClassLoaderData()
  • 5ad4a91: 8268127: Shenandoah: Heap size may be too small for region to align to large page size
  • 7a37816: 8264866: Remove unneeded WorkArounds.isAutomaticModule
  • ... and 168 more: https://git.openjdk.java.net/jdk/compare/66274320251f492b0bba79cdfef88ad5b0d104fa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 9, 2021
@RogerRiggs
Copy link
Contributor Author

@RogerRiggs RogerRiggs commented Jun 9, 2021

/integrate
Thank you for all the comments and suggestions, much appreciated!

@openjdk openjdk bot closed this Jun 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@RogerRiggs Since your change was applied there have been 184 commits pushed to the master branch:

  • dd34a4c: 8268372: ZGC: dynamically select the number of concurrent GC threads used
  • 4388959: 8268056: Update java.net and java.nio to use switch expressions
  • 9cfd560: 8267663: [vector] Add unsigned comparison operators on AArch64
  • 4413142: 8268017: C2: assert(phi_type->isa_int() || phi_type->isa_ptr() || phi_type->isa_long()) failed: bad phi type
  • 2bfd708: 8266557: assert(SafepointMechanism::local_poll_armed(_handshakee)) failed: Must be
  • 4d1cf51: 8240349: jlink should not leave partial image output directory on failure
  • 07108c9: 8268241: deprecate JVM TI Heap functions 1.0
  • c9dbc4f: 8266891: Provide a switch to force the class space to a specific location
  • 2cc1977: 8268424: JFR tests fail due to GC cause 'G1 Preventive Collection' not in the valid causes after JDK-8257774
  • 58a59e3: 8240997: Remove more "hack" word in security codes
  • ... and 174 more: https://git.openjdk.java.net/jdk/compare/66274320251f492b0bba79cdfef88ad5b0d104fa...master

Your commit was automatically rebased without conflicts.

Pushed as commit 13d6180.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@RogerRiggs RogerRiggs deleted the 8264859-context-filter-factory branch Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
5 participants