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

Updating the version of log4j2 #47298

Closed

Conversation

pgomulka
Copy link
Contributor

Updating to latest 2.12.1

closes #45523

Updating to latest 2.12.1

closes elastic#45523
@pgomulka pgomulka added :Core/Infra/Logging Log management and logging utilities v8.0.0 v7.5.0 labels Sep 30, 2019
@pgomulka pgomulka self-assigned this Sep 30, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka
Copy link
Contributor Author

pgomulka commented Oct 2, 2019

I am facing some problems with security permission with this log4j2 version
It appears that it now requires getClassLoader permission
this was introduced in this commit https://gitbox.apache.org/repos/asf?p=logging-log4j2.git;a=commitdiff;h=dfced48;hp=a15b58450c4afcb9a7604308e0d3d81872226953

the discussion https://jira.apache.org/jira/browse/LOG4J2-2266

the test that is failing

java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "getClassLoader")

    at __randomizedtesting.SeedInfo.seed([E11D7EE195541C22:C13E3F81DC84F92D]:0)
    at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
    at java.base/java.security.AccessController.checkPermission(AccessController.java:1044)
    at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:408)
    at java.base/java.lang.ClassLoader.checkClassLoaderPermission(ClassLoader.java:2048)
    at java.base/java.lang.ClassLoader.getParent(ClassLoader.java:1805)
    at org.apache.logging.log4j.util.LoaderUtil.getClassLoaders(LoaderUtil.java:121)
    at org.apache.logging.log4j.util.PropertiesUtil$Environment.<init>(PropertiesUtil.java:334)
    at org.apache.logging.log4j.util.PropertiesUtil$Environment.<init>(PropertiesUtil.java:312)
    at org.apache.logging.log4j.util.PropertiesUtil.<init>(PropertiesUtil.java:71)
    at org.apache.logging.log4j.simple.SimpleLoggerContext.<init>(SimpleLoggerContext.java:70)
    at org.elasticsearch.common.logging.DeprecationLoggerTests$1.<init>(DeprecationLoggerTests.java:337)
    at org.elasticsearch.common.logging.DeprecationLoggerTests.testLogPermissions(DeprecationLoggerTests.java:337)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at 

@pugnascotia Should we add a security grant for log4j to allow this?
Oddly I only see this failing in test. When running a server I don't see this problem

@pugnascotia
Copy link
Contributor

Let's ask @rjernst.

@rjernst
Copy link
Member

rjernst commented Oct 10, 2019

Logging is initialized before we set the security manager, which is why this does not fail in production. I think the permission could be avoided if we mock or subclass the LoggerContext interface in the test instead of extending SimpleLoggerContext?

Separately, this looks like a regression in log4j, as there is special handling there to detect security manager and do best effort when it does not allow certain permissions. The problem here is the loop traversing the hierarchy of classloaders. The calls to getParent() should be protected in LoaderUtil.getClassLoaders() so that if a security manager is installed without this permission, only the classloader of LoaderUtil gets returned. We should at least report this issue back to log4j.

@pgomulka
Copy link
Contributor Author

pgomulka commented Nov 26, 2020

I suspect the problem with getClassLoader and security permissions we were facing should be fixed by apache/logging-log4j2@fbde9cd

I am getting a lot of thirdpartyaudit problems though..

edit: the fix is actually scheduled for version 3.0.

@pgomulka
Copy link
Contributor Author

@rjernst

Logging is initialized before we set the security manager, which is why this does not fail in production. I think the permission could be avoided if we mock or subclass the LoggerContext interface in the test instead of extending SimpleLoggerContext?

But is this correct? if we initialise logging before security manager, then some of the logging static settings will be incorrect. Like with class loading. See here in old 2.11 version - the same logic is in latest log4j version
https://github.com/apache/logging-log4j2/blob/rel/2.11.1/log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java#L59

@pgomulka
Copy link
Contributor Author

awaits a fix within log4j

@L3P3
Copy link

L3P3 commented Dec 12, 2021

We need another quick update to patch Elastic against the new #log4shell hack vector!

@nik9000
Copy link
Member

nik9000 commented Dec 12, 2021 via email

arteam pushed a commit to arteam/elasticsearch that referenced this pull request Dec 14, 2021
Originally we tried to a log4j update in elastic#47298, but we were unable to
that due to the `DeprecationLoggerTests.testLogPermissions` test
failing. The test relied on mocking and got removed in
https://github.com/elastic/elasticsearch/pull/61474/files#diff-70de5a6ba5c637e7f19c51341417760d6e957beb5a1fa5703049095ea2719ee0L47

Now we should be able to the upgrade and then we can address the Security
Manager permission questions raised in elastic#47298 separately.
@pgomulka
Copy link
Contributor Author

pgomulka commented Dec 14, 2021

We discussed this with @ChrisHegarty and @arteam
We have analysed the use of LoaderUtil and how it differs in between versions 2.11 and 2.16. Even though we do not agree with how the classloader hierarchy is traversed we think it is safe to use it.

The failing DeprecationLoggerTests.testLogPermissions is removed in latest versions, I raised an issue to analyse if we still need it #81708

@arteam arteam mentioned this pull request Dec 14, 2021
@ChrisHegarty
Copy link
Contributor

We discussed this with @ChrisHegarty and @arteam
We have analysed the use of LoaderUtil and how it differs in between versions 2.11 and 2.16. Even though we do not agree with how the classloader hierarchy is traversed we think it is safe to use it.

Agreed. From the perspective of LoaderUtil, there are no substantive differences relating to how permissions are determined and checked. The code does not take into account the dynamic nature of the security manager, and permission stack walking, but that is not new in 2.15, it's there in prior versions too. We'll proceed with the upgrade, and circle back to the aforementioned issue in log4j at some future point.

arteam added a commit that referenced this pull request Dec 15, 2021
Originally we tried to a log4j update in #47298, but we were unable to
that due to the `DeprecationLoggerTests.testLogPermissions` test
failing. The test relied on mocking and got removed in
https://github.com/elastic/elasticsearch/pull/61474/files#diff-70de5a6ba5c637e7f19c51341417760d6e957beb5a1fa5703049095ea2719ee0L47

Now we should be able to the upgrade and then we can address the Security
Manager permission questions raised in #47298 separately.

* Initialize pattern layout with AccessController.doPrivileged

We need the `getClassLoader` permissions

* Disable the SecurityManager for command testing because of `CommandLoggingConfigurator` 
which fails under the `SecurityManager`
@xeraa xeraa mentioned this pull request Dec 16, 2021
@pgomulka
Copy link
Contributor Author

closing as log4j was updated to 2.17

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 stalled Team:Core/Infra Meta label for core/infra team v8.0.0-rc1 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider upgrading log4j2 to 2.12.1