HIVE-29838

ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java

@@ -126,8 +126,8 @@ private static class KryoWithHooks extends Kryo implements Configurable {
     private Hook globalHook;
     // this should be set on-the-fly after borrowing this instance and needs to be reset on release
     private Configuration configuration;
-    // default false, should be reset on release
-    private boolean isExprNodeFirst = false;
+    // when non-null, the first class read must be compatible with this type
+    private Class<?> rootType = null;
     // total classes we have met during (de)serialization, should be reset on release
     private long classCounter = 0;
 
@@ -237,27 +237,26 @@ public Configuration getConf() {
 
     @Override
     public com.esotericsoftware.kryo.kryo5.Registration getRegistration(Class type) {
-      // If PartitionExpressionForMetastore performs deserialization at remote HMS,
-      // the first class encountered during deserialization must be an ExprNodeDesc,
-      // throw exception to avoid potential security problem if it is not.
-      if (isExprNodeFirst && classCounter == 0) {
-        if (!ExprNodeDesc.class.isAssignableFrom(type)) {
-          throw new UnsupportedOperationException(
-              "The object to be deserialized must be an ExprNodeDesc, but encountered: " + type);
-        }
+      // If this instance deserializes a payload that a remote client controls (e.g. PartitionExpressionForMetastore at
+      // a remote HMS) or that a client can persist (e.g. a table property copied into the job conf), the first class
+      // encountered during deserialization must be compatible with the expected root type.
+      if (rootType != null && classCounter == 0 && !rootType.isAssignableFrom(type)) {
+        throw new UnsupportedOperationException("The object to be deserialized must be a "
+            + rootType.getName() + ", but encountered: " + type);
       }
       classCounter++;
       return super.getRegistration(type);
     }
 
-    public void setExprNodeFirst(boolean isPartFilter) {
-      this.isExprNodeFirst = isPartFilter;
+    void setRootType(Class<?> rootType) {
+      this.rootType = rootType;
+      this.classCounter = 0;
     }
 
     // reset the fields on release
     public void restore() {
       setConf(null);
-      isExprNodeFirst = false;
+      rootType = null;
       classCounter = 0;
     }
   }
@@ -868,7 +867,7 @@ public static byte[] serializeObjectWithTypeInformation(Serializable object) {
   public static <T> T deserializeObjectWithTypeInformation(byte[] bytes,
       boolean isPartFilter) {
     KryoWithHooks kryo = (KryoWithHooks) borrowKryo();
-    kryo.setExprNodeFirst(isPartFilter);
+    kryo.setRootType(isPartFilter ? ExprNodeDesc.class : null);
     try (Input inp = new Input(new ByteArrayInputStream(bytes))) {
       return (T) kryo.readClassAndObject(inp);
     } finally {
@@ -899,17 +898,14 @@ public static byte[] serializeObjectToKryo(Serializable object) {
     return baos.toByteArray();
   }
 
-  public static <T extends Serializable> T deserializeObjectFromKryo(byte[] bytes, Class<T> clazz) {
-    Input inp = new Input(new ByteArrayInputStream(bytes));
-    Kryo kryo = borrowKryo();
-    T func = null;
-    try {
-      func = kryo.readObject(inp, clazz);
+  public static <T> T deserializeObjectFromKryo(byte[] bytes, Class<T> clazz) {
+    KryoWithHooks kryo = (KryoWithHooks) borrowKryo();
+    kryo.setRootType(clazz);
+    try (Input inp = new Input(new ByteArrayInputStream(bytes))) {
+      return kryo.readObject(inp, clazz);
     } finally {
       releaseKryo(kryo);
     }
-    inp.close();
-    return func;
   }
 
   public static String serializeObject(Serializable expr) {

ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java

@@ -25,20 +25,29 @@
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.hadoop.hive.metastore.FileFormatProxy;
 import org.apache.hadoop.hive.metastore.PartitionExpressionProxy;
 import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.ql.exec.FunctionRegistry;
 import org.apache.hadoop.hive.ql.exec.SerializationUtilities;
+import org.apache.hadoop.hive.ql.exec.UDF;
 import org.apache.hadoop.hive.ql.io.orc.OrcFileFormatProxy;
 import org.apache.hadoop.hive.ql.io.orc.OrcInputFormat;
-import org.apache.hadoop.hive.ql.io.sarg.ConvertAstToSearchArg;
 import org.apache.hadoop.hive.ql.io.sarg.SearchArgument;
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl;
 import org.apache.hadoop.hive.ql.metadata.HiveException;
 import org.apache.hadoop.hive.ql.parse.SemanticException;
 import org.apache.hadoop.hive.ql.plan.ExprNodeDesc;
 import org.apache.hadoop.hive.ql.plan.ExprNodeDescUtils;
 import org.apache.hadoop.hive.ql.plan.ExprNodeGenericFuncDesc;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDF;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDFBridge;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDFMacro;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2;
 import org.apache.hadoop.hive.serde2.typeinfo.PrimitiveTypeInfo;
 import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory;
 import org.slf4j.Logger;
@@ -50,6 +59,18 @@
 public class PartitionExpressionForMetastore implements PartitionExpressionProxy {
   private static final Logger LOG = LoggerFactory.getLogger(PartitionExpressionForMetastore.class);
 
+  /**
+   * Classes that are never acceptable in a partition expression.
+   * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically disallowed in a secure environment.
+   * This set should be in sync with the denylist in
+   * {@link org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater}.
+   */
+  private static final Set<Class<? extends GenericUDF>> DENIED_UDFS = Set.of(
+      GenericUDFReflect.class,
+      GenericUDFReflect2.class,
+      GenericUDFInFile.class
+  );
+
   @Override
   public String convertExprToFilter(byte[] exprBytes, String defaultPartitionName, boolean decodeFilterExpToStr)
       throws MetaException {
@@ -123,9 +144,48 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException {
     if (expr == null) {
       throw new MetaException("Failed to deserialize expression - ExprNodeDesc not present");
     }
+    validateDeserializedExpr(expr);
     return expr;
   }
 
+  /**
+   * Rejects client-supplied expression graphs that would execute arbitrary code when the metastore stringifies or
+   * evaluates them.
+   */
+  private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException {
+    if (expr instanceof ExprNodeGenericFuncDesc exprNodeGenericFuncDesc) {
+      validateDeserializedExprNodeGenericFuncDesc(exprNodeGenericFuncDesc);
+    }
+    if (expr.getChildren() != null) {
+      for (ExprNodeDesc child : expr.getChildren()) {
+        validateDeserializedExpr(child);
+      }
+    }
+  }
+
+  private void validateDeserializedExprNodeGenericFuncDesc(ExprNodeGenericFuncDesc expr) throws MetaException {
+    GenericUDF genericUDF = expr.getGenericUDF();
+    if (DENIED_UDFS.contains(genericUDF.getClass())) {
+      throw new MetaException(genericUDF.getUdfName() + " is not allowed in partition expressions");
+    }
+    if (!FunctionRegistry.isBuiltInFuncExpr(expr)) {
+      throw new MetaException("Only built-in UDFs are allowed in partition expressions");
+    }
+    if (genericUDF instanceof GenericUDFBridge genericUDFBridge) {
+      Class<? extends UDF> udfClass = genericUDFBridge.getUdfClass();
+      if (!UDF.class.isAssignableFrom(udfClass)) {
+        throw new MetaException("Class in partition filter expression is not a UDF: " + udfClass);
+      }
+    }
+    if (genericUDF instanceof GenericUDFMacro genericUDFMacro) {
+      // a macro body is an expression graph of its own
+      ExprNodeDesc body = genericUDFMacro.getBody();
+      if (body != null) {
+        validateDeserializedExpr(body);
+      }
+    }
+  }
+
   @Override
   public FileFormatProxy getFileFormatProxy(FileMetadataExprType type) {
     switch (type) {
@@ -150,6 +210,6 @@ public FileMetadataExprType getMetadataType(String inputFormat) {
 
   @Override
   public SearchArgument createSarg(byte[] expr) {
-    return ConvertAstToSearchArg.create(expr);
+    return SerializationUtilities.deserializeObjectFromKryo(expr, SearchArgumentImpl.class);
   }
 }

ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java

@@ -55,7 +55,6 @@
 
 import com.google.common.collect.Lists;
 
-
 import static org.junit.Assert.fail;
 import static org.junit.Assert.assertEquals;
 import org.junit.Before;
@@ -170,6 +169,19 @@ public void testPartitionExpr() throws Exception {
     } catch (IMetaStoreClient.IncompatibleMetastoreException ignore) {
     }
 
+    // Denied expression => throw the specific exception
+    try {
+      var expr = e.val("currentTimeMillis").val("java.lang.System").fn("reflect", TypeInfoFactory.intTypeInfo, 2).val(0)
+          .pred("=", 2).build();
+      checkExpr(-1, dbName, tblName, expr, tbl);
+      fail("Should have thrown");
+    } catch (IMetaStoreClient.IncompatibleMetastoreException ex) {
+      assertEquals(
+          "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect is not allowed in partition expressions",
+          ex.getMessage()
+      );
+    }
+
     // Invalid expression => throw some exception, but not incompatible metastore.
     try {
       checkExpr(-1, dbName, tblName, e.val(31).intCol("p3").pred(">", 2).build(), tbl);

ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java

@@ -46,13 +46,13 @@
 import org.apache.hadoop.hive.ql.plan.PartitionDesc;
 import org.apache.hadoop.hive.ql.plan.TableDesc;
 import org.apache.hadoop.hive.ql.plan.VectorPartitionDesc;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPEqual;
 import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPNull;
 import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory;
 import org.junit.Assert;
 import org.junit.Test;
 
 public class TestSerializationUtilities {
-
   @Test
   public void testEveryPropertiesAreSerialized() throws Exception {
     MapWork mapWork = doSerDeser(null);
@@ -246,4 +246,53 @@ private static MapWork mockMapWorkWithSomePartitionDescProperties() throws Excep
 
     return mapWork;
   }
+
+  @Test
+  public void testDeserializeObjectWithTypeInformationAcceptsLegitimateExpression() {
+    ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc(
+        TypeInfoFactory.stringTypeInfo, "value"));
+
+    byte[] typed = SerializationUtilities.serializeObjectWithTypeInformation(expr);
+    Object deserialized = SerializationUtilities.deserializeObjectWithTypeInformation(typed, true);
+    Assert.assertTrue(deserialized instanceof ExprNodeGenericFuncDesc);
+
+    String base64 = SerializationUtilities.serializeExpression(expr);
+    Assert.assertNotNull(SerializationUtilities.deserializeExpression(base64));
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void testDeserializeObjectWithTypeInformationRejectsNonExprRoot() {
+    byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(
+        new LinkedHashMap<String, String>());
+    SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true);
+  }
+
+  @Test
+  public void testDeserializeObjectFromKryoAcceptsLegitimateExpression() {
+    ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc(
+        TypeInfoFactory.stringTypeInfo, "value"));
+    String exprString = "(col1 = 'value')";
+    Assert.assertEquals(exprString, expr.getExprString());
+
+    byte[] kryo = SerializationUtilities.serializeObjectToKryo(expr);
+    Assert.assertEquals(
+        exprString,
+        SerializationUtilities.deserializeObjectFromKryo(kryo, ExprNodeGenericFuncDesc.class).getExprString()
+    );
+    Assert.assertNotNull(SerializationUtilities.deserializeObjectFromKryo(kryo, Object.class));
+
+    String base64 = SerializationUtilities.serializeExpression(expr);
+    Assert.assertEquals(
+        exprString,
+        SerializationUtilities.deserializeExpression(base64).getExprString()
+    );
+  }
+
+  private static ExprNodeGenericFuncDesc buildColumnEqualsConstant(ExprNodeConstantDesc constant) {
+    List<ExprNodeDesc> children = new ArrayList<>();
+    children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "col1", "tab", false));
+    children.add(constant);
+    return new ExprNodeGenericFuncDesc(TypeInfoFactory.booleanTypeInfo,
+        new GenericUDFOPEqual(), children);
+  }
 }