apache / accumulo

Apache Accumulo
https://accumulo.apache.org
Apache License 2.0
1.07k stars 444 forks source link

Use ObjectInputFilter when deserializing fate objects. #3660

Open keith-turner opened 1 year ago

keith-turner commented 1 year ago

Describe the bug Fate uses java serialization w/o any validation of the java objects read from persistent storage. Starting with Java 9 there is a new mechanism that could be used to validate object prior to deserialization.

The following is a diff for a quick experiment I did that did not work. With the following change a sunny day IT would not run and the manager logs were full of errors. So the following is probably too strict, did not look into why it was failing.

diff --git a/core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java b/core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java
index da8572c7cb..b8a44e62f8 100644
--- a/core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java
+++ b/core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java
@@ -25,6 +25,7 @@ import static org.apache.accumulo.core.util.UtilWaitThread.sleepUninterruptibly;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.ObjectInputFilter;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
@@ -87,6 +88,13 @@ public class ZooStore<T> implements TStore<T> {
     try {
       ByteArrayInputStream bais = new ByteArrayInputStream(ser);
       ObjectInputStream ois = new ObjectInputStream(bais);
+      ois.setObjectInputFilter(filterInfo -> {
+        var clazz = filterInfo.serialClass();
+        if(clazz != null && clazz.getName().startsWith("org.apache.accumulo")){
+          return ObjectInputFilter.Status.ALLOWED;
+        }
+        return ObjectInputFilter.Status.REJECTED;
+      });
       return ois.readObject();
     } catch (Exception e) {
       throw new RuntimeException(e);

Expected behavior Fate deserialization only works with a narrow set of types.

EdColeman commented 1 year ago

It has been discussed to look into using Java Records (jdk-14?) when we can to avoid the serialization issue.

keith-turner commented 1 year ago

It has been discussed to look into using Java Records (jdk-14?) when we can to avoid the serialization issue.

It would be good to move away from java serialization in Fate. Json+Gson would be nice or if Java records works that would be nice. While we are using java serialization it would be good to make it more strict.

ctubbsii commented 1 year ago

Changing the overall serialization should be done in a major release. If this ticket is targeting something more narrow for 2.1 only, that might be nice. But it still may be a lot of effort without much return, if we're going to change to a safer serialization strategy anyway after 2.1/3.0, which I'd love to do for 3.1.