Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.7k
Restrict LDAP access via JNDI#608
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
publicstaticList<String>getLocalIps() { | ||
List<String>localIps =newArrayList<>(); | ||
localIps.add("localhost"); | ||
localIps.add("127.0.0.1"); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this 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 suggestedallowed 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.
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 arefinal
, thoughNumber
isn't. I would keepNumber
out of this list.
There was a problem hiding this comment.
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.
privatestaticfinalList<String>permanentAllowedHosts =newArrayList<>(); | ||
privatestaticfinalList<String>permanentAllowedClasses =newArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Mind upper-casing these twostatic
s, please?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
importstaticorg.junit.jupiter.api.Assertions.fail; | ||
/** | ||
* Test LDAP object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
*TestLDAPobject | |
*MaliciousLDAPobjectthatshouldn't be deserialized. | |
* | |
*@seeJndiLdapLookupTest |
import static org.junit.Assert.fail; | ||
/** | ||
* JndiLookupTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
*JndiLookupTest | |
*LDAP-specializedtestsfor {@linkJndiLookup}. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading.Please reload this page.
/** | ||
* JndiLookupTest | ||
*/ | ||
public class JndiLdapLookupTest { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
log4j-core/src/main/java/org/apache/logging/log4j/core/net/JndiManager.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
LOGGER.warn("Attempt to access ldap server not in allowed list"); | ||
returnnull; | ||
} | ||
Attributesattributes =this.context.getAttributes(name); |
There was a problem hiding this comment.
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 ofcontext.lookup
? I imagine the attributes could change over time.
There was a problem hiding this comment.
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 inhttps://datatracker.ietf.org/doc/html/rfc2713.
There was a problem hiding this comment.
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 thatcontext.getAttributes(name)
verifies against the same data (cached without any sort of refresh or network call) as the subsequent call tocontext.lookup(name)
, otherwise the verification isn't very helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
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.
wcc526 commentedDec 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Is it a security vulnerability? |
Glavo commentedDec 9, 2021
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 commentedDec 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedDec 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
+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 Foundationhttps://www.apache.org/foundation/contributing.html or this project's main contributorhttps://github.com/sponsors/rgoers ? |
My understanding is that the procedure is to hold off on announcing the vulnerability until a patch is available. (Seehttps://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 toget 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 teamshould 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 commentedDec 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You can't ask everybody to upgrade to 2.15 at once. And the Thanks toLOG4J2-703, I think it's quite safe to remove I posted all log4j-core jar with |
remkop commentedDec 10, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Update: the vote for log4j-2.15.0 passed and the release is in progress. I can see the log4j web site reflecting thelog4j 2.15.0 release, but I cannot see the 2.15.0artifacts 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 👍 |
yuezk commentedDec 10, 2021
Hi@rgoers, is log4j 1.x vulnerable? |
remkop commentedDec 10, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@yuezk, as far as I can tell, log4j 1.x does not support lookups. Update (2021-12-11 09:09 JST): according tothis 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 tothis 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 isEnd of Life and hasother security vulnerabilities that will not be fixed. |
We need to look at the log4j 1 JMS Appender which I thought had at leastprogrammatic 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.htmlGary …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>.>> |
alphawoodexec left a comment
There was a problem hiding this 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
Restricts access to LDAP via JNDI.