Skip to content

Conversation

@tobias-trabelsi-sonarsource
Copy link

@tobias-trabelsi-sonarsource tobias-trabelsi-sonarsource commented Nov 24, 2020

Hello there 👋

it looks like we have a common CVE justification for the DoD, so maybe we could update log4j to fix it. Looks like you already justified it here but this might be a quick win :)

Also there is another PR that seems stale for ~ a year now that also wanted to update log4j.

We at SonarSource would prefer if this gets backported to the 7.x branch as well

This would fix GHSA-vwqq-5vrc-xw9h as well as #45523

@matriv matriv added the :Core/Infra/Logging Log management and logging utilities label Nov 25, 2020
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Nov 25, 2020
@elasticmachine
Copy link
Collaborator

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

@pgomulka pgomulka self-assigned this Nov 26, 2020
@pgomulka
Copy link
Contributor

ok to test

@pgomulka
Copy link
Contributor

@tobias-trabelsi-sonarsource can you try running ./gradlew precommit locallly and update the PR?
feel free to try with latest 2.14.0

@tobias-trabelsi-sonarsource
Copy link
Author

tobias-trabelsi-sonarsource commented Nov 26, 2020

Hi @pgomulka
i tried again with 2.14.0 but got the same error as jenkins. i am not very familiar with the gradle plugin that you are using here and can not find any information about this specific error. can you give me some hints on where to look to make the tests pass?

currently there is a bunch of forbidden APIs and one missing class: javax.jms.Message

EDIT: reverted back to 2.13.2 as it looks like there was a breaking change in there ( see CI run )

@pgomulka
Copy link
Contributor

@tobias-trabelsi-sonarsource you made the right changes, thank you.
the CI is failing now because log4j is being configured before the security manager is being set.
log4j thinks it can use getClassLoader LoaderUtil:57 but in fact, we later set security manager to prevent that. hence the errors.
I will take a look into this..

@pgomulka
Copy link
Contributor

pgomulka commented Dec 1, 2020

this is what I found out so far about this failure.

The test fails because we set the securityManager (ESTestCase:232) and then try to create log4j PatternLayout which in log4j 2.13.2 is using getClassLoader (see stacktrace). That classloading log4j behaviour was introduced in https://issues.apache.org/jira/browse/LOG4J2-2779

This do not fail in production as we initialise our testing before security manager.

if we could set security after the Patternlayout from the test is created, it should work I think..
sth like
LoggingAuditTrailTests#lookupPatternLayout

patternLayout = PatternLayout.newBuilder().withPattern(patternLayoutFormat).withCharset(StandardCharsets.UTF_8).build();
        BootstrapForTesting.ensureInitialized();

or even move these log4j tests to evil logging test module and disable security manager there?

can we set a tests.security.manager flag per test?
@rjernst do you think any of this would work? Do you think there is a different workaround?

@rjernst
Copy link
Member

rjernst commented Dec 1, 2020

I think this needs to be fixed upstream in log4j. There are a few problems in LoaderUtil.getClassLoaders(). First, it tries to check for the context classloader being aware of the security manager. But it determines whether a security manager exists statically when the class is initialized. Second, it tries to traverse the context classloader hierarchy, as well as get the system class loader, without accounting for security manager.

The only workaround would be patching the LoaderUtil class at build time, but we should try to fix this upstream before considering that as an interim solution.

@tobias-trabelsi-sonarsource
Copy link
Author

Urgh this is a little bit out of my knowledge area. Will any of you look into the log4j codebase to propose the fix upstream? Should i leave this PR open until this is patched or should i close the PR?

@pgomulka
Copy link
Contributor

pgomulka commented Dec 3, 2020

@tobias-trabelsi-sonarsource I will try to raise a log4j issue and possibly work on a fix there.
I suspect if it will be released it would be latest version, so we would have to add substantially to this PR.
I suppose we can close this one in favour of #47298
I will ping you once we make more progress

@tobias-trabelsi-sonarsource
Copy link
Author

Okay then i will close this one for now. Good luck on your journey :)

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 Team:Core/Infra Meta label for core/infra team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants