Skip to content

Commit

Permalink
Restore User class to be binary compatible with the previous version
Browse files Browse the repository at this point in the history 
Execute privileged actions using AccessController
Refactored code to simplify and generalize name replacement in serialization descriptor
  • Loading branch information
@vrozov
vrozov committed Jun 19, 2021
1 parent c7ee229 commit b478722
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 29 deletions.
3 changes: 1 addition & 2 deletions plugin-security.policy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,7 @@ grant {
//Java 9+
permission java.lang.RuntimePermission "accessClassInPackage.com.sun.jndi.*";

//Enable this permission to debug unauthorized de-serialization attempt
//permission java.io.SerializablePermission "enableSubstitution";
permission java.io.SerializablePermission "enableSubstitution";
};

grant codeBase "${codebase.netty-common}" {
Expand Down
91 changes: 70 additions & 21 deletions src/main/java/org/opensearch/security/support/Base64Helper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
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,6 +78,9 @@
public class Base64Helper {
private static final Logger logger = LogManager.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,
SocketAddress.class,
Expand Down Expand Up @@ -109,10 +114,69 @@ 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 @@ -143,7 +207,6 @@ static ObjectOutputStream create(ByteArrayOutputStream out) throws IOException {

private SafeObjectOutputStream(OutputStream out) throws IOException {
super(out);
//useProtocolVersion(PROTOCOL_VERSION_2);

SecurityManager sm = System.getSecurityManager();
if (sm != null) {
Expand All @@ -156,21 +219,8 @@ private SafeObjectOutputStream(OutputStream out) throws IOException {
}

@Override
protected void writeClassDescriptor(ObjectStreamClass desc) throws IOException {
if (desc.getName().equals(User.class.getName())) {
final Field name;
try {
desc = SerializationUtils.clone(desc);
name = desc.getClass().getDeclaredField("name");
name.setAccessible(true);
name.set(desc, "com.amazon.opendistroforelasticsearch.security.user.User");
logger.warn("Changed desc {}", desc);
} catch (ReflectiveOperationException e) {
logger.error("Failed to change desc {} name", desc, e);
}
//desc = ObjectStreamClass.lookup(com.amazon.opendistroforelasticsearch.security.user.User.class);
}
super.writeClassDescriptor(desc);
protected void writeClassDescriptor(final ObjectStreamClass desc) throws IOException {
super.writeClassDescriptor(descriptorReplacer.replace(desc));
}

@Override
Expand Down Expand Up @@ -230,12 +280,11 @@ protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException, Clas
@Override
protected ObjectStreamClass readClassDescriptor() throws IOException, ClassNotFoundException {
ObjectStreamClass desc = super.readClassDescriptor();

if (desc.getName().equals("com.amazon.opendistroforelasticsearch.security.user.User")) {
desc = ObjectStreamClass.lookup(org.opensearch.security.user.User.class);
logger.warn("replaced class desc {}", desc);
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;
}
}
Expand Down
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> securityRoles = new HashSet<String>();
private final Set<String> openDistroSecurityRoles = 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);
securityRoles.addAll(in.readList(StreamInput::readString));
openDistroSecurityRoles.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(securityRoles ==null?Collections.emptyList():new ArrayList<String>(securityRoles));
out.writeStringCollection(openDistroSecurityRoles ==null?Collections.emptyList():new ArrayList<String>(openDistroSecurityRoles));
}

/**
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.securityRoles != null) {
this.securityRoles.addAll(securityRoles);
if(securityRoles != null && this.openDistroSecurityRoles != null) {
this.openDistroSecurityRoles.addAll(securityRoles);
}
}

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

0 comments on commit b478722

Please sign in to comment.