Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Cleaned-up log4j 1.2 that disables scary networking (trunk, binary-incompatible) #16

Closed
wants to merge 31 commits into from

Conversation

lsimons
Copy link

@lsimons lsimons commented Dec 17, 2021

As discussed on the dev@logging mailing list, this PR is targeting a log4j 1.2 security fix release. This PR combines code/documentation/build system changes.

The code PR is ready for review. What does everyone think?

The result is ready for user testing. Please try to build on your own environment and then please try the resulting jar file(s) in your applications.

Besides review/test/fixing there is one more obstacle to merging: as it stands apache/log4j is a read-only git-svn mirror. I am hoping the Logging PMC will consider a switch to git for log4j 1.x, so this PR can then actually be merged in and released. This is being discussing on the dev@logging mailing list.

* README.md: describe intent and situation
* LICENSE: update copyright year
* .gitignore: start a git ignore file
* INSTALL: note use of maven 3 / JDK 6 / toolchains.xml
* pom.xml: update some old metadata, use toolchain
Copy over missing 1.2 content from the HTML site.
Went through the pom.xml element by element and modernized each bit.

* all plugins to latest stable compatible version
* remove use of ant in favor of maven constructs
* clean up website content, fix some broken pages / links
* remove duplicate LICENSE/NOTICE files in favor of main files
* stop trying to build NT*.ddl from maven pom and use binaries
* update INSTALL file to point at binaries
* various small tweaks and cleanups

All tests pass including on windows with NT*.dll.

All goals I can test work (release & deploy untested).

All fixable build warnings fixed.
Disables:
* JMSAppender
* JMSSink
* SimpleSocketServer
* SocketHubAppender
* SocketServer

These classes now log an error instead of making network connections.

Their external APIs should remain intact to allow code using them to
migrate without blowing up before runtime, in many cases.

Added tests for the remaining behavior.
Disables
* SocketNode
* TelnetAppender

Adds tests for the remaining behavior.

Adds warnings
* for SyslogAppender if logging to a non-loopback address
* deprecation for SocketAppender since it's the client side of the
  disabled server code

Normalizes warnings in other net code to use LogLog.error()
consistently and removes internal use of log4j Logger objects.
Significant test suite cleanup/refactoring.

Change maven unit test config to run all tests instead of a whitelist.
Many more unit tests are now running.
Make maven clean up the unit test outputs.

Remove hopelessly broken tests:
* tests/src/java/org/apache/log4j/AsyncAppenderTestCase.java
* tests/src/java/org/apache/log4j/defaultInit/TestCase2.java
* tests/src/java/org/apache/log4j/defaultInit/TestCase3.java
* tests/src/java/org/apache/log4j/defaultInit/TestCase4.java
None of these were being run during the build.

Change many tests that read/write files to use unique filenames.
This avoids tests output leaking between tests.
The suite is now a bit less brittle.

Delete left-over SocketServer configs now that it is disabled.
@lsimons lsimons force-pushed the 2021-security-fixes branch 10 times, most recently from 165e537 to 04df5e0 Compare December 17, 2021 16:10
@tony--
Copy link

tony-- commented Dec 18, 2021

Thanks for all the effort @lsimons!
I have a couple questions.

  1. Is your objective to release a one-time security update in 1.2.18 that addresses the CVEs that are open against 1.2.17?
  2. I took a look at the CI you have set up and it looks like the build is working for JDK8 and up. As Log4j 2.12.2 was released to support Java 7, it seems that Java 7 support would be important in 1.2.18. Is it your intention to also support Java 7? I'm new to GitHub actions, so I'll take a look and see if there is some obvious reason why you are not already building for Java7, such as it not being supported in GitHub actions.
    Update: I see now that Java actions only natively supported 8+ and you are using jabba to grab zulu JDK6, so you are actually testing on 6, 8, 11, 17. Seems like it's necessary to add 7. Is 6 needed?

@lsimons
Copy link
Author

lsimons commented Dec 18, 2021

One time security fix

Thanks for all the effort @lsimons! I have a couple questions.

  1. Is your objective to release a one-time security update in 1.2.18 that addresses the CVEs that are open against 1.2.17?

Yes, basically. To be precise...

With this PR I want to make a one-time contribution that

  • addresses known security issues including open CVEs,
  • disables any/all other behavior considered 'obviously' risky from an unsupported logging library in 2021 sysadmin/ops terms (like opening server sockets or using JNDI),
  • is broadly backwards and forwards compatible, and
  • is easy to build and test from source in 2021.

From that contribution the Apache Logging PMC (which I am not on, I'm just a contributor) could then make an official release that I agree they should call 1.2.18, and that I think should be a one-time-only update.

Then if there's ever any further critical security vulnerabilities that someone wants to fix, having an easily buildable package would make it a bit easier. But I have no intention to do that kind of work.

Build: what's needed

  1. I took a look at the CI you have set up and it looks like the build is working for JDK8 and up. As Log4j 2.12.2 was released to support Java 7, it seems that Java 7 support would be important in 1.2.18. Is it your intention to also support Java 7? I'm new to GitHub actions, so I'll take a look and see if there is some obvious reason why you are not already building for Java7, such as it not being supported in GitHub actions.
    Update: I see now that Java actions only natively supported 8+ and you are using jabba to grab zulu JDK6, so you are actually testing on 6, 8, 11, 17. Seems like it's necessary to add 7. Is 6 needed?

I don't think we need to add 7. Whether 6 is strictly needed for compiling can be debated.

My intention:

  • a jar that works on JDK 1.4 and higher, just like 1.2.17, including 5/6/7/8/9/11/17
  • default build that uses JDK 6 for compiling, just like 1.2.17, for max backward/forward compatibility
  • alternative build that uses modern JDKs for compiling, for forward compatibility
  • use a relatively modern JDK for running a modern maven and its plugins, because many maven plugins require JDK 8 or higher
  • make sure latest JDKs also work for forward compatibility, so someone can rebuild from source next year

Build: details
It's bean rather messy to figure out the matrix of (in)compatibilities.

There's two build variants, using maven profiles. The default profile uses a maven toolchain so that maven and its plugins run using a modern JDK, while certain key plugins (like the compiler plugin, jar plugin, etc) run using a toolchain JDK.

The toolchain JDK is configured to be the venerable JDK 1.6, the same JDK that was used to produce 1.2.17, for maximum compatibility. That build is also configured to use source=1.4 and target=1.4, meaning the resulting jar is compatible with any java JDK or JRE >=1.4, including 5, 7, 8, 9, 11, 17. For this default build It should be possible to use a toolchain JDK >= 1.6 and < 9. JDK9 and later do not support source/target=1.4. But using additional toolchains doesn't really do anything: jars produced with JDK 6 work on later JDKs.

Testing with JDK 1.4 and JDK 5 needs to be done by hand, it's not really possible to automate on github actions, due to oracle's license restrictions.

The alternative no-toolchain profile doesn't use a toolchain setup. It runs maven and builds everything using a single JDK. This is configured to use source=7 and target=7, so that the resulting jar is compatible with any JDK or JRE >= 7...but it can't actually run using JDK 7, because many current maven plugins require JDK 8.

Build: building with JDK 7
Now it could be possible that using JDK 7 as (another) toolchain build works as well as JDK6, but that makes for a 3-dimensional matrix build which doesn't seem worth it.

Build: OMG it's complex!
Yeah. Thanks for reviewing it. The reason I kind of remember how to do this kind of setup is that I used to work on apache gump many years ago, which was the CI system used for log4j 1.2.

Suggested by Vladimir Sitnikov.
These were standalone server components with a main() method.
Having an error exit is no better than having a classloader error.
Not having the classes makes audit more straightforward.

Suggested by Vladimir Sitnikov and others.
This was a JMX HTTP server that was deprecated a loooong time ago.
Running such an agent from log4j is not a good idea.
If using SMTPS, warn that host will not be verified (CVE-2020-9488).
Note this commit does not actually fix the security issue. Instead it
warns an upgrade is needed (or a change in configuration).

If using SMTP, warn about an unencrypted protocol.

Does not warn about a possibly safer use, talking to the local loopback.
Disables
* JDBCAppender

Adds tests for the remaining behavior.
Disables
* ExternallyRolledFileAppender network code

Behavior reduces to be a normal non-rolling FileAppender.
@lsimons
Copy link
Author

lsimons commented Dec 18, 2021

Build: building with JDK 7
Now it could be possible that using JDK 7 as (another) toolchain build works as well as JDK6, but that makes for a 3-dimensional matrix build which doesn't seem worth it.

So....I added a single build for it.
Looks like it's working fine: https://github.com/lsimons/log4j/runs/4569682928?check_suite_focus=true

This replaces clirr that doesn't work with japicmp that does work.

Note pom.xml now contains a short list of minor incompatibilities
we choose to allow with their reason. This list will need maintenance
in case of further/future releases.
@lsimons lsimons changed the title WIP: cleaned-up log4j 1.2 that disables scary networking Cleaned-up log4j 1.2 that disables scary networking Dec 18, 2021
@lsimons
Copy link
Author

lsimons commented Dec 18, 2021

Github actions was used for basic compatibility testing, results can be seen at https://github.com/lsimons/log4j/actions , these all work:

  • Toolchain 1.6, JDK 11, OS ubuntu-18.04
  • Toolchain 1.6, JDK 11, OS ubuntu-20.04
  • Toolchain 1.6, JDK 11, OS windows-2019
  • Toolchain 1.6, JDK 11, OS windows-2022
  • Toolchain 1.7, JDK 11, OS ubuntu-18.04
  • JDK 8, OS ubuntu-18.04
  • JDK 11, OS ubuntu-18.04
  • JDK 17, OS ubuntu-18.04
  • JDK 8, OS ubuntu-20.04
  • JDK 11, OS ubuntu-20.04
  • JDK 17, OS ubuntu-20.04
  • JDK 8, OS windows-2019
  • JDK 11, OS windows-2019
  • JDK 17, OS windows-2019
  • JDK 8, OS windows-2022
  • JDK 11, OS windows-2022
  • JDK 17, OS windows-2022

As mentioned above, compatibility with many other JREs and JDKs is also expected. In particular the toolchain build produces source/target 1.4 artifacts, so testing with 1.4 and 5 can be done.

@lsimons lsimons marked this pull request as ready for review December 18, 2021 18:26
@lsimons lsimons marked this pull request as draft December 19, 2021 10:13
@lsimons lsimons changed the title Cleaned-up log4j 1.2 that disables scary networking Cleaned-up log4j 1.2 that disables scary networking (trunk, binary-incompatible) Dec 19, 2021
@lsimons
Copy link
Author

lsimons commented Dec 19, 2021

I will close this PR in favor of the new PR #17 .

That new PR is fully source/binary compatible with 1.2.17, which was requested on the dev@logging mailing list.

It's probably a better basis to start from for an official security fix release.

(Of course feel free to take this PR instead for a trunk-development-based approach, it offers roughly equivalent security improvements.)

@lsimons lsimons closed this Dec 19, 2021
@lsimons
Copy link
Author

lsimons commented Jan 20, 2022

See

for a maintained/released fork with similar security fixes.

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

Successfully merging this pull request may close these issues.

5 participants