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

Patch log4j JAR to remove JndiLookup class #81629

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

mark-vieira
Copy link
Contributor

@mark-vieira mark-vieira commented Dec 10, 2021

We've all heard about the log4j vulnerability by now. We have other ways to mitigate this issue but since we can't completely block usages of this via SecurityManager, just blow this thing away altogether.

We could also upgrade to log4j 2.15.0 but there are compatibility issues with that.

@mark-vieira mark-vieira added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >refactoring labels Dec 10, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Dec 10, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@mark-vieira
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-1

@jayaddison
Copy link

@mark-vieira are you able to share the compatibility issue with log4j 2.15.0? (I didn't see any breaking changes in the manually-prepared version of their changelog, and understand there may be build issues in practice)

@gruselglatz
Copy link

Which Versions will get this Patch backported?

@@ -275,6 +275,10 @@ configure(subprojects.findAll { ['archives', 'packages'].contains(it.name) }) {
}
}
}
all {
resolutionStrategy.dependencySubstitution {
substitute module("org.apache.logging.log4j:log4j-core") using project(":libs:elasticsearch-log4j") because "patched to remove JndiLookup clas"}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a small typo in clas.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be a good idea to put the } on a separate line for readability.

Copy link

Choose a reason for hiding this comment

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

Also, clas -> class (sorry for my OCD)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I was slightly... rushed 😉

@arteam arteam added the :Core/Infra/Logging Log management and logging utilities label Dec 13, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Dec 13, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@mark-vieira
Copy link
Contributor Author

@mark-vieira are you able to share the compatibility issue with log4j 2.15.0? (I didn't see any breaking changes in the manually-prepared version of their changelog, and understand there may be build issues in practice)

I was informed by a teammate it would involve other code changes in Elasticsearch. I don't have the details right now but at the time given we wanted to get a fix out as quickly as possible we went the route of just patching the JAR. Our intention is to remove this in lieu of an updated log4j version eventually though.

@jayaddison
Copy link

Thanks @mark-vieira for your time to respond (not to mention for the fix itself!). Completely understood re: the urgency of preparing a patched release.

@rayzhangcl
Copy link

hi team are we still trying to bump the log4j version as a permanent fix?

@mark-vieira
Copy link
Contributor Author

hi team are we still trying to bump the log4j version as a permanent fix?

Yes, we still intend to upgrade log4j as a permanent solution here.

@mtn217
Copy link

mtn217 commented Dec 13, 2021

@mark-vieira, do you have an ETA for when the log4j version update change will be released?

@mark-vieira
Copy link
Contributor Author

No specific ETA at the moment but this is high priority for us.

@hanusto
Copy link

hanusto commented Dec 14, 2021

Do you plan to backport to 7.12.x version please? Because it is for example still supported by Spring Boot platform 2.5.x:

Thanks

@klimchuk
Copy link

There is log4j 2.16.0 already

@Xon
Copy link

Xon commented Dec 14, 2021

Removing the JndiLookup class looks to have migrated CVE-2021-45046

@hanusto
Copy link

hanusto commented Dec 14, 2021

Yes, but it looks like still vulnerable: https://twitter.com/cyb3rops/status/1470791578046406657?s=21.

@mark-vieira
Copy link
Contributor Author

mark-vieira commented Dec 14, 2021

Do you plan to backport to 7.12.x version please? Because it is for example still supported by Spring Boot platform 2.5.x:

@hanusto The Elasticsearch dependency that Spring Boot brings in does not include a log4j implementation by default as it's an optional dependency. If you are using log4j-core in your project I suggest you follow the instructions provided by the Spring team to upgrade instead.

@hanusto
Copy link

hanusto commented Dec 14, 2021

@mark-vieira but what I am trying to say is that Spring Boot supports the version of ES and the version of ES is vulnerable. So it is not about ES client used by SB but only about statement that SB app should be connected to vulnerable ES. If you will use newer version of ES, SB will say that there is some incompatibility.

@mark-vieira
Copy link
Contributor Author

mark-vieira commented Dec 14, 2021

Spring Boot supports the version of ES and the version of ES is vulnerable

Understood. We don't intend to backport fixes to earlier unsupported versions of Elasticsearch. For Elasticseaarch 7.12 you are not vulnerable if running with Java 9 or later due to the behavior of the Java Security Manager. Alternatively you can add -Dlog4j2.formatMsgNoLookups=true to your jvm.options file.

@ZhenTu
Copy link

ZhenTu commented Dec 15, 2021

Xray scanning reports 7.16.1 still has log4j-core in elasticsearch-sql-cli.jar, which is located in /usr/share/elasticsearch/bin folder.

~/bin$ ls
elasticsearch elasticsearch-croneval elasticsearch-keystore elasticsearch-saml-metadata elasticsearch-sql-cli x-pack-env
elasticsearch-certgen elasticsearch-env elasticsearch-migrate elasticsearch-service-tokens elasticsearch-sql-cli-7.16.1.jar x-pack-security-env
elasticsearch-certutil elasticsearch-env-from-file elasticsearch-node elasticsearch-setup-passwords elasticsearch-syskeygen x-pack-watcher-env
elasticsearch-cli elasticsearch-geoip elasticsearch-plugin elasticsearch-shard elasticsearch-users

arteam added a commit to arteam/elasticsearch that referenced this pull request Dec 15, 2021
@arteam arteam mentioned this pull request Dec 15, 2021
@mark-vieira
Copy link
Contributor Author

Xray scanning reports 7.16.1 still has log4j-core in elasticsearch-sql-cli.jar, which is located in /usr/share/elasticsearch/bin folder.

That is correct. We determined this wasn't an issue given that JAR isn't on the Elasticsearch server classpath. This is a "fatjar" bundled to be used as a CLI tool so is not susceptible to RCE.

arteam added a commit to arteam/elasticsearch that referenced this pull request Dec 15, 2021
@m-31
Copy link

m-31 commented Dec 16, 2021

We installed logstash-7.16.1-1.x86_64 rpm on a Amazon Linux release 2 (Karoo) from https://artifacts.elastic.co/packages/7.x/yum and in /usr/share/logstash we executed

find . \( -type f -name '*.jar' -o -name '*.war' \) -print0 |  xargs -0 -I '{}' sh -c "jar tf {} | grep -i JndiLookup &&  echo {}"

and got

./logstash-core/lib/jars/log4j-core-2.15.0.jar
org/apache/logging/log4j/core/lookup/JndiLookup.class
./vendor/bundle/jruby/2.5.0/gems/logstash-input-tcp-6.2.3-java/vendor/jar-dependencies/org/logstash/inputs/logstash-input-tcp/6.2.3/logstash-input-tcp-6.2.3.jar
org/apache/logging/log4j/core/lookup/JndiLookup.class

Are you really sure that the JndiLookup class is removed in 7.16.1 and https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-45046 is resolved???

@mark-vieira
Copy link
Contributor Author

The class file was only removed for Elasticsearch, not Logstash given that Logstash simply upgraded to Log4j to mitigate 2.15.0. It looks like Logstash has already already upgraded to Log4j 2.16.0 which further mitigates the CVE you mention and that should be available in the next release.

@pugnascotia
Copy link
Contributor

@Marv1995 please see the earlier comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >refactoring Team:Core/Infra Meta label for core/infra team Team:Delivery Meta label for Delivery team v6.8.21 v7.16.1 v8.0.0-rc2 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.