[#2831] - Add optional JEP-290 ObjectInputFilter support to DefaultSerializer, with a conservative resource-limit default on RememberMe deserialization#2832
Conversation
…aultSerializer Add an optional ObjectInputFilter (getObjectInputFilter/setObjectInputFilter, null by default) to DefaultSerializer, applied in deserialize() when set, and pre-configure AbstractRememberMeManager's default serializer with a conservative resource-limit-only filter (maxdepth=30;maxarray=100000;maxrefs=10000;maxbytes=10000000). Document the one-line class-based allow-list override on getSerializer(). The null default means no behavior change for existing callers.
098b81c to
7c6a78d
Compare
There was a problem hiding this comment.
Thank you very much for your contribution!
Please don't force-push. We do squash-and-merge at the end so multiple commits are OK
Looks good for my end, with minor changes.
- This could go to 3.0.1 (milestone is already there and open) so you can change all the
@sinceoccurences there - There should not be any casting needed, and if there is, please use JDK 17 pattern matching
instanceofsyntax instead of casting. - Pull up the API into
Serializerclass so all instanceof and casting can be avoided. - You can add checkstyle suppressions where necessary, if that's an impediment
…3.0.1 Move getObjectInputFilter/setObjectInputFilter onto the Serializer interface as default methods (getter returns null, setter is a no-op); DefaultSerializer overrides them. AbstractRememberMeManager now calls serializer.setObjectInputFilter(...) directly, removing the instanceof check, the cast, and @SuppressWarnings. The documented override example and the test drop their casts too. Change all @SInCE 3.1 occurrences to 3.0.1.
|
Thanks @lprimak, all addressed in a new commit (no force-push):
Rebuilt locally: |
There was a problem hiding this comment.
Approved. I think it gives users an easier way to provide defence-in-depth to RememberMe users.
I really don't see any downsides, but others should take a look at this to see if there is anything I missed.
One question did come up from @rmannibucau - Why do we need all this code if you can just add a system property at runtime?
Because:
- The property (
-Djdk.serialFilter=<filter-pattern>) needs to be added to each application deployment (this way, it's added by default) - The property is global only. The Shiro filtering can be applied only to RememberMe deserialization, not globally
There was a problem hiding this comment.
Pull request overview
This PR introduces opt-in JEP-290 (java.io.ObjectInputFilter) support for Shiro’s Java-serialization-based DefaultSerializer, and preconfigures AbstractRememberMeManager to apply a conservative, resource-limit-only filter when deserializing RememberMe cookie payloads (while documenting how applications can override with a stricter class allow-list).
Changes:
- Add default
getObjectInputFilter()/setObjectInputFilter(ObjectInputFilter)hooks to theSerializerAPI (no-op by default). - Implement filter storage + application in
DefaultSerializer#deserialize(byte[]), and wire a default resource-limit filter intoAbstractRememberMeManager’s default serializer. - Add unit/integration tests covering unfiltered behavior, allow-list behavior, and RememberMe round-trips under the default filter.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lang/src/main/java/org/apache/shiro/lang/io/Serializer.java | Adds default JEP-290 filter accessor/mutator methods to the serializer interface. |
| lang/src/main/java/org/apache/shiro/lang/io/DefaultSerializer.java | Stores an optional ObjectInputFilter and applies it during deserialization. |
| core/src/main/java/org/apache/shiro/mgt/AbstractRememberMeManager.java | Preconfigures the default serializer with a conservative resource-limit filter and documents override guidance. |
| lang/src/test/java/org/apache/shiro/lang/io/DefaultSerializerTest.java | Adds focused tests for filtered/unfiltered DefaultSerializer behavior. |
| core/src/test/java/org/apache/shiro/mgt/AbstractRememberMeManagerObjectInputFilterTest.java | Adds RememberMe-path tests validating default filter behavior and customization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Good question, @rmannibucau. I'd frame these as solving different problems, complementary to the global property rather than a replacement. Scope is the main one. On-by-default, and config not flags. The property has to be set on every deployment's JVM launch (container entrypoint / systemd / app-server args); it's easy to omit and lives in ops/infra config, invisible to the application. A conservative default in Shiro gives RememberMe baseline protection with no operator action, and the stricter allow-list is one line of Shiro config that travels with the application (version-controlled, reviewable) rather than a launch flag. To be upfront about scope: the default this PR ships is resource-limits-only ( |
|
The rational of my comment is that you already need to set it on any JVM due to all CVE we got by the past in that area, this doesn't prevent to have a saner default if not set (easy to check in default impl). On a security standpoint it is saner to use a globally immutable config than a mutable (setter) config I think, this is why most of the solutions come as system properties and static init (thinking to AMQ for ex). Now to make it clear, there is no strong disagreement on the PR, just not a match on the security practise I do promote and rely upon but this is not a big deal if it helps somebody. |
|
Thanks @rmannibucau, that's a fair distinction and I don't think we actually disagree on much. You're right that a security-conscious operator will already have a global Two reasons it landed as a setter here rather than a property:
That said, your comment points at a real interaction worth handling. Because Shiro sets a per-stream filter, in the default configuration (no custom filter factory) it replaces the global Either way, no objection to it landing as-is. Thanks for the review. |
fixes #2831
What
Adds a first-class, opt-in way to constrain the
ObjectInputStreamthat Shiro uses when it deserializes a RememberMe cookie, and wires a conservative default in:DefaultSerializergains an optionalObjectInputFilter(getObjectInputFilter()/setObjectInputFilter(ObjectInputFilter)), applied indeserialize()if set. Default isnull, so behavior for every existing caller is unchanged unless they opt in.AbstractRememberMeManager's default serializer is pre-configured with a conservative, resource-limit-only filter:maxdepth=30;maxarray=100000;maxrefs=10000;maxbytes=10000000. It does not restrict which classes may be deserialized.getSerializer()javadoc documents how an application that knows its principal class shape can opt into a stricter class-based allow-list in one line.The primary value here is the new
setObjectInputFilterAPI and the documented allow-list override: it gives security-conscious operators a supported, one-line way to lock down the RememberMe deserialization sink (DefaultSerializer#deserialize(byte[])->AbstractRememberMeManager#getRememberedPrincipals, which reads the client-supplied cookie before authentication). Today that requires subclassing.Why, and an honest scope of what the default does and does not do
This is the same deserialization sink as the historical Shiro-550 / CVE-2016-4437 issue. The hardcoded default key was fixed there; no per-stream deserialization filter was ever added, and there is no supported way to configure one short of subclassing
DefaultSerializer. SHIRO-824 (2021) shows a user asking for exactly this and being pointed at manual JEP-290 self-subclassing.Being precise about the resource-limit default, because JEP-290 resource limits and class filtering solve different problems:
maxdepth/maxarray/maxrefs/maxbytes) is defense-in-depth against oversized or deeply nested, denial-of-service-shaped payloads reachingreadObject().Key management is an operator responsibility per Shiro's own security model, so this is not claiming to close a stated guarantee; it is optional hardening plus a supported configuration point that did not exist before.
Design choice: resource-limit default, not a class allow-list
PrincipalCollection+ timestamp is orders of magnitude below every limit;maxdepth=30sits well above the depth of realistic principal graphs), while bounding pathological payload sizes. The stronger class-based allow-list is a documented one-liner for applications that want it.Testing
DefaultSerializerTest(4 cases,shiro-lang) andAbstractRememberMeManagerObjectInputFilterTest(4 cases,shiro-core), exercising the raw serializer and the full encrypt/decrypt/deserializeAbstractRememberMeManagerround trip. The oversized-payload rejection test asserts on theInvalidClassExceptioncause (the JEP-290 rejection), which is what actually distinguishes the filtered path from the unfiltered one, plus a realSimplePrincipalCollectionround trip under the new default and the documented allow-list override path.shiro-lang8/8,shiro-core344/344, all green.mvn verifyclean (build + tests + checkstyle) on both modules.java.io.ObjectInputFilteris stable JDK 9+ API, used directly, no reflection.Note on API surface
I initially added a
setObjectInputFilterconvenience method directly onAbstractRememberMeManager, butmvn checkstyle:checkreported the class was already at the configuredMethodCountCheckceiling of 30 methods. Rather than bundle a checkstyle config change into this change, I removed that method: the default is wired inline in the existing no-arg constructor, and the override path uses thesetObjectInputFiltermethod now available onDefaultSerializer(via a documented cast ongetSerializer()). Net new public API onAbstractRememberMeManager: zero. Happy to revisit if you would rather raise the method-count limit and have a first-class setter.A few process notes
@since 3.1is a placeholder for the next release; the current release is 3.0.0 and I did not see a next-minor milestone, so please correct the tag to the actual next version at merge if it differs.Checklist (per PR template)
mvn verify-equivalent checks run locally (build, full test suites, checkstyle)