Restrict LDAP access via JNDI #608
Conversation
| public static List<String> getLocalIps() { | ||
| List<String> localIps = new ArrayList<>(); | ||
| localIps.add("localhost"); | ||
| localIps.add("127.0.0.1"); |
There was a problem hiding this comment.
Perhaps IPv6 as well [::1]? (brackets may or may not be necessary depending how we extract the host)
There was a problem hiding this comment.
I could have but this pre-seeding really isn't necessary as they show up in the while loop anyway.
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()); |
There was a problem hiding this comment.
The rest of the classes are final, though Number isn't. I would keep Number out of this list.
There was a problem hiding this comment.
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.
| private static final List<String> permanentAllowedHosts = new ArrayList<>(); | ||
| private static final List<String> permanentAllowedClasses = new ArrayList<>(); |
There was a problem hiding this comment.
Mind upper-casing these two statics, please?
There was a problem hiding this comment.
I could go either way on casing here since the included values aren't constant (e.g. NetUtils.getLocalIps() is dynamic).
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
| * Test LDAP object | |
| * Malicious LDAP object that shouldn't be deserialized. | |
| * | |
| * @see JndiLdapLookupTest |
| import static org.junit.Assert.fail; | ||
|
|
||
| /** | ||
| * JndiLookupTest |
There was a problem hiding this comment.
| * JndiLookupTest | |
| * LDAP-specialized tests for {@link JndiLookup}. |
There was a problem hiding this comment.
Rats. You just reminded me I need to rename this test as it is no longer specific to LDAP.
There was a problem hiding this comment.
yes need to write a more generic test
| /** | ||
| * JndiLookupTest | ||
| */ | ||
| public class JndiLdapLookupTest { |
There was a problem hiding this comment.
@rgoers, I cannot see the test where the exploit indeed works when the prevention mechanism is not in place. Am I missing something?
There was a problem hiding this comment.
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.
log4j-core/src/main/java/org/apache/logging/log4j/core/net/JndiManager.java
Outdated
Show resolved
Hide resolved
| LOGGER.warn("Attempt to access ldap server not in allowed list"); | ||
| return null; | ||
| } | ||
| Attributes attributes = this.context.getAttributes(name); |
There was a problem hiding this comment.
Are we confident the attributes resolved here are equivalent to the results of context.lookup? I imagine the attributes could change over time.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well, yeah. That is how DirContext is documented to work. https://docs.oracle.com/javase/8/docs/api/javax/naming/directory/DirContext.html#getAttributes-java.lang.String-
There was a problem hiding this comment.
Perfect, thank you for the explanation, I'm not very familiar with these components :-)
…servers and classes that can be accessed via LDAP.
|
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... |
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. |
|
Your patience will soon be rewarded... |
|
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 ? |
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. |
|
You can't ask everybody to upgrade to 2.15 at once. And the Thanks to LOG4J2-703, I think it's quite safe to remove I posted all log4j-core jar with |
|
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 |
|
@remkop thanks for your great work |
|
Hi @rgoers, is log4j 1.x vulnerable? |
Hi @yuezk, as far as I can tell, log4j 1.x does not support lookups. 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. |
|
We need to look at the log4j 1 JMS Appender which I thought had at least
programmatic support for JNDI.
Gary
…On Thu, Dec 9, 2021, 20:26 Remko Popma ***@***.***> wrote:
Hi @rgoers <https://github.com/rgoers>, is log4j 1.x vulnerable?
Hi @yuezk <https://github.com/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
<http://svn.apache.org/viewvc/logging/log4j/trunk/>. So, no guarantees
but it looks like 1.x is not impacted by this vulnerability.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#608 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJB6NYD7R35WOHFKBO3ILLUQFJLHANCNFSM5JA3ZEGA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
As documented here:
https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/net/JMSAppender.html
Gary
…On Thu, Dec 9, 2021, 20:30 Gary Gregory ***@***.***> wrote:
We need to look at the log4j 1 JMS Appender which I thought had at least
programmatic support for JNDI.
Gary
On Thu, Dec 9, 2021, 20:26 Remko Popma ***@***.***> wrote:
> Hi @rgoers <https://github.com/rgoers>, is log4j 1.x vulnerable?
>
> Hi @yuezk <https://github.com/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
> <http://svn.apache.org/viewvc/logging/log4j/trunk/>. So, no guarantees
> but it looks like 1.x is not impacted by this vulnerability.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#608 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAJB6NYD7R35WOHFKBO3ILLUQFJLHANCNFSM5JA3ZEGA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
|
|
I reported this denial of Service Vulnerability (cve-2021-45046) to the logging PMC last week. I proposed the core code and trigger method of denial of service vulnerability. In terms of time, I should have proposed it earlier, but I didn't see my name in the CVE credit. I hope my name can also be added to the CVE credit. 4ra1n |
@EmYiQing Apologies if we missed anyone in giving proper credit to the people who helped discovering these vulnerabilities. The Apache Logging PMC have been extremely busy with confirming and fixing the vulnerability reports, releasing new versions of the software, and communicating about the vulnerabilities. When I woke up this morning I found my inbox had grown to 700+ unread messages. :-) Please don't hesitate to remind us again in a few days or a week or so when we have a bit more breathing space. Thank you for your understanding! |
|
@remkop Thank you for your reply |
@EmYiQing The bug was originally disclosed to Apache on November 24th by Chen Zhaojun of Alibaba Cloud Security Team - https://www.mcafee.com/blogs/enterprise/mcafee-enterprise-atr/log4shell-vulnerability-is-the-coal-in-our-stocking-for-2021/ |
|
@mrdgsmith Here I discuss the denial of Service Vulnerability CVE-2021-45046 rather than the JNDI injection Vulnerability CVE-2021-44228 |
@EmYiQing I updated the credit section for the |
|
@remkop Thank you very much for your work |
|
@remkop Description of
I can confirm that |
|
@quaff Good point yes. |
@remkop Can you contact somebody to update CVE description that |
|
@quaff Thanks for noticing this! The Log4j security page has already been updated to reflect this. |
Restricts access to LDAP via JNDI.
The text was updated successfully, but these errors were encountered: