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

Remove LOG4J from builds. #1972

Merged
merged 1 commit into from
Dec 13, 2021
Merged

Conversation

rgrunber
Copy link
Contributor

  • LOG4J does not appear to be used by any dependency

Signed-off-by: Roland Grunberg rgrunber@redhat.com

Prompted by GHSA-jfh8-c2jp-5v3q .

@fbricon @testforstephen , any idea why we kept this around. I was thinking for the underlying logging from m2e/buildship but neither Maven nor Gradle projects are having any issues logging verbosely without it.

If there is a good reason, we can keep it and apply the 1.x workaround, rgrunber/vscode-java@a163ee7 .

- LOG4J does not appear to be used by any dependency

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
@rgrunber rgrunber added build/infra dependencies Pull requests that update a dependency file labels Dec 11, 2021
@testforstephen
Copy link
Contributor

image

I use Dependency Analysis to search the dependents of org.apache.log4j, here are the results:

  • org.apache.ant
  • org.eclipse.xtext
  • org.eclipse.xtext.xbase

Does eclipse side have any plan to address the issue?

@rgrunber
Copy link
Contributor Author

rgrunber commented Dec 12, 2021

Neither of org.eclipse.xtext or org.eclipse.xtext.base are included in the JDT-LS runtime tarball. As for org.apache.ant, looking in the META-INF/MANIFEST.MF, under Import-Package, I see org.apache.log4j;resolution:=optional, so it should be safe to leave it out.

Update : You can also follow the thread in https://www.eclipse.org/lists/cross-project-issues-dev/2021/Dec/index.html on the topic. Though from what I can see, the Eclipse platform (eg. eclipse-SDK-4.22-linux-gtk-x86_64.tar.gz) and some higher profile simrel projects aren't vulnerable. There are some projects under the simrel repository that are vulnerable that are addressing it.

Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

Tried this PR with vscode-java-debug, vscode-java-test, vscode-pde together, no breaking was found. Also logback is logged normally.

LGTM.

@rgrunber rgrunber merged commit d28e26e into eclipse-jdtls:master Dec 13, 2021
@rgrunber rgrunber deleted the remove-log4j branch December 13, 2021 04:33
@rgrunber rgrunber added this to the Early December milestone Dec 13, 2021
@cormacrelf
Copy link

cormacrelf commented Dec 13, 2021

This is a good move regardless, but for completeness current releases of eclipse.jdt.ls only depend on and bundle log4j 1.x and 1.x is not vulnerable to the CVE-2021-44228. The GitHub advisory says all versions < 2.15.0 are affected, but it's technically only 2.0.0-beta9 onwards, according to https://logging.apache.org/log4j/2.x/security.html. This is a result of the lookups feature being a new addition in version 2.

The current release has a log4j jar in the plugins folder, but eclipse.jdt.ls/plugins/org.apache.log4j_1.2.15.v201012070815.jar is log4j version 1.2.15, i.e. 1.x. You can see how this would be confusing, especially as it is followed by a v2... string!

Getting rid of 1.x is nevertheless a good idea, because of e.g. CVE-2019-17571 and the fact that it hasn't been updated since 2015. But if you're auditing your systems for the new CVE, eclipse.jdt.ls should not be your highest priority.

Hope that helps anyone passing by.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/infra dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants