From fa9cd5b48ffc2aeb14370d62d4de0aa43f105fe7 Mon Sep 17 00:00:00 2001 From: Chang Liu Date: Tue, 22 Mar 2022 09:31:41 -0700 Subject: [PATCH] Revert "Replace opensearch class names with opendistro class names during serialization and restore them back during deserialization (#1278)" (#1691) This reverts commit 4abbafc1f49f8ba392538102132876a58e1f9a75. Signed-off-by: cliu123 --- plugin-security.policy | 3 +- .../security/support/Base64Helper.java | 85 ------------------- .../org/opensearch/security/user/User.java | 12 +-- 3 files changed, 8 insertions(+), 92 deletions(-) diff --git a/plugin-security.policy b/plugin-security.policy index d34b40f1a9..fa4840ac7f 100644 --- a/plugin-security.policy +++ b/plugin-security.policy @@ -73,7 +73,8 @@ grant { //Java 9+ permission java.lang.RuntimePermission "accessClassInPackage.com.sun.jndi.*"; - permission java.io.SerializablePermission "enableSubstitution"; + //Enable this permission to debug unauthorized de-serialization attempt + //permission java.io.SerializablePermission "enableSubstitution"; }; grant codeBase "${codebase.netty-common}" { diff --git a/src/main/java/org/opensearch/security/support/Base64Helper.java b/src/main/java/org/opensearch/security/support/Base64Helper.java index 07ceffdf2c..d7dfa01711 100644 --- a/src/main/java/org/opensearch/security/support/Base64Helper.java +++ b/src/main/java/org/opensearch/security/support/Base64Helper.java @@ -31,9 +31,6 @@ package org.opensearch.security.support; import com.amazon.dlic.auth.ldap.LdapUser; -import org.apache.commons.lang3.SerializationUtils; -import org.slf4j.LoggerFactory; -import org.slf4j.Logger; import org.ldaptive.AbstractLdapBean; import org.ldaptive.LdapAttribute; import org.ldaptive.LdapEntry; @@ -49,7 +46,6 @@ import java.io.ObjectStreamClass; import java.io.OutputStream; import java.io.Serializable; -import java.lang.reflect.Field; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.SocketAddress; @@ -60,8 +56,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import java.util.regex.Pattern; import org.opensearch.OpenSearchException; @@ -76,10 +70,6 @@ import com.google.common.io.BaseEncoding; public class Base64Helper { - private static final Logger logger = LoggerFactory.getLogger(Base64Helper.class); - - private static final String ODFE_PACKAGE = "com.amazon.opendistroforelasticsearch"; - private static final String OS_PACKAGE = "org.opensearch"; private static final Set> SAFE_CLASSES = ImmutableSet.of( String.class, @@ -114,69 +104,10 @@ private static boolean isSafeClass(Class cls) { SAFE_ASSIGNABLE_FROM_CLASSES.stream().anyMatch(c -> c.isAssignableFrom(cls)); } - private static class DescriptorNameSetter { - private static final Field NAME = getField(); - - private DescriptorNameSetter() { - } - - private static Field getFieldPrivileged() { - try { - final Field field = ObjectStreamClass.class.getDeclaredField("name"); - field.setAccessible(true); - return field; - } catch (NoSuchFieldException | SecurityException e) { - logger.error("Failed to get ObjectStreamClass declared field", e); - if (e instanceof RuntimeException) { - throw (RuntimeException) e; - } else { - throw new RuntimeException(e); - } - } - } - - private static Field getField() { - SpecialPermission.check(); - return AccessController.doPrivileged((PrivilegedAction) () -> getFieldPrivileged()); - } - - public static void setName(ObjectStreamClass desc, String name) { - try { - logger.debug("replacing descriptor name from [{}] to [{}]", desc.getName(), name); - NAME.set(desc, name); - } catch (IllegalAccessException e) { - logger.error("Failed to replace descriptor name from {} to {}", desc.getName(), name, e); - throw new OpenSearchException(e); - } - } - } - - private static class DescriptorReplacer { - private final ConcurrentMap nameToDescriptor = new ConcurrentHashMap<>(); - - public ObjectStreamClass replace(final ObjectStreamClass desc) { - final String name = desc.getName(); - if (name.startsWith(OS_PACKAGE)) { - return nameToDescriptor.computeIfAbsent(name, s -> { - SpecialPermission.check(); - // we can't modify original descriptor as it is cached by ObjectStreamClass, create clone - final ObjectStreamClass clone = AccessController.doPrivileged( - (PrivilegedAction)() -> SerializationUtils.clone(desc) - ); - DescriptorNameSetter.setName(clone, s.replace(OS_PACKAGE, ODFE_PACKAGE)); - return clone; - }); - } - return desc; - } - } - private final static class SafeObjectOutputStream extends ObjectOutputStream { private static final boolean useSafeObjectOutputStream = checkSubstitutionPermission(); - private final DescriptorReplacer descriptorReplacer = new DescriptorReplacer(); - private static boolean checkSubstitutionPermission() { SecurityManager sm = System.getSecurityManager(); if (sm != null) { @@ -218,11 +149,6 @@ private SafeObjectOutputStream(OutputStream out) throws IOException { ); } - @Override - protected void writeClassDescriptor(final ObjectStreamClass desc) throws IOException { - super.writeClassDescriptor(descriptorReplacer.replace(desc)); - } - @Override protected Object replaceObject(Object obj) throws IOException { Class clazz = obj.getClass(); @@ -276,16 +202,5 @@ protected Class resolveClass(ObjectStreamClass desc) throws IOException, Clas throw new InvalidClassException("Unauthorized deserialization attempt ", clazz.getName()); } - - @Override - protected ObjectStreamClass readClassDescriptor() throws IOException, ClassNotFoundException { - ObjectStreamClass desc = super.readClassDescriptor(); - final String name = desc.getName(); - if (name.startsWith(ODFE_PACKAGE)) { - desc = ObjectStreamClass.lookup(Class.forName(name.replace(ODFE_PACKAGE, OS_PACKAGE))); - logger.debug("replaced descriptor name from [{}] to [{}]", name, desc.getName()); - } - return desc; - } } } diff --git a/src/main/java/org/opensearch/security/user/User.java b/src/main/java/org/opensearch/security/user/User.java index f7f7b93709..a1e2be5614 100644 --- a/src/main/java/org/opensearch/security/user/User.java +++ b/src/main/java/org/opensearch/security/user/User.java @@ -67,7 +67,7 @@ public class User implements Serializable, Writeable, CustomAttributesAware { * roles == backend_roles */ private final Set roles = new HashSet(); - private final Set openDistroSecurityRoles = new HashSet(); + private final Set securityRoles = new HashSet(); private String requestedTenant; private Map attributes = new HashMap<>(); private boolean isInjected = false; @@ -78,7 +78,7 @@ public User(final StreamInput in) throws IOException { roles.addAll(in.readList(StreamInput::readString)); requestedTenant = in.readString(); attributes = in.readMap(StreamInput::readString, StreamInput::readString); - openDistroSecurityRoles.addAll(in.readList(StreamInput::readString)); + securityRoles.addAll(in.readList(StreamInput::readString)); } /** @@ -244,7 +244,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeStringCollection(new ArrayList(roles)); out.writeString(requestedTenant); out.writeMap(attributes, StreamOutput::writeString, StreamOutput::writeString); - out.writeStringCollection(openDistroSecurityRoles ==null?Collections.emptyList():new ArrayList(openDistroSecurityRoles)); + out.writeStringCollection(securityRoles ==null?Collections.emptyList():new ArrayList(securityRoles)); } /** @@ -260,12 +260,12 @@ public synchronized final Map getCustomAttributesMap() { } public final void addSecurityRoles(final Collection securityRoles) { - if(securityRoles != null && this.openDistroSecurityRoles != null) { - this.openDistroSecurityRoles.addAll(securityRoles); + if(securityRoles != null && this.securityRoles != null) { + this.securityRoles.addAll(securityRoles); } } public final Set getSecurityRoles() { - return this.openDistroSecurityRoles == null ? Collections.emptySet() : Collections.unmodifiableSet(this.openDistroSecurityRoles); + return this.securityRoles == null ? Collections.emptySet() : Collections.unmodifiableSet(this.securityRoles); } }