dengzhhu653 commented on code in PR #6726:
URL: https://github.com/apache/hive/pull/6726#discussion_r3964234919
##########
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:
this might disable the customer class in their udf, why should we restrict
the class as we can restrict the udf to be used?
##########
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:
this might disable the customer class in their udf, why should we restrict
the class as we can limit the udf to be used?
--
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]
