TINKERPOP-3278 Disable Java serialization on Gryo IO reads - #3578
TINKERPOP-3278 Disable Java serialization on Gryo IO reads#3578GumpacG wants to merge 6 commits into
Conversation
| this.graphFilter = VertexProgramHelper.deserialize(ConfUtil.makeApacheConfiguration(configuration), Constants.GREMLIN_HADOOP_GRAPH_FILTER); | ||
| this.gryoReader = GryoReader.build().mapper( | ||
| GryoMapper.build().addRegistries(IoRegistryHelper.createRegistries(ConfUtil.makeApacheConfiguration(configuration))).create()).create(); | ||
| GryoMapper.build().javaSerializationAllowed(false).addRegistries(IoRegistryHelper.createRegistries(ConfUtil.makeApacheConfiguration(configuration))).create()).create(); |
There was a problem hiding this comment.
for our documentation sake, could you please add a comment as to why we disable java serialization here - something that say why it isn't necessary basically. my take is that as a record reader (and you need a similar comment in GryoRecordWriter) it is meant to serialize a graph Element which would not serialize things like OptionsStrategy and classes that use JavaSerializer.
this is in contrast to the MapReduce shuffle path (which we didn't change): VertexWritable.write()/readFields() go through KryoShimServiceLoader, which uses the GryoPool from HadoopPools. That pool is built with initializeMapper(m -> m.registrationRequired(false)) — i.e. deliberately unlocked, and not hardened. That's consistent with the threat model, which explicitly places "the Hadoop object pools that additionally run unlocked" out-of-model (they only carry intra-job graph elements between mapper and reducer, not attacker-controlled bytes). I think you should probably add a clarifying comment there about why we didn't disable the JavaSerializer there.
There was a problem hiding this comment.
Added comments, thanks!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.7-dev #3578 +/- ##
=============================================
+ Coverage 75.49% 75.53% +0.03%
- Complexity 13161 13184 +23
=============================================
Files 1092 1093 +1
Lines 67208 67269 +61
Branches 7391 7401 +10
=============================================
+ Hits 50742 50810 +68
+ Misses 13837 13836 -1
+ Partials 2629 2623 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| this.typeRegistrations = builder.typeRegistrations; | ||
| this.javaSerializationAllowed = builder.javaSerializationAllowed; | ||
| this.typeRegistrations = builder.typeRegistrations.stream(). | ||
| filter(tr -> javaSerializationAllowed || !(tr.getShadedSerializer() instanceof JavaSerializer)). |
There was a problem hiding this comment.
Is there a reason we are doing the filtering of direct JavaSerializer here in the constructor, instead of inside createMapper() via the resolvesToJavaSerializer helper with the rest?
There was a problem hiding this comment.
It was a miss. I've moved it to the helper function. Thanks!
|
VOTE +1 - recommend squashing commits in this case. not sure there's much value or history to preserve in this little change. |
Summary
Several Gryo type registrations, mostly
TraversalStrategyimplementations andTraversalExplanation,were bound to Kryo's
JavaSerializer, which deserializes viajava.io.ObjectInputStream.readObject().Reading a Gryo document from an untrusted source could therefore reconstruct and run an arbitrary
Serializableobject graph during decoding, before the graph layer accepts or rejects anything.This change stops the graph-document IO paths from reaching that sink, while leaving full fidelity
available for trusted, in-process use.
What changed
New API
GryoMapper.Builder.javaSerializationAllowed(boolean), defaulttrue. Whenfalse, every registrationwhose serializer resolves to a
JavaSerializeris dropped, including registrations contributed through anIoRegistryoraddCustom(...), whether the serializer is supplied directly, as aFunction, or via aclass default serializer.
Paths hardened (build their mappers with
javaSerializationAllowed(false))io()step (read and write branches)GryoReaderandGryoWriterdefault mappersGryoIo(coversgraph.io(IoCore.gryo())andgryograph persistence)GryoRecordReader,GryoRecordWriter)Reads and writes are hardened together so these paths cannot produce a document they will not read back.
Unchanged (full fidelity retained for trusted bytes)
GryoMapper.build().create()GryoPool, and thespark-gremlinand Hadoop object pools (which additionally run unlocked by design)Behavior change
A stream that presents one of the affected type ids now fails with an unregistered class id instead of
being deserialized:
Affected types:
PartitionStrategy,SubgraphStrategy,SeedStrategy,VertexProgramStrategy,ProductiveByStrategy,OptionsStrategyandTraversalExplanationin both Gryo 1.0 and 3.0, plusGroupStep.GroupBiOperatorandOrderGlobalStep.OrderBiOperatorin Gryo 1.0.Breaking changes and capability impact
Breaking: reading a Gryo document that embeds one of the affected types now fails on the default IO paths
(
io(),GryoReader,GryoWriter,GryoIo, and the Hadoop formats) instead of deserializing it. Restore theold behavior where the bytes are trusted with
javaSerializationAllowed(true)(see below). Additive API only,no signatures changed;
GryoMapper.build().create()and graph-structure documents are unaffected.A graph document produced by
writeGraph/writeVerticescontains only vertices, edges, properties andtheir values, never strategies, so in practice nothing is lost on the graph-document paths. Nothing in the
repository round-trips these types through Gryo: OLAP ships strategies to workers via JDK serialization in
the job configuration, not through the Gryo registrations.
The capability is removed from the default, not from the product. A directly built mapper keeps full
fidelity, and
javaSerializationAllowed(true)restores it on any builder.GryoIoexposes it through theonMapperconsumer where the bytes are trusted:Migration: a caller holding an older
.kryodocument that embeds one of the affected types can read it bysupplying a mapper with
javaSerializationAllowed(true)toGryoReader, or via theonMapperhook above.Testing
GryoMapperTest(parameterized over Gryo 1.0 and 3.0): a canary provingreadObject()does not run on ahardened read, a positive control proving the crafted bytes do reach the sink on a full-fidelity mapper
(so the negative assertions are meaningful), graph-structure round-trips, and registration checks against
the constructed
Kryofor default, direct-custom,Function-custom and class-default-serializer forms.GryoRecordReaderWriterTest: asserts the Hadoop reader and writer build hardened mappers.Docs
CHANGELOG entry, an upgrade note in
docs/src/upgrade/release-3.7.x.asciidoc, and scoped updates toTHREAT_MODEL.md(sections 8 and 9) describing which mappers are hardened.Assisted-by: Kiro: Claude Opus 4.8