This is an automated email from the ASF dual-hosted git repository.
kenhuuu pushed a commit to branch 3.7-dev
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/3.7-dev by this push:
new 374b0c76d0 Enforce and guard the Gryo Java serialization hardening
374b0c76d0 is described below
commit 374b0c76d0e90585f154ee55bc4cf7a8840bd43a
Author: Guian Gumpac <[email protected]>
AuthorDate: Mon Aug 10 10:11:48 2026 -0700
Enforce and guard the Gryo Java serialization hardening
Assisted-by: Kiro: Claude Opus 5
---
docs/src/upgrade/release-3.7.x.asciidoc | 14 +-
.../process/traversal/step/sideEffect/IoStep.java | 4 +-
.../gremlin/structure/io/gryo/GryoMapper.java | 38 ++++-
.../traversal/step/sideEffect/IoStepTest.java | 169 +++++++++++++++++++++
.../gremlin/structure/io/gryo/GryoMapperTest.java | 71 +++++++++
5 files changed, 289 insertions(+), 7 deletions(-)
diff --git a/docs/src/upgrade/release-3.7.x.asciidoc
b/docs/src/upgrade/release-3.7.x.asciidoc
index 4b8440eab0..38e939150b 100644
--- a/docs/src/upgrade/release-3.7.x.asciidoc
+++ b/docs/src/upgrade/release-3.7.x.asciidoc
@@ -66,7 +66,9 @@ GryoMapper.build().javaSerializationAllowed(false).create();
----
Callers that supply their own mapper to `GryoReader` or `GryoWriter` are
unaffected and should add
-`javaSerializationAllowed(false)` if they read bytes they do not control.
`GryoIo` accepts an `onMapper` consumer, so
+`javaSerializationAllowed(false)` if they read bytes they do not control.
Dropping those registrations only holds
+while classes must be registered up front, so `create()` rejects
`javaSerializationAllowed(false)` combined with
+`registrationRequired(false)` and throws `IllegalStateException`. `GryoIo`
accepts an `onMapper` consumer, so
full fidelity can be restored where the bytes are trusted:
[source,java]
@@ -74,6 +76,16 @@ full fidelity can be restored where the bytes are trusted:
GryoIo.build().graph(graph).onMapper(m -> ((GryoMapper.Builder)
m).javaSerializationAllowed(true)).create();
----
+Note that the builder handed to `onMapper` is already hardened, so turning
registration off there completes the
+rejected combination even though `javaSerializationAllowed(false)` was never
called directly. Such a consumer now
+needs to opt back into full fidelity explicitly:
+
+[source,java]
+----
+GryoIo.build().graph(graph).onMapper(m -> ((GryoMapper.Builder) m)
+ .registrationRequired(false).javaSerializationAllowed(true)).create();
+----
+
See: link:https://issues.apache.org/jira/browse/TINKERPOP-3278[TINKERPOP-3278]
==== conjoin() Step Null Handling
diff --git
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java
index d42d477f88..839dd5eb0c 100644
---
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java
+++
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStep.java
@@ -141,7 +141,7 @@ public class IoStep<S> extends AbstractStep<S,S> implements
ReadWriting {
* Builds a {@link GraphReader} instance to use. Attempts to detect the
file format to be read using the file
* extension or simply uses configurations provided by the user on the
parameters given to the step.
*/
- private GraphReader constructReader() {
+ GraphReader constructReader() {
final Object objectOrClass = parameters.get(IO.reader,
this::detectFileType).get(0);
if (objectOrClass instanceof GraphReader)
return (GraphReader) objectOrClass;
@@ -176,7 +176,7 @@ public class IoStep<S> extends AbstractStep<S,S> implements
ReadWriting {
* Builds a {@link GraphWriter} instance to use. Attempts to detect the
file format to be write using the file
* extension or simply uses configurations provided by the user on the
parameters given to the step.
*/
- private GraphWriter constructWriter() {
+ GraphWriter constructWriter() {
final Object objectOrClass = parameters.get(IO.writer,
this::detectFileType).get(0);
if (objectOrClass instanceof GraphWriter)
return (GraphWriter) objectOrClass;
diff --git
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java
index 502cc52b7d..75b755c027 100644
---
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java
+++
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapper.java
@@ -180,6 +180,17 @@ public final class GryoMapper implements Mapper<Kryo> {
*/
private final AtomicInteger currentSerializationId = new
AtomicInteger(65536);
+ /**
+ * The message {@link #create()} refuses the unsafe flag combination
with. Package private so that a test can
+ * pin it without restating it.
+ */
+ static final String UNSAFE_COMBINATION_MESSAGE =
+ "javaSerializationAllowed(false) requires
registrationRequired(true), as resolving a class named in "
+ + "the stream rather than registered by id reinstates a
JavaSerializer for any type that declares "
+ + "one as its default. io(), GryoReader, GryoWriter and GryoIo
set javaSerializationAllowed(false) "
+ + "for the caller, so pass javaSerializationAllowed(true) to
opt back into full fidelity for "
+ + "trusted bytes";
+
private boolean registrationRequired = true;
private boolean referenceTracking = true;
private boolean javaSerializationAllowed = true;
@@ -266,10 +277,12 @@ public final class GryoMapper implements Mapper<Kryo> {
* When set to {@code true}, all classes serialized by the {@code
Kryo} instances created from this
* {@link GryoMapper} must have their classes known up front and
registered appropriately through this
* builder. By default this value is {@code true}. This approach is
more efficient than setting the
- * value to {@code false}.
+ * value to {@code false}. It must also stay {@code true} when
+ * {@link #javaSerializationAllowed(boolean)} is {@code false}, as
{@link #create()} rejects that combination.
*
* @param registrationRequired set to {@code true} if the classes
should be registered up front or
* {@code false} otherwise
+ * @see #javaSerializationAllowed(boolean)
*/
public Builder registrationRequired(final boolean
registrationRequired) {
this.registrationRequired = registrationRequired;
@@ -278,7 +291,12 @@ public final class GryoMapper implements Mapper<Kryo> {
/**
* When set to {@code false}, every registration whose serializer is
Kryo's {@code JavaSerializer} is dropped,
- * producing a mapper suited to reading bytes from an untrusted
source. By default this value is {@code true}.
+ * closing the native Java deserialization sink those registrations
carry. This builder defaults the value to
+ * {@code true}, while the paths that read graph documents set it to
{@code false} for the caller, namely
+ * {@code io()}, {@link GryoReader}, {@link GryoWriter}, {@link
GryoIo} and the Hadoop Gryo formats, so a
+ * builder obtained from one of those arrives hardened already.
Dropping the registrations narrows what a Gryo
+ * document can do while decoding rather than making an untrusted
document safe in general, as other
+ * registered types still resolve a class named in the stream.
* <p/>
* That serializer reads by way of {@code
java.io.ObjectInputStream.readObject()}, which reconstructs and runs
* an arbitrary {@code Serializable} object graph while decoding,
before the graph layer can accept or reject
@@ -286,8 +304,13 @@ public final class GryoMapper implements Mapper<Kryo> {
* not need; a stream that carries one now fails with an unregistered
class id. Registrations contributed
* through an {@link IoRegistry} or {@code addCustom(...)} are covered
on the same terms, including those
* whose serializer is a {@code Function} or a class default that
resolves to a {@code JavaSerializer}. This
- * relies on the default {@link #registrationRequired(boolean)} of
{@code true}. Callers that need the full
- * fidelity for trusted, in-process work should leave this value at
{@code true}.
+ * requires {@link #registrationRequired(boolean)} to stay {@code
true}, which {@link #create()} enforces.
+ * Without it a stream may name a class as a string rather than by
registered id, and Kryo then resolves that
+ * class implicitly with its default serializer, which is a {@code
JavaSerializer} for any type declaring one.
+ * Callers that need the full fidelity for trusted, in-process work
should leave this value at {@code true}.
+ * <p/>
+ * A supplied mapper is used as given, so {@link GryoPool}, which
builds its own and is what OLAP relies on,
+ * keeps these registrations and is deliberately out of scope.
*
* @param javaSerializationAllowed set to {@code false} to drop the
{@code JavaSerializer} registrations or
* {@code true} to keep them
@@ -312,8 +335,15 @@ public final class GryoMapper implements Mapper<Kryo> {
/**
* Creates a {@code GryoMapper}.
+ *
+ * @throws IllegalStateException if {@link
#javaSerializationAllowed(boolean)} is {@code false} while
+ * {@link
#registrationRequired(boolean)} is {@code false}, a combination that
+ * would not hold the {@code
JavaSerializer} registrations out
*/
public GryoMapper create() {
+ if (!javaSerializationAllowed && !registrationRequired)
+ throw new IllegalStateException(UNSAFE_COMBINATION_MESSAGE);
+
// consult the registry if provided and inject registry entries as
custom classes.
registries.forEach(registry -> {
final List<Pair<Class, Object>> serializers =
registry.find(GryoIo.class);
diff --git
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStepTest.java
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStepTest.java
new file mode 100644
index 0000000000..b4987c1726
--- /dev/null
+++
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/IoStepTest.java
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect;
+
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import
org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.OptionsStrategy;
+import org.apache.tinkerpop.gremlin.structure.io.GraphReader;
+import org.apache.tinkerpop.gremlin.structure.io.GraphWriter;
+import org.apache.tinkerpop.gremlin.structure.io.gryo.GryoMapper;
+import org.apache.tinkerpop.gremlin.structure.io.gryo.GryoReader;
+import org.apache.tinkerpop.shaded.kryo.io.Output;
+import org.junit.Test;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.OutputStream;
+import java.io.Serializable;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.IsInstanceOf.instanceOf;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * The {@code io()} step reads bytes the caller may not control, so the Gryo
reader and writer it builds must refuse
+ * native Java serialization. Nothing else in this module exercises {@link
IoStep#constructReader()} or
+ * {@link IoStep#constructWriter()}, so without these tests that hardening
could be dropped and the build would stay
+ * green.
+ */
+public class IoStepTest {
+
+ /**
+ * The Gryo type id of {@code OptionsStrategy}, registered with the shaded
{@code JavaSerializer} in both
+ * {@code GryoVersion.V1_0} and {@code GryoVersion.V3_0}, pinned against
the registry in
+ * {@link #shouldInvokeJavaDeserializationOnAFullFidelityGryoReader()}.
+ */
+ private static final int OPTIONS_STRATEGY_GRYO_ID = 187;
+
+ /**
+ * Kryo shifts written class ids to leave room for its {@code NULL} and
{@code NAME} markers.
+ */
+ private static final int CLASS_ID_OFFSET = 2;
+
+ /**
+ * Kryo's reference marker for an object being seen for the first time.
+ */
+ private static final int KRYO_NOT_NULL = 1;
+
+ /**
+ * {@code readObject} is used deliberately rather than the {@code
readGraph} that {@code io().read()} calls.
+ * {@code readGraph} checks the file header first, so the crafted stream
would be rejected on its first byte and
+ * the canary would never run. Both decode with the same mapper this step
builds.
+ */
+ @Test
+ public void shouldNotInvokeJavaDeserializationOnTheGryoReaderIoBuilds()
throws Exception {
+ final GraphReader reader = new IoStep<>(__.start().asAdmin(),
"graph.kryo").constructReader();
+ // without this, a dispatch regression that returned some other reader
would still pass below, since the
+ // crafted Gryo bytes would simply be refused and the canary would
stay unset
+ assertThat(reader, instanceOf(GryoReader.class));
+
+ DeserializationCanary.FIRED = false;
+ try (final InputStream stream = new
ByteArrayInputStream(maliciousGryoBytes())) {
+ reader.readObject(stream, Object.class);
+ } catch (Exception ignored) {
+ // refusing the stream outright is the expected outcome, since the
JavaSerializer backed registration was
+ // dropped. what matters is that nothing was deserialized on the
way to that decision
+ }
+
+ assertFalse("the reader io() builds must not invoke
ObjectInputStream.readObject() on the bytes it reads",
+ DeserializationCanary.FIRED);
+ }
+
+ /**
+ * Positive control for the test above. The same crafted stream must reach
{@code ObjectInputStream.readObject()}
+ * through a full fidelity reader, otherwise that assertion could hold for
the wrong reason and prove nothing.
+ */
+ @Test
+ public void shouldInvokeJavaDeserializationOnAFullFidelityGryoReader()
throws Exception {
+ final GryoMapper fullFidelity = GryoMapper.build().create();
+ assertEquals("OptionsStrategy's gryo id changed, so the crafted stream
no longer selects the JavaSerializer",
+ OPTIONS_STRATEGY_GRYO_ID,
+
fullFidelity.createMapper().getRegistration(OptionsStrategy.class).getId());
+
+ final GryoReader reader =
GryoReader.build().mapper(fullFidelity).create();
+
+ DeserializationCanary.FIRED = false;
+ try (final InputStream stream = new
ByteArrayInputStream(maliciousGryoBytes())) {
+ reader.readObject(stream, Object.class);
+ } catch (Exception ignored) {
+ // the payload deserializes to the canary rather than to an
OptionsStrategy, so a failure is possible
+ // here, but it would come after readObject() has already run
+ }
+
+ assertTrue("the crafted stream must reach
ObjectInputStream.readObject() on a full fidelity reader, " +
+ "otherwise
shouldNotInvokeJavaDeserializationOnTheGryoReaderIoBuilds proves nothing",
+ DeserializationCanary.FIRED);
+ }
+
+ /**
+ * The writer is hardened on the same terms as the reader, so that {@code
io()} cannot write a document it will
+ * not read back.
+ */
+ @Test
+ public void shouldNotWriteTypesWithJavaSerializerOnTheGryoWriterIoBuilds()
throws Exception {
+ final GraphWriter writer = new IoStep<>(__.start().asAdmin(),
"graph.kryo").constructWriter();
+
+ try (final OutputStream stream = new ByteArrayOutputStream()) {
+ writer.writeObject(stream,
OptionsStrategy.build().with("some-key", "some-value").create());
+ fail("the writer io() builds must not write a JavaSerializer
backed type");
+ } catch (IllegalArgumentException expected) {
+ // Kryo refuses the unregistered class, since the registration was
dropped
+ }
+ }
+
+ /**
+ * A Gryo stream that presents {@code OptionsStrategy}'s type id and then
a raw Java-serialized payload. Crafting
+ * it needs no cooperation from the Gryo writer, which is why the sink was
reachable from untrusted bytes.
+ */
+ private byte[] maliciousGryoBytes() throws Exception {
+ final ByteArrayOutputStream javaPayload = new ByteArrayOutputStream();
+ try (final ObjectOutputStream oos = new
ObjectOutputStream(javaPayload)) {
+ oos.writeObject(new DeserializationCanary());
+ }
+
+ final Output malicious = new Output(javaPayload.size() + 64, -1);
+ malicious.writeVarInt(OPTIONS_STRATEGY_GRYO_ID + CLASS_ID_OFFSET,
true);
+ malicious.writeVarInt(KRYO_NOT_NULL, true);
+ malicious.writeBytes(javaPayload.toByteArray());
+ malicious.flush();
+ return malicious.toBytes();
+ }
+
+ /**
+ * A deliberately inert {@code Serializable} used to detect whether native
Java deserialization ran during a Gryo
+ * read. It touches nothing outside this class: no process execution, no
filesystem, no reflection.
+ */
+ private static class DeserializationCanary implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ static volatile boolean FIRED = false;
+
+ private void readObject(final ObjectInputStream in) throws
IOException, ClassNotFoundException {
+ in.defaultReadObject();
+ FIRED = true;
+ }
+ }
+}
diff --git
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java
index 5de7fa31ec..bd997f29c8 100644
---
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java
+++
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/gryo/GryoMapperTest.java
@@ -199,6 +199,33 @@ public class GryoMapperTest {
}
}
+ /**
+ * Dropping the {@code JavaSerializer} registrations only holds while
registration stays required. Without it a
+ * stream may name a class as a string instead of using a registered id,
and Kryo resolves that class implicitly
+ * with its default serializer, which is a {@code JavaSerializer} for any
type declaring one. The combination is
+ * therefore refused rather than producing a mapper that looks hardened
and is not.
+ */
+ @Test
+ public void
shouldRejectJavaSerializationDisabledWithoutRegistrationRequired() {
+ try {
+
builder.get().javaSerializationAllowed(false).registrationRequired(false).create();
+ fail("javaSerializationAllowed(false) with
registrationRequired(false) must be refused");
+ } catch (IllegalStateException expected) {
+ assertEquals(GryoMapper.Builder.UNSAFE_COMBINATION_MESSAGE,
expected.getMessage());
+ }
+ }
+
+ /**
+ * The guard above must not reject either flag on its own, since both
remain legitimate: full fidelity without
+ * registration is how the OLAP pools run, and hardening with registration
required is the IO default.
+ */
+ @Test
+ public void
shouldAllowEitherRegistrationRequiredOrJavaSerializationSettingAlone() {
+ builder.get().registrationRequired(false).create();
+ builder.get().javaSerializationAllowed(false).create();
+
builder.get().javaSerializationAllowed(false).registrationRequired(true).create();
+ }
+
@Test
public void shouldSerializeWithoutRegistration() throws Exception {
final GryoMapper mapper =
builder.get().registrationRequired(false).create();
@@ -461,6 +488,31 @@ public class GryoMapperTest {
DeserializationCanary.FIRED);
}
+ /**
+ * Positive control for {@link
#shouldNotInvokeJavaDeserializationOnGryoReaderRead()}. Supplying a full
fidelity
+ * mapper explicitly must let the same crafted stream reach {@code
ObjectInputStream.readObject()} through
+ * {@link GryoReader}. Without this, that test could pass for the wrong
reason. {@code readObject} is the one
+ * entry point that does not check the header, so were it ever to start
doing so, the crafted bytes would be
+ * rejected on the first byte, the canary would never run, and the
assertion there would hold while proving
+ * nothing.
+ */
+ @Test
+ public void shouldInvokeJavaDeserializationOnDefaultMapperGryoReaderRead()
throws Exception {
+ final GryoReader reader =
GryoReader.build().mapper(builder.get().create()).create();
+
+ DeserializationCanary.FIRED = false;
+ try (final InputStream stream = new
ByteArrayInputStream(maliciousGryoBytes())) {
+ reader.readObject(stream, Object.class);
+ } catch (Exception ignored) {
+ // the payload deserializes to the canary rather than to an
OptionsStrategy, so a failure is possible
+ // here, but it would come after readObject() has already run
+ }
+
+ assertTrue("the crafted stream must reach
ObjectInputStream.readObject() through GryoReader, otherwise " +
+ "shouldNotInvokeJavaDeserializationOnGryoReaderRead
proves nothing",
+ DeserializationCanary.FIRED);
+ }
+
/**
* Hardening the mapper must not cost anything on the graph structure that
a Gryo document actually carries.
*/
@@ -626,6 +678,25 @@ public class GryoMapperTest {
assertEquals(OPTIONS_STRATEGY_GRYO_ID,
restored.getRegistration(OptionsStrategy.class).getId());
}
+ /**
+ * {@link GryoIo} applies its hardening before the {@code onMapper}
consumer, so turning registration off there
+ * leaves the combination the mapper refuses. This is the likelier way to
write it than the direct builder form,
+ * and it changes behaviour for anyone who did, so it is pinned separately.
+ */
+ @Test
+ public void shouldRejectRegistrationNotRequiredThroughGryoIoOnMapper() {
+ final Io.Builder<GryoIo> io = GryoIo.build(gryoVersion());
+ io.graph(EmptyGraph.instance());
+ io.onMapper(m -> ((GryoMapper.Builder) m).registrationRequired(false));
+
+ try {
+ io.create().mapper().create();
+ fail("registrationRequired(false) through onMapper must be
refused, since GryoIo hardens the mapper");
+ } catch (IllegalStateException expected) {
+ assertEquals(GryoMapper.Builder.UNSAFE_COMBINATION_MESSAGE,
expected.getMessage());
+ }
+ }
+
/**
* Without such a consumer, {@link GryoIo} is hardened like the reader and
writer defaults.
*/
