Skip to content

Commit

Permalink
Revert "Replace opensearch class names with opendistro class names du…
Browse files Browse the repository at this point in the history 
…ring serialization and restore them back during deserialization (#1278)" (#1691)

This reverts commit 4abbafc.

Signed-off-by: cliu123 <[email protected]>
  • Loading branch information
@cliu123
cliu123 authored Mar 22, 2022
1 parent e638c76 commit fa9cd5b
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 92 deletions.
3 changes: 2 additions & 1 deletion plugin-security.policy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}" {
Expand Down
85 changes: 0 additions & 85 deletions src/main/java/org/opensearch/security/support/Base64Helper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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<Class<?>> SAFE_CLASSES = ImmutableSet.of(
String.class,
Expand Down Expand Up @@ -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<Field>) () -> 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<String, ObjectStreamClass> 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<ObjectStreamClass>)() -> 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) {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
}
}
12 changes: 6 additions & 6 deletions src/main/java/org/opensearch/security/user/User.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public class User implements Serializable, Writeable, CustomAttributesAware {
* roles == backend_roles
*/
private final Set<String> roles = new HashSet<String>();
private final Set<String> openDistroSecurityRoles = new HashSet<String>();
private final Set<String> securityRoles = new HashSet<String>();
private String requestedTenant;
private Map<String, String> attributes = new HashMap<>();
private boolean isInjected = false;
Expand All @@ -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));
}

/**
Expand Down Expand Up @@ -244,7 +244,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeStringCollection(new ArrayList<String>(roles));
out.writeString(requestedTenant);
out.writeMap(attributes, StreamOutput::writeString, StreamOutput::writeString);
out.writeStringCollection(openDistroSecurityRoles ==null?Collections.emptyList():new ArrayList<String>(openDistroSecurityRoles));
out.writeStringCollection(securityRoles ==null?Collections.emptyList():new ArrayList<String>(securityRoles));
}

/**
Expand All @@ -260,12 +260,12 @@ public synchronized final Map<String, String> getCustomAttributesMap() {
}

public final void addSecurityRoles(final Collection<String> securityRoles) {
if(securityRoles != null && this.openDistroSecurityRoles != null) {
this.openDistroSecurityRoles.addAll(securityRoles);
if(securityRoles != null && this.securityRoles != null) {
this.securityRoles.addAll(securityRoles);
}
}

public final Set<String> getSecurityRoles() {
return this.openDistroSecurityRoles == null ? Collections.emptySet() : Collections.unmodifiableSet(this.openDistroSecurityRoles);
return this.securityRoles == null ? Collections.emptySet() : Collections.unmodifiableSet(this.securityRoles);
}
}

0 comments on commit fa9cd5b

Please sign in to comment.