Security: fix(spanner), implement look-ahead deserialization filtering in CloudClientExecutor#12308
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a secure deserialization mechanism in CloudClientExecutor by implementing a look-ahead filter with a custom SecureObjectInputStream and an allowlist of authorized classes. The review feedback identifies critical issues: the class names in the allowlist use incorrect casing (e.g., partitionOptions instead of PartitionOptions), which would cause legitimate deserialization to fail, and the unmarshall method signature was unnecessarily broadened to throw Exception instead of specific checked exceptions.
| "com.google.cloud.spanner.partitionOptions", | ||
| "com.google.cloud.spanner.partition", |
There was a problem hiding this comment.
The class names com.google.cloud.spanner.partitionOptions and com.google.cloud.spanner.partition in the ALLOWED_CLASSES set appear to be incorrect. Based on standard Java naming conventions and the pull request description mentioning com.google.cloud.spanner.Partition, these should likely start with an uppercase letter (e.g., Partition and PartitionOptions). If these names are wrong, legitimate deserialization for partitioned reads/queries will fail with an InvalidClassException, breaking existing functionality.
| "com.google.cloud.spanner.partitionOptions", | |
| "com.google.cloud.spanner.partition", | |
| "com.google.cloud.spanner.PartitionOptions", | |
| "com.google.cloud.spanner.Partition", |
| throws IOException, ClassNotFoundException { | ||
| ObjectInputStream objectInputStream = new ObjectInputStream(input.newInput()); | ||
| return (T) objectInputStream.readObject(); | ||
| private <T extends Serializable> T unmarshall(ByteString input) throws Exception { |
There was a problem hiding this comment.
The unmarshall method's signature was changed to throw Exception, which is too broad. This forces callers to catch a generic Exception, obscuring the specific exceptions that can be thrown and potentially hiding other issues. The previous signature throws IOException, ClassNotFoundException was more precise. The new implementation can throw IOException (from input.newInput(), SecureObjectInputStream constructor, ois.readObject(), and InvalidClassException) and ClassNotFoundException (from ois.readObject()). Please revert to a more specific exception signature.
| private <T extends Serializable> T unmarshall(ByteString input) throws Exception { | |
| private <T extends Serializable> T unmarshall(ByteString input) throws IOException, ClassNotFoundException { |
missoum1307
left a comment
missoum1307
left a comment
There was a problem hiding this comment.
Fixed the casing for Partition and PartitionOptions and narrowed the exception signature to IOException, ClassNotFoundException as suggested.
1 participant
This PR addresses an insecure deserialization vulnerability in the CloudClientExecutor unmarshalling logic. By replacing the default ObjectInputStream with a hardened SecureObjectInputStream, we now enforce a strict allowlist of classes permitted for de-serialization.
The unmarshall helper method previously deserialized ByteString inputs (such as Transaction IDs and Partition tokens) without validating the underlying class types. This could allow the instantiation of unauthorized classes if a malicious payload was provided via the gRPC interface.
Changes:
Introduced SecureObjectInputStream, a subclass of ObjectInputStream that overrides resolveClass.
Implemented ALLOWED_CONF_CLASSES, a strictly defined Set of permitted classes including BatchTransactionId and Partition.
Updated unmarshall to use the new filtered stream.
Regression Testing: Confirmed that legitimate Spanner operations, specifically Partitioned Reads/Queries, remain functional as com.google.cloud.spanner.Partition and its dependencies are explicitly allowed.