diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java index b61f4d484439..cf341fdcbad3 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java +++ b/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 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 deserializeObjectFromKryo(byte[] bytes, Class clazz) { - Input inp = new Input(new ByteArrayInputStream(bytes)); - Kryo kryo = borrowKryo(); - T func = null; - try { - func = kryo.readObject(inp, clazz); + public static T deserializeObjectFromKryo(byte[] bytes, Class 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) { diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java index 522c9896684f..ff952a391f5d 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java +++ b/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> 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 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); } } diff --git a/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java b/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java index fab397c52063..8eb1f8563ae1 100644 --- a/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java +++ b/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); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java index 0c003d5e46de..eef5891f2cce 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java +++ b/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()); + 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 children = new ArrayList<>(); + children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "col1", "tab", false)); + children.add(constant); + return new ExprNodeGenericFuncDesc(TypeInfoFactory.booleanTypeInfo, + new GenericUDFOPEqual(), children); + } } diff --git a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java new file mode 100644 index 000000000000..a074a599eb8f --- /dev/null +++ b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java @@ -0,0 +1,124 @@ +/* + * 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.hadoop.hive.ql.optimizer.ppr; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.ql.exec.SerializationUtilities; +import org.apache.hadoop.hive.ql.exec.UDFArgumentException; +import org.apache.hadoop.hive.ql.metadata.HiveException; +import org.apache.hadoop.hive.ql.plan.ExprNodeColumnDesc; +import org.apache.hadoop.hive.ql.plan.ExprNodeConstantDesc; +import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; +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.GenericUDFOPEqual; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2; +import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector; +import org.apache.hadoop.hive.serde2.objectinspector.primitive.PrimitiveObjectInspectorFactory; +import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; +import org.junit.Assert; +import org.junit.Test; + +/** + * Verifies that the metastore-side expression deserialization only accepts benign expression + * graphs: the expression bytes arrive straight from Thrift clients, so classes outside the + * allowlist, reflect()/reflect2(), and GenericUDFBridge instances pointing at non-UDF classes + * must all be rejected before anything stringifies or evaluates the expression. + */ +public class TestPartitionExpressionForMetastore { + public static class CustomGenericUDF extends GenericUDF { + @Override + public ObjectInspector initialize(ObjectInspector[] arguments) throws UDFArgumentException { + return PrimitiveObjectInspectorFactory.writableVoidObjectInspector; + } + + @Override + public Object evaluate(DeferredObject[] arguments) throws HiveException { + return null; + } + + @Override + public String getDisplayString(String[] children) { + return "custom_udf"; + } + } + + @Test + public void testComparisonExpressionIsAccepted() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFOPEqual(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "2026-08-11")); + String filter = new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + Assert.assertNotNull(filter); + } + + @Test(expected = MetaException.class) + public void testReflectUdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFReflect(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testReflect2UdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFReflect2(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testInFileUdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFInFile(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "/tmp/x")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testCustomUdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new CustomGenericUDF(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "test")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testBridgeToNonUdfClassIsRejected() throws Exception { + GenericUDFBridge bridge = new GenericUDFBridge("evil", false, "java.lang.ProcessBuilder"); + ExprNodeGenericFuncDesc expr = buildExpression(bridge, + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "x")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + private ExprNodeGenericFuncDesc buildExpression(GenericUDF udf, ExprNodeConstantDesc constant) { + List children = new ArrayList<>(); + children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "ds", "tab", true)); + children.add(constant); + return new ExprNodeGenericFuncDesc(TypeInfoFactory.booleanTypeInfo, udf, children); + } +}