-
Notifications
You must be signed in to change notification settings - Fork 680
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
Log4j Critical Exploit - Play1 possibly unaffected #1367
Comments
The log4j version currently used is 1.2.17, but this is also a really outdated version from 2012. Please share if someone has opinions about if this issue can be exploited in Play1. Maybe Play1 soon has to be upgraded to take use of 2.15.0 version of Log4j? |
Env vars are not interpolated in log4j 1.x. Any vars you want interpolated need to be set with |
@cies Could you please elaborate a bit? I did not understand what you meant by the '-D' flags. Is none of the two mentioned exploits above relevant for Play 1.x or do we have to set some flags to be safe? |
Here's some further information on whether log4j v1 is affected: apache/logging-log4j2#608 (comment) As of right now it only appears to be affected if we are using the JMS appender, which I don't think we are? @cies I don't think the worry here is with env vars, it's with logging input from URL params and things like that. Unless I misunderstood your comment? And a separate discussion is whether we should try to move to 2.15.0 due to other disclosed vulnerabilities in 1.x. |
All, |
OK, I have log4j2 v 2.17.0 working in a dev environment without any code changes to play. It required the following changes:
The log4j-1.2-api jar is a "compatibility bridge" that appears to map v1 calls/classes to v2. Without it, there are compile errors. With it, initial testing appears to work fine. The loading of properties using PropertyConfigurator.configure (called in play Logger.java init() ) was deprecated. As a result, we do lose the ability to specify config files in application.conf using "application.log.path". Thus the need for the logj2.xml file in the conf directory. There is a “log4j1.compatibility” settings in log4j2 that is supposed to allow reading of log4j v1's config files. I was not able to make this work and just created the log4j2.xml config file which seems to be OK. I searched the log4j2 code for use of this “log4j1.compatibility” flag and I could not find any place where it may be used to impact actual code logic (i.e. it appears to ONLY be used to config log4j2 using your old config files) A proper migration to log4j2 should:
Hope this is helpful to others. UPDATE: The following appears to continue useful code for loading config files programmatically in log4j2: UPDATE (1/6/2022): The following code fails in test mode when using log4j2 |
Hi and thanks for all the reactions and contributions here ! The situation is still a bit complex and my opinion would be not to hurry for a 2.x migration, because :
Therefore I am not arguing against migrating, and this PR is a good preparation for Log4j 2.x migration which will benefit for any version of Log4j 2.x, but maybe waiting a few more weeks would be a smart choice before releasing a new version. |
Just came across a new option, a drop in replacement for log4j 1.2.17 with the exploits present in log4j v1 fixed: https://reload4j.qos.ch I believe it was created by one of the original log4j authors. This might be the quickest and least disruptive fix. I took a look at the two CVEs that are fixed and (as far as I understand it) one requires access to change the configuration, and the other only affects configurations that listen over the network via the SocketServer class. So I'm not sure how common either will be in the wild, but if the fix is a drop in then probably worth it. |
@megamattron Sure, reload4j is the best way to upgrade log4j in Play framework. P.S. Yes, those security issues found in log4j1 are not the same as Log4Shell (which is urgently critical issue). |
@asolntsev this is not the best way to improve, but the simplest way, probably not changing anything in the code but the library. the question is whether log4j1/reload4j has no more vulnerabilities since it has been dead since 2015 I would choose something new log4j2 or logback |
The RePlay project has upgraded to The |
My vote is for reload4j |
I've gone ahead and updated my JDK 17 compatible Play1 pull request with reload4j and all tests passed. I'll do some further app testing but it's a good start and can't beat drop in replacement |
my vote is for log4j2 or logback @tazmaniax I am more sympathetic to log4j2, as this library has been screened probably the most recently in terms of security |
@jacol84 I'm certainly not opposed to log4j2 and that might indeed be the better strategic decision. Your points on log4j2's recent security screening and further advancement are definitely valid. The only reason I settled for reload4j now for my specific build was it seemed to be the more incremental step and to @cies 's point there have been significant amount of effort since 6th January to ensure it is robust and secure, see https://reload4j.qos.ch/news.html. Moving forward, and not sure how this can be done, it would be good to get some more preferences from within the community coupled with a robust set of pros & cons between the available options. In an ideal world Play1 would be flexible enough to support all options with minimal configuration so it can be decided on a per application basis. |
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
- fix rebase conflict
FWIW: We would prefer log4j2 |
- fix rebase conflict
playframework#1367 fix CRLF to LF update slf4j-api-1.7.35.jar add solving the problem when log4j fails to use automatic configuration
fixed test i-am-a-developer
#1367 change log4j-1.2.17 to log4j-2.17.1
Caveat: Play 1.7.1 seems to expect a v2 formatted file, while using what seems the v1 filename (log4j.properties, not log4j2.properties as per the v2 spec). In short: using a v2-formatted file in the location |
they actually use such a standard log4j2.(yml|json|properties|xml|jsn) but I didn't see this change in migration when I followed the commands from this page When I have a moment, I will try to improve it |
A Log4j critical exploit was made public, here's a good overview: https://www.lunasec.io/docs/blog/log4j-zero-day/ I believe the minimum version number where the exploit is functional is 2.0 and I believe play1 is on 1.2.17, so it shouldn't be a problem. Due to the severity of the issue I thought it best to post here and get another opinion just in case.
The text was updated successfully, but these errors were encountered: