Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict LDAP access via JNDI #608

Merged
merged 4 commits into from
Dec 5, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions log4j-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,11 @@
<artifactId>awaitility</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.zapodot</groupId>
<artifactId>embedded-ldap-junit</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ public static class Builder<B extends Builder<B>> extends AbstractAppender.Build
@PluginBuilderAttribute
private boolean immediateFail;

@PluginBuilderAttribute
private String allowedLdapClasses;

@PluginBuilderAttribute
private String allowedLdapHosts;

@PluginBuilderAttribute
private String allowedJndiProtocols;

// Programmatic access only for now.
private JmsManager jmsManager;

Expand All @@ -100,8 +109,21 @@ public JmsAppender build() {
JmsManager actualJmsManager = jmsManager;
JmsManagerConfiguration configuration = null;
if (actualJmsManager == null) {
Properties additionalProperties = null;
if (allowedLdapClasses != null || allowedLdapHosts != null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this reference allowedJndiProtocols as well?

additionalProperties = new Properties();
if (allowedLdapHosts != null) {
additionalProperties.put(JndiManager.ALLOWED_HOSTS, allowedLdapHosts);
}
if (allowedLdapClasses != null) {
additionalProperties.put(JndiManager.ALLOWED_CLASSES, allowedLdapClasses);
}
if (allowedJndiProtocols != null) {
additionalProperties.put(JndiManager.ALLOWED_PROTOCOLS, allowedJndiProtocols);
}
}
final Properties jndiProperties = JndiManager.createProperties(factoryName, providerUrl, urlPkgPrefixes,
securityPrincipalName, securityCredentials, null);
securityPrincipalName, securityCredentials, additionalProperties);
configuration = new JmsManagerConfiguration(jndiProperties, factoryBindingName, destinationBindingName,
userName, password, false, reconnectIntervalMillis);
actualJmsManager = AbstractManager.getManager(getName(), JmsManager.FACTORY, configuration);
Expand Down Expand Up @@ -202,6 +224,21 @@ public Builder setUserName(final String userName) {
return this;
}

public Builder setAllowedLdapClasses(final String allowedLdapClasses) {
this.allowedLdapClasses = allowedLdapClasses;
return this;
}

public Builder setAllowedLdapHosts(final String allowedLdapHosts) {
this.allowedLdapHosts = allowedLdapHosts;
return this;
}

public Builder setAllowedJndiProtocols(final String allowedJndiProtocols) {
this.allowedJndiProtocols = allowedJndiProtocols;
return this;
}

/**
* Does not include the password.
*/
Expand All @@ -212,7 +249,8 @@ public String toString() {
+ ", securityCredentials=" + securityCredentials + ", factoryBindingName=" + factoryBindingName
+ ", destinationBindingName=" + destinationBindingName + ", username=" + userName + ", layout="
+ getLayout() + ", filter=" + getFilter() + ", ignoreExceptions=" + isIgnoreExceptions()
+ ", jmsManager=" + jmsManager + "]";
+ ", jmsManager=" + jmsManager + ", allowedLdapClasses=" + allowedLdapClasses
+ ", allowedLdapHosts=" + allowedLdapHosts + ", allowedJndiProtocols=" + allowedJndiProtocols + "]";
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,69 @@

package org.apache.logging.log4j.core.net;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Properties;
import java.util.concurrent.TimeUnit;

import javax.naming.Context;
import javax.naming.InitialContext;
import javax.naming.NamingEnumeration;
import javax.naming.NamingException;
import javax.naming.directory.Attribute;
import javax.naming.directory.Attributes;
import javax.naming.directory.DirContext;
import javax.naming.directory.InitialDirContext;

import org.apache.logging.log4j.core.appender.AbstractManager;
import org.apache.logging.log4j.core.appender.ManagerFactory;
import org.apache.logging.log4j.core.util.JndiCloser;
import org.apache.logging.log4j.core.util.NetUtils;
import org.apache.logging.log4j.util.PropertiesUtil;

/**
* Manages a JNDI {@link javax.naming.Context}.
* Manages a JNDI {@link javax.naming.directory.DirContext}.
*
* @since 2.1
*/
public class JndiManager extends AbstractManager {

public static final String ALLOWED_HOSTS = "allowedLdapHosts";
public static final String ALLOWED_CLASSES = "allowedLdapClasses";
public static final String ALLOWED_PROTOCOLS = "allowedJndiProtocols";

private static final JndiManagerFactory FACTORY = new JndiManagerFactory();
private static final String PREFIX = "log4j2.";
private static final String LDAP = "ldap";
private static final String LDAPS = "ldaps";
private static final String JAVA = "java";
private static final List<String> permanentAllowedHosts = NetUtils.getLocalIps();
private static final List<String> permanentAllowedClasses = Arrays.asList(Boolean.class.getName(),
Byte.class.getName(), Character.class.getName(), Double.class.getName(), Float.class.getName(),
Integer.class.getName(), Long.class.getName(), Short.class.getName(), String.class.getName());
private static final List<String> permanentAllowedProtocols = Arrays.asList(JAVA, LDAP, LDAPS);
private static final String SERIALIZED_DATA = "javaSerializedData";
private static final String CLASS_NAME = "javaClassName";
private static final String REFERENCE_ADDRESS = "javaReferenceAddress";
private static final String OBJECT_FACTORY = "javaFactory";
private final List<String> allowedHosts;
private final List<String> allowedClasses;
private final List<String> allowedProtocols;

private final Context context;
private final DirContext context;

private JndiManager(final String name, final Context context) {
private JndiManager(final String name, final DirContext context, final List<String> allowedHosts,
final List<String> allowedClasses, final List<String> allowedProtocols) {
super(null, name);
this.context = context;
this.allowedHosts = allowedHosts;
this.allowedClasses = allowedClasses;
this.allowedProtocols = allowedProtocols;
}

/**
Expand Down Expand Up @@ -168,21 +206,89 @@ protected boolean releaseSub(final long timeout, final TimeUnit timeUnit) {
* @throws NamingException if a naming exception is encountered
*/
@SuppressWarnings("unchecked")
public <T> T lookup(final String name) throws NamingException {
public synchronized <T> T lookup(final String name) throws NamingException {
try {
URI uri = new URI(name);
if (!allowedProtocols.contains(uri.getScheme().toLowerCase(Locale.ROOT))) {
LOGGER.warn("Log4j JNDI does not allow protocol {}", uri.getScheme());
return null;
}
if (LDAP.equalsIgnoreCase(uri.getScheme()) || LDAPS.equalsIgnoreCase(uri.getScheme())) {
if (!allowedHosts.contains(uri.getHost())) {
LOGGER.warn("Attempt to access ldap server not in allowed list");
return null;
}
Attributes attributes = this.context.getAttributes(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we confident the attributes resolved here are equivalent to the results of context.lookup? I imagine the attributes could change over time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you mean the attributes associated with the returned item. All the attributes returned from LDAP for Java objects are specified in https://datatracker.ietf.org/doc/html/rfc2713.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make sure that context.getAttributes(name) verifies against the same data (cached without any sort of refresh or network call) as the subsequent call to context.lookup(name), otherwise the verification isn't very helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you for the explanation, I'm not very familiar with these components :-)

if (attributes != null) {
// In testing the "key" for attributes seems to be lowercase while the attribute id is
// camelcase, but that may just be true for the test LDAP used here. This copies the Attributes
// to a Map ignoring the "key" and using the Attribute's id as the key in the Map so it matches
// the Java schema.
Map<String, Attribute> attributeMap = new HashMap<>();
NamingEnumeration<? extends Attribute> enumeration = attributes.getAll();
while (enumeration.hasMore()) {
Attribute attribute = enumeration.next();
attributeMap.put(attribute.getID(), attribute);
}
Attribute classNameAttr = attributeMap.get(CLASS_NAME);
if (attributeMap.get(SERIALIZED_DATA) != null) {
if (classNameAttr != null) {
String className = classNameAttr.get().toString();
if (!allowedClasses.contains(className)) {
LOGGER.warn("Deserialization of {} is not allowed", className);
return null;
}
} else {
LOGGER.warn("No class name provided for {}", name);
return null;
}
} else if (attributeMap.get(REFERENCE_ADDRESS) != null
|| attributeMap.get(OBJECT_FACTORY) != null) {
LOGGER.warn("Referenceable class is not allowed for {}", name);
return null;
}
}
}
} catch (URISyntaxException ex) {
// This is OK.
}
return (T) this.context.lookup(name);
}

private static class JndiManagerFactory implements ManagerFactory<JndiManager, Properties> {

@Override
public JndiManager createManager(final String name, final Properties data) {
String hosts = data != null ? data.getProperty(ALLOWED_HOSTS) : null;
String classes = data != null ? data.getProperty(ALLOWED_CLASSES) : null;
String protocols = data != null ? data.getProperty(ALLOWED_PROTOCOLS) : null;
List<String> allowedHosts = new ArrayList<>();
List<String> allowedClasses = new ArrayList<>();
List<String> allowedProtocols = new ArrayList<>();
addAll(hosts, allowedHosts, permanentAllowedHosts, ALLOWED_HOSTS, data);
addAll(classes, allowedClasses, permanentAllowedClasses, ALLOWED_CLASSES, data);
addAll(protocols, allowedProtocols, permanentAllowedProtocols, ALLOWED_PROTOCOLS, data);
try {
return new JndiManager(name, new InitialContext(data));
return new JndiManager(name, new InitialDirContext(data), allowedHosts, allowedClasses,
allowedProtocols);
} catch (final NamingException e) {
LOGGER.error("Error creating JNDI InitialContext.", e);
return null;
}
}

private void addAll(String toSplit, List<String> list, List<String> permanentList, String propertyName,
Properties data) {
if (toSplit != null) {
list.addAll(Arrays.asList(toSplit.split("\\s*,\\s*")));
data.remove(propertyName);
}
toSplit = PropertiesUtil.getProperties().getStringProperty(PREFIX + propertyName);
if (toSplit != null) {
list.addAll(Arrays.asList(toSplit.split("\\s*,\\s*")));
}
list.addAll(permanentList);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package org.apache.logging.log4j.core.util;

import java.io.File;
import java.net.Inet4Address;
import java.net.Inet6Address;
import java.net.InetAddress;
import java.net.MalformedURLException;
import java.net.NetworkInterface;
Expand All @@ -25,11 +27,14 @@
import java.net.URISyntaxException;
import java.net.URL;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
import java.util.List;

import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.status.StatusLogger;
import org.apache.logging.log4j.util.Strings;

/**
* Networking-related convenience methods.
Expand Down Expand Up @@ -79,6 +84,45 @@ public static String getLocalHostname() {
}
}

public static List<String> getLocalIps() {
List<String> localIps = new ArrayList<>();
localIps.add("localhost");
localIps.add("127.0.0.1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps IPv6 as well [::1]? (brackets may or may not be necessary depending how we extract the host)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have but this pre-seeding really isn't necessary as they show up in the while loop anyway.

try {
final InetAddress addr = Inet4Address.getLocalHost();
setHostName(addr, localIps);
} catch (final UnknownHostException ex) {
// Ignore this.
}
try {
final Enumeration<NetworkInterface> interfaces = NetworkInterface.getNetworkInterfaces();
if (interfaces != null) {
while (interfaces.hasMoreElements()) {
final NetworkInterface nic = interfaces.nextElement();
final Enumeration<InetAddress> addresses = nic.getInetAddresses();
while (addresses.hasMoreElements()) {
final InetAddress address = addresses.nextElement();
setHostName(address, localIps);
}
}
}
} catch (final SocketException se) {
// ignore.
}
return localIps;
}

private static void setHostName(InetAddress address, List<String> localIps) {
String[] parts = address.toString().split("\\s*/\\s*");
if (parts.length > 0) {
for (String part : parts) {
if (Strings.isNotBlank(part) && !localIps.contains(part)) {
localIps.add(part);
}
}
}
}

/**
* Returns the local network interface's MAC address if possible. The local network interface is defined here as
* the {@link java.net.NetworkInterface} that is both up and not a loopback interface.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache license, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the license for the specific language governing permissions and
* limitations under the license.
*/
package org.apache.logging.log4j.core.lookup;

import javax.naming.Context;
import javax.naming.Name;
import javax.naming.spi.ObjectFactory;
import java.util.Hashtable;

import static org.junit.jupiter.api.Assertions.fail;

/**
* Test LDAP object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Test LDAP object
* Malicious LDAP object that shouldn't be deserialized.
*
* @see JndiLdapLookupTest

*/
public class JndiExploit implements ObjectFactory {
@Override
public Object getObjectInstance(Object obj, Name name, Context nameCtx, Hashtable<?, ?> environment)
throws Exception {
fail("getObjectInstance must not be allowed");
return null;
}
}
Loading