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

Restrict LDAP access via JNDI #608

merged 4 commits into from Dec 5, 2021

Conversation

rgoers
Copy link
Member

@rgoers rgoers commented Nov 30, 2021

Restricts access to LDAP via JNDI.

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.

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

Great job @rgoers! Thanks so much for such a prompt action!

I guess you're gonna incorporate the suggested allowed schemes configuration knob too.

I am also inclined to add a property (log4j2.jndiLookupEnabled?) for toggling JNDI lookups and disable it by default. What do you think?

permanentAllowedClasses.add(Float.class.getName());
permanentAllowedClasses.add(Integer.class.getName());
permanentAllowedClasses.add(Long.class.getName());
permanentAllowedClasses.add(Number.class.getName());
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the classes are final, though Number isn't. I would keep Number out of this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is on the first commit. The second commit added filtering by protocol.
I haven't added a complete kill switch and wasn't planning to. Rather than doing that I think it would be better to figure out a generic way to "hide" plugins.
More importantly, the class is abstract so can never appear here. I am not inspecting sub-classes, only the actual class being instantiated. I will remove Number.

Comment on lines 57 to 58
private static final List<String> permanentAllowedHosts = new ArrayList<>();
private static final List<String> permanentAllowedClasses = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Mind upper-casing these two statics, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way on casing here since the included values aren't constant (e.g. NetUtils.getLocalIps() is dynamic).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @carterkozak that lower-case feels more appropriate since these are not immutable lists.

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

import static org.junit.Assert.fail;

/**
* JndiLookupTest
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
* JndiLookupTest
* LDAP-specialized tests for {@link JndiLookup}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rats. You just reminded me I need to rename this test as it is no longer specific to LDAP.

Copy link

Choose a reason for hiding this comment

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

yes need to write a more generic test

log4j-core/src/test/resources/java-import.ldif Outdated Show resolved Hide resolved
/**
* JndiLookupTest
*/
public class JndiLdapLookupTest {
Copy link
Member

Choose a reason for hiding this comment

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

@rgoers, I cannot see the test where the exploit indeed works when the prevention mechanism is not in place. Am I missing something?

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 tested that before I wrote any code. Once the code was added it isn't possible for it to happen any more as we no longer support Referenceable objects and there is no way to re-enable them. They are simply too dangerous.

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 :-)

…servers and classes that can be accessed via LDAP.
@rgoers rgoers merged commit c77b3cb into release-2.x Dec 5, 2021
@wcc526
Copy link

wcc526 commented Dec 9, 2021

Is it a security vulnerability?

@Glavo
Copy link

Glavo commented Dec 9, 2021

Is it a security vulnerability?

I think it is.

It is very surprising that this critical security issue does not seem to have received due attention. It was reported to Apache half a month ago, but it was not fixed until five days ago. Even today, it has not released a new stable version to solve it.

@garydgregory
Copy link
Member

Is it a security vulnerability?

I think it is.

It is very surprising that this critical security issue does not seem to have received due attention. It was reported to Apache half a month ago, but it was not fixed until five days ago. Even today, it has not released a new stable version to solve it.

Oh so glad you show such appreciation for the work of volunteers...

@Glavo
Copy link

Glavo commented Dec 9, 2021

Is it a security vulnerability?

I think it is.
It is very surprising that this critical security issue does not seem to have received due attention. It was reported to Apache half a month ago, but it was not fixed until five days ago. Even today, it has not released a new stable version to solve it.

Oh so glad you show such appreciation for the work of volunteers...

@garydgregory

I wonder when log4j 2.15 will be officially released? It's hard to imagine that the craziest vulnerability this year has not been solved in the release half a month after it was reported.

Its impact is unimaginable. Countless services using log4j2 are exposed to the risk of being attacked, and the way to attack them is surprisingly simple. Even now I dare not open my minecraft server, because any member can attack it if they want - he/she can easily control my server by sending a text through the chat bar.

Is there anyone dealing with this matter urgently? It's really incomprehensible that I didn't see Apache give any emergency warning under such a serious problem.

@GalvinGao
Copy link

GalvinGao commented Dec 9, 2021

Is it a security vulnerability?

I think it is.
It is very surprising that this critical security issue does not seem to have received due attention. It was reported to Apache half a month ago, but it was not fixed until five days ago. Even today, it has not released a new stable version to solve it.

Oh so glad you show such appreciation for the work of volunteers...

@garydgregory

I wonder when log4j 2.15 will be officially released? It's hard to imagine that the craziest vulnerability this year has not been solved in the release half a month after it was reported.

Its impact is unimaginable. Countless services using log4j2 are exposed to the risk of being attacked, and the way to attack them is surprisingly simple. Even now I dare not open my minecraft server, because any member can attack it if they want - he/she can easily control my server by sending a text through the chat bar.

Is there anyone dealing with this matter urgently? It's really incomprehensible that I didn't see Apache give any emergency warning under such a serious problem.

+1, and we are in desperate need of a CVE and security advisory to be announced asap. This could affect hundreds of thousands, if not millions, of services actively running on the internet.

We of course appreciate the efforts from contributors, but overall this is a major security issue that needs a new version release and a security advisory.

@garydgregory
Copy link
Member

Your patience will soon be rewarded...

@garydgregory
Copy link
Member

Also, if this matters to you so much, why not show it with a donation to the Apache Software Foundation https://www.apache.org/foundation/contributing.html or this project's main contributor https://github.com/sponsors/rgoers ?

@remkop
Copy link
Contributor

remkop commented Dec 9, 2021

Is it a security vulnerability?

I think it is.
It is very surprising that this critical security issue does not seem to have received due attention. It was reported to Apache half a month ago, but it was not fixed until five days ago. Even today, it has not released a new stable version to solve it.

Oh so glad you show such appreciation for the work of volunteers...

@garydgregory
I wonder when log4j 2.15 will be officially released? It's hard to imagine that the craziest vulnerability this year has not been solved in the release half a month after it was reported.
Its impact is unimaginable. Countless services using log4j2 are exposed to the risk of being attacked, and the way to attack them is surprisingly simple. Even now I dare not open my minecraft server, because any member can attack it if they want - he/she can easily control my server by sending a text through the chat bar.
Is there anyone dealing with this matter urgently? It's really incomprehensible that I didn't see Apache give any emergency warning under such a serious problem.

+1, and we are in desperate need of a CVE and security advisory to be announced asap. This could affect hundreds of thousands, if not millions, of services actively running on the internet.

We of course appreciate the efforts from contributors, but overall this is a major security issue that needs a new version release and a security advisory.

My understanding is that the procedure is to hold off on announcing the vulnerability until a patch is available. (See https://www.apache.org/security/).

For background:

The team is taking it seriously. As Gary said, we are all volunteers working on this in our spare time. We are also in different time zones so communication is not instantaneous. If you think things can be improved, that's great! We need more people like you and I would encourage you to get involved!

We are in the process of getting a release out with the fix. During review, some security experts found a new vulnerability in our fix (a way to bypass the fix). This has been addressed and we are now in the process of reviewing the updated 2nd release candidate.

Usually (as per ASF rules) the team should wait 72 hours after creating a release candidate before publishing the release to give the community enough time to review and cast their votes. We are building consensus to shorten that window for this particular release, given its urgency.

@zhangyoufu
Copy link

zhangyoufu commented Dec 9, 2021

You can't ask everybody to upgrade to 2.15 at once. And the formatMsgNoLookups option is available to log4j ≥ 2.10 only.

Thanks to LOG4J2-703, I think it's quite safe to remove org/apache/logging/log4j/core/lookup/JndiLookup.class from log4j-core-*.jar as a workaround. Just zip -q -d log4j-core-*.jar org/apache/logging/log4j/core/lookup/JndiLookup.class to disable ${jndi:...} functionality completely.

I posted all log4j-core jar with JndiLookup.class removed at https://github.com/zhangyoufu/log4j2-without-jndi for reference. Simple local test looks promising.

@remkop
Copy link
Contributor

remkop commented Dec 10, 2021

Update: the vote for log4j-2.15.0 passed and the release is in progress.

I can see the log4j web site reflecting the log4j 2.15.0 release, but I cannot see the 2.15.0 artifacts on Maven Central yet at this moment. It may take a few hours before mirror servers are synchronized and the artifacts become available for you.

An announcement email for the release will be sent out soon (within 24 hours - we usually wait some time for the mirror servers to catch up).

Thank you @zhangyoufu for the suggested workaround for older versions of log4j to remove the JndiLookup.class class! The team likes your idea and we will include the workaround you suggested in the release notes and announcement email. Many thanks!

@moonming
Copy link
Member

@remkop thanks for your great work 👍
I come from the Apache APISIX community, and we can intercept this security vulnerability at the API gateway level to provide some help. I wonder if there are such regular expression rules? thanks

@yuezk
Copy link

yuezk commented Dec 10, 2021

Hi @rgoers, is log4j 1.x vulnerable?

@remkop
Copy link
Contributor

remkop commented Dec 10, 2021

Hi @rgoers, is log4j 1.x vulnerable?

Hi @yuezk, as far as I can tell, log4j 1.x does not support lookups. I also could not find any other reference to JNDI in the log4j 1.x source code. So, no guarantees but it looks like 1.x is not impacted by this vulnerability. CORRECTION: log4j 1.x contains a JMS Appender which can use JNDI. So I would say that, yes, log4j 1.x is also impacted by this vulnerability (Thank you @garydgregory for pointing this out).

Update (2021-12-11 09:09 JST): according to this analysis by @ceki (the author of log4j 1.x), Log4j 1.x is not impacted, since it does not have lookups, and the JMS Appender only loads Strings from the remote server, not serialized objects.

Update (2021-12-12 10:09 JST): according to this analysis by @TopStreamsNet, strictly speaking, applications using Log4j 1.x may be impacted if their configuration uses JNDI. However, the risk is much lower.

Note that log4j 1.x is End of Life and has other security vulnerabilities that will not be fixed. So we do not recommend using log4j 1.x as a way to avoid this security vulnerability. The recommendation is to upgrade to 2.15.0.

@garydgregory
Copy link
Member

garydgregory commented Dec 10, 2021 via email

@garydgregory
Copy link
Member

garydgregory commented Dec 10, 2021 via email

jsoo1 pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 17, 2022
jsoo1 pushed a commit to awakesecurity/nixpkgs that referenced this pull request Jul 18, 2022
Copy link

@alphawoodexec alphawoodexec left a comment

Choose a reason for hiding this comment

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

log4j-core/pom.xml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet