okumin commented on code in PR #6726:
URL: https://github.com/apache/hive/pull/6726#discussion_r3975975040
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java:
##########
@@ -237,31 +242,102 @@ 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)) {
+ // 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, and every class in the
+ // stream must pass the allowlist check. Kryo is otherwise willing to
instantiate any classpath class named by the
+ // payload (registrationRequired=false plus StdInstantiatorStrategy),
which turns these payloads into a
+ // deserialization-of-untrusted-data primitive.
+ if (untrustedRootType != null) {
+ if (classCounter == 0 && !untrustedRootType.isAssignableFrom(type)) {
+ throw new UnsupportedOperationException("The object to be
deserialized must be a "
+ + untrustedRootType.getName() + ", but encountered: " + type);
+ }
+ if (!isAllowedForUntrustedDeserialization(type)) {
throw new UnsupportedOperationException(
- "The object to be deserialized must be an ExprNodeDesc, but
encountered: " + type);
+ "Deserialization of " + type + " is not allowed from an
untrusted payload");
}
}
classCounter++;
return super.getRegistration(type);
}
public void setExprNodeFirst(boolean isPartFilter) {
- this.isExprNodeFirst = isPartFilter;
+ setUntrustedRootType(isPartFilter ? ExprNodeDesc.class : null);
+ }
+
+ void setUntrustedRootType(Class<?> rootType) {
+ this.untrustedRootType = rootType;
+ this.classCounter = 0;
}
// reset the fields on release
public void restore() {
setConf(null);
- isExprNodeFirst = false;
+ untrustedRootType = null;
classCounter = 0;
}
}
+ /**
+ * Package prefixes that classes read from an untrusted Kryo payload may
come from. These cover everything a
+ * legitimate serialized expression ({@link ExprNodeDesc} graph) or search
argument (SearchArgumentImpl graph)
+ * contains: expression descriptors and plan literals, builtin and installed
UDFs, type infos and object inspectors,
+ * Hive/Hadoop value types, and plain JDK value/collection classes. Known
gadget carriers (commons-collections,
+ * beanutils, xalan/TemplatesImpl, ...) all live outside these prefixes.
+ */
+ private static final String[] UNTRUSTED_ALLOWED_PACKAGE_PREFIXES = new
String[] {
+ "java.lang.",
+ "java.util.",
+ "java.sql.",
+ "java.time.",
+ "java.math.",
+ "org.apache.hadoop.hive.ql.plan.",
+ "org.apache.hadoop.hive.ql.udf.",
+ "org.apache.hadoop.hive.ql.io.sarg.",
+ "org.apache.hadoop.hive.serde2.",
+ "org.apache.hadoop.hive.common.type.",
+ "org.apache.hadoop.io."
+ };
+
+ /**
+ * Classes that are never acceptable in an untrusted payload even though
they pass the package allowlist.
+ * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically
disallowed in a secure environment.
+ * {@link
org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater}
+ */
+ private static final Set<String> UNTRUSTED_DENIED_CLASS_NAMES = new
HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect",
+ "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2",
+ "org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile"
+ ));
+
+ @VisibleForTesting
+ static boolean isAllowedForUntrustedDeserialization(Class<?> type) {
+ Class<?> component = type;
+ while (component.isArray()) {
+ component = component.getComponentType();
+ }
+ if (component.isPrimitive()) {
+ return true;
+ }
+ String name = component.getName();
+ if (UNTRUSTED_DENIED_CLASS_NAMES.contains(name)) {
Review Comment:
IMO, HMS should not resolve HS2's configurations in most cases. Also,
`hive.server2.builtin.udf.blacklist` is usually configured via
`SettableConfigUpdater`. It is not easy to ensure that the class is invoked
from HMS. Since these three classes aren't helpful for filtering out
partitions, I'd say banning them doesn't introduce a new issue.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java:
##########
@@ -237,31 +242,102 @@ 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)) {
+ // 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, and every class in the
+ // stream must pass the allowlist check. Kryo is otherwise willing to
instantiate any classpath class named by the
+ // payload (registrationRequired=false plus StdInstantiatorStrategy),
which turns these payloads into a
+ // deserialization-of-untrusted-data primitive.
+ if (untrustedRootType != null) {
+ if (classCounter == 0 && !untrustedRootType.isAssignableFrom(type)) {
+ throw new UnsupportedOperationException("The object to be
deserialized must be a "
+ + untrustedRootType.getName() + ", but encountered: " + type);
+ }
+ if (!isAllowedForUntrustedDeserialization(type)) {
throw new UnsupportedOperationException(
- "The object to be deserialized must be an ExprNodeDesc, but
encountered: " + type);
+ "Deserialization of " + type + " is not allowed from an
untrusted payload");
}
}
classCounter++;
return super.getRegistration(type);
}
public void setExprNodeFirst(boolean isPartFilter) {
- this.isExprNodeFirst = isPartFilter;
+ setUntrustedRootType(isPartFilter ? ExprNodeDesc.class : null);
+ }
+
+ void setUntrustedRootType(Class<?> rootType) {
+ this.untrustedRootType = rootType;
+ this.classCounter = 0;
}
// reset the fields on release
public void restore() {
setConf(null);
- isExprNodeFirst = false;
+ untrustedRootType = null;
classCounter = 0;
}
}
+ /**
+ * Package prefixes that classes read from an untrusted Kryo payload may
come from. These cover everything a
+ * legitimate serialized expression ({@link ExprNodeDesc} graph) or search
argument (SearchArgumentImpl graph)
+ * contains: expression descriptors and plan literals, builtin and installed
UDFs, type infos and object inspectors,
+ * Hive/Hadoop value types, and plain JDK value/collection classes. Known
gadget carriers (commons-collections,
+ * beanutils, xalan/TemplatesImpl, ...) all live outside these prefixes.
+ */
+ private static final String[] UNTRUSTED_ALLOWED_PACKAGE_PREFIXES = new
String[] {
+ "java.lang.",
+ "java.util.",
+ "java.sql.",
+ "java.time.",
+ "java.math.",
+ "org.apache.hadoop.hive.ql.plan.",
+ "org.apache.hadoop.hive.ql.udf.",
+ "org.apache.hadoop.hive.ql.io.sarg.",
+ "org.apache.hadoop.hive.serde2.",
+ "org.apache.hadoop.hive.common.type.",
+ "org.apache.hadoop.io."
+ };
+
+ /**
+ * Classes that are never acceptable in an untrusted payload even though
they pass the package allowlist.
+ * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically
disallowed in a secure environment.
+ * {@link
org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater}
+ */
+ private static final Set<String> UNTRUSTED_DENIED_CLASS_NAMES = new
HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect",
+ "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2",
+ "org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile"
+ ));
+
+ @VisibleForTesting
+ static boolean isAllowedForUntrustedDeserialization(Class<?> type) {
+ Class<?> component = type;
+ while (component.isArray()) {
+ component = component.getComponentType();
+ }
+ if (component.isPrimitive()) {
+ return true;
+ }
+ String name = component.getName();
+ if (UNTRUSTED_DENIED_CLASS_NAMES.contains(name)) {
+ return false;
+ }
+ // Custom (temporary/permanent) UDFs live in user packages. The classes
themselves were
+ // installed by an administrator, so allowing kryo to instantiate them is
no worse than any
+ // query invoking them.
+ if (GenericUDF.class.isAssignableFrom(component) ||
UDF.class.isAssignableFrom(component)) {
+ return true;
+ }
+ for (String prefix : UNTRUSTED_ALLOWED_PACKAGE_PREFIXES) {
+ if (name.startsWith(prefix)) {
+ return true;
+ }
+ }
+ return false;
Review Comment:
Your point is understandable, but I don't have an effective way to restrict
only what we really want to block.
One option is to disallow non-built-in UDFs as well. This could be
reasonable. Does anyone want to extend the reflective partitioning filtering
beyond standard UDFs? If not, we may remove L330-332.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java:
##########
@@ -111,21 +115,64 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes)
throws MetaException {
try {
expr =
SerializationUtilities.deserializeObjectWithTypeInformation(exprBytes, true);
} catch (Exception ex) {
- LOG.error("Failed to deserialize the expression, fall back to
deserializeObjectFromKryo", ex);
+ LOG.error("Failed to deserialize the expression, fall back to
deserializeUntrustedObjectFromKryo", ex);
try {
- expr = SerializationUtilities.deserializeObjectFromKryo(exprBytes,
ExprNodeGenericFuncDesc.class);
+ // The fallback must use the same untrusted-payload restrictions as
the primary path: these bytes come straight
+ // from a Thrift client.
+ expr =
SerializationUtilities.deserializeUntrustedObjectFromKryo(exprBytes,
ExprNodeGenericFuncDesc.class);
} catch (Exception e) {
LOG.error("Failed to deserialize the expression", e);
throw new
MetaException("SerializationUtilities#deserializeObjectWithTypeInformation: " +
ex.getMessage() +
- ", SerializationUtilities#deserializeObjectFromKryo: " +
e.getMessage());
+ ", SerializationUtilities#deserializeUntrustedObjectFromKryo: " +
e.getMessage());
}
}
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. The Kryo-level class allowlist already blocks
reflect/reflect2/java_method/in_file; a
+ * {@link GenericUDFBridge} instance is legitimate (it wraps builtin
old-style UDFs like year()), but it instantiates
+ * whatever class name its {@code udfClassName} field carries, so that name
must resolve to a real {@link UDF}.
+ */
+ 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 (genericUDF instanceof GenericUDFBridge genericUDFBridge) {
+ String udfClassName = genericUDFBridge.getUdfClassName();
+ Class<?> udfClass;
Review Comment:
It is reasonable. I updated.
https://github.com/apache/hive/pull/6726/changes/b1edbc84e488b66ac0b69fee6628675e394e5782
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
