Security Puzzle (Log4J Edition)

Apache Log4J has been in the spotlight for its JNDI feature introducing a major security flaw in december.

Here is the function for JNDI lookups modified to avoid unexpected remote class being loaded. This function was used to fix potential RCE in version 2.15.

Can you see why the validation was insufficient?

private final DirContext context; //javax.naming.directory.DirContext

[...]

@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 (uri.getScheme() != null) {
        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);
            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);

}

Ref: Changeset

What is/are the weakness in the code above?

• A) Incomplete blacklist / whitelist
• B) Improper URL parsing
• C) Unexpected Nullpointer
• D) Exception handling
• E) Time of Check, Time of Use

Answer (SPOILER!)

It is both a TOCTOU and a URL parsing issue!

• TOCTOU explanation: https://www.gosecure.net/blog/2021/12/21/log4j-2-15-toctou-vulnerability-illustrated-by-gosecure-researchers/
• DNS re-parsing issue: https://www.lunasec.io/docs/blog/log4j-zero-day-severity-of-cve-2021-45046-increased/

Did you identify either weakness?