DanielLeens commented on PR #11557: URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-5391448834
Thanks for the careful diffing against `e62beaaadf98` -> `8dfbf773206e`, @SEZ9 -- you're right that neither of those two commits touches `ManagedSourceRegisterOperation.java`, because I wanted to bring back the actual justification you asked for rather than a placeholder fix. I went back through `readInternal`/`writeInternal` in `ManagedSourceRegisterOperation.java` again against how the rest of the engine serializes this exact type, and I don't think this is unsafe Java deserialization in the sense the finding implies: - `readerLocation` is a `TaskLocation`, which implements Hazelcast's `IdentifiedDataSerializable` (`TaskLocation.java:34`), not `java.io.Serializable`-style native deserialization. `in.readObject()`/`out.writeObject()` here route through Hazelcast's own factory/classId mechanism, not `ObjectInputStream`. - The factory is a closed switch statement in `TaskDataSerializerHook.java` (`Factory.create(int typeId)`): `case TASK_LOCATION_TYPE: return new TaskLocation();` -- it can only ever construct one of the classes explicitly listed in that switch, keyed by an int classId. There is no reflection-based arbitrary-class lookup, so there's no gadget-chain surface the way there would be with raw Java native deserialization. - This is also not a pattern unique to this PR: `SourceRegisterOperation`, `SinkRegisterOperation`, `AssignSplitOperation`, `RequestSplitOperation`, `CloseIdleReaderOperation`, `LastCheckpointNotifyOperation`, and `GetTaskGroupAddressOperation` (at minimum) all serialize `TaskLocation` the exact same way today on `dev`. If this is a real vulnerability, it is an engine-wide property of every RPC operation that carries a `TaskLocation`, not something `ManagedSourceRegisterOperation` introduces. Given that, I'd like to close F1 as "not a new or unique risk introduced by this PR" rather than fix it locally here, since a local-only fix (hand-rolling the three `TaskLocation` fields instead of `writeObject`/`readObject`) would just make this one operation inconsistent with every sibling operation for no real security gain. If you still think the underlying pattern needs hardening, I'd support opening a separate engine-wide hardening issue that looks at all `IdentifiedDataSerializable` RPC payloads together, rather than scoping it to this feature PR. Let me know if I'm missing a concrete exploit path here that is specific to the Hazelcast `IdentifiedDataSerializable` factory model -- happy to keep digging if so. Separately, I'll get the conflict with `dev` resolved in the next push so that gate isn't blocking review in parallel. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
