You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/12/22 18:25:31 UTC
[GitHub] [logging-log4j2] riven8192 opened a new pull request #649: Remove requirement of JndiLookup class to exist
riven8192 opened a new pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649
To address the log4j jndi-vulnerability, many companies have removed JndiLookup.class from the artifact, whether or not they upgraded log4j2. This is a safeguard against the potential of newly discovered routes to JndiLookup, which are not yet patched. We would rather remove the JndiLookup alltogether, to take away the core issue: that a logging-api should not allow code-execution, not matter how each patch tries to limit access to this class.
In 2.17.0 the check for the JndiLookup class is based on a FQCN: "org.apache.logging.log4j.core.lookup.JndiLookup"
However, in the master-branch, we see a dependency on: JndiLookup.class.getName(), which in the case of a removed class-file, would fail the loading of *any* interpolator-plugin, as all checks will cause an exception, and handleError to be executed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999789007
Now that we have provides releases for all Java versions that ever saw a Log4j 2 release, I don't think our code should account for mangled jar files.
- Java 8 -> 2.17.0
- Java 7 -> 2.12.3
- Java 6 -> 2.3.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 edited a comment on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 edited a comment on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999896230
Thanks for your reply. With repackaging I didn't mean a refactor on the log4j side, but for example a 3rd party library like a google client-api, that renames:
`org.apache.logging.log4j.core.lookup.JndiLookup`
to
`com.google.apis.oauth2.repackaged.org.apache.logging.log4j.core.lookup.JndiLookup`
breaking the effectiveness of the patch, leaving the service/server vulnerable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 edited a comment on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 edited a comment on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999896230
Thanks for your reply. With repackaging I didn't mean a refactor on the log4j side, but for example a 3rd party library like a google client-api, that renames:
`org.apache.logging.log4j.core.lookup.JndiLookup`
to
`com.google.api.client.oauth2.repackaged.org.apache.logging.log4j.core.lookup.JndiLookup`
breaking the effectiveness of the patch, leaving the service/server vulnerable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999785941
@garydgregory The 2.17.0 patch does not factor in a safeguard for repackaged dependencies (where the java package-names are changed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999896230
Thanks for your reply. With repackaging I didn't mean a refactor on the log4j side, but for example a 3rd party library like a google client-api, that renames:
`org.apache.logging.log4j.core.lookup.JndiLookup`
to
`com.google.apis.oauth2.repackaged.org.apache.logging.log4j.core.lookup.JndiLookup`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999784891
This has already been handled in 2.17.0. Note that PRs should be created against the release-2.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 closed pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 closed pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers edited a comment on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
rgoers edited a comment on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999887585
We were in such a hurry to get patches out that nothing was brought back to the master (3.0) branch. I am actually in the process right now of moving the JNDI pieces to their own log4j-jndi module, which is in keeping with the more modular approach we are trying to take with 3.0. Because the Interpolator is in log4j-core it will have to take a slightly different approach to determining whether JndiLookup should be included. I'm not sure I see the value of changing it in the release-2.x branch. We have no plans to move the class anywhere else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999908082
Thanks @carterkozak - that indeed ensures security in the case I described.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999900777
@riven8192 the repackaging plugins I'm aware of also match string constants and rewrite those when they match fully qualified class names. I believe that would work correctly with the implementation on release-2.x, however not all repacking scripts update strings, in which case we'd end up logging a warning to the StatusLogger in that codepath.
> breaking the effectiveness of the patch, leaving the service/server vulnerable.
I'm not sure that's entirely correct -- `JndiLookup` constructor checks the enablement property itself, and throws if jndi lookups haven't been explicitly turned on:
https://github.com/apache/logging-log4j2/blob/a19ef9bceeaad862cfc0b50394a7f791d5e17b8c/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/JndiLookup.java#L46-L50
This would cause a warning to be logged here:
https://github.com/apache/logging-log4j2/blob/a19ef9bceeaad862cfc0b50394a7f791d5e17b8c/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java#L78-L87
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] riven8192 commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
riven8192 commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999883312
Thanks for the information regarding the branches @garydgregory.
I will not (yet) make PRs for those branches, as it seems it'd be preferable to agree on the underlying principles.
I quite regularly see jars with repackaged dependencies, simply because it reduces the problems you can face when multiple projects/dependencies require different versions of dependencies on the classpath. I actually encountered repackaged dependencies in several Google client-APIs. It would be rather unfortunate if people would effectively lose the security of the latest patches, because they turn out to have repackaged classes on the classpath.
I agree with you that in principle log4j should not support artifact-mangling, but given the potential security implications I think it's strongly recommended to replace:
`clazz.getName().equals("org.apache.logging.log4j.core.lookup.JndiLookup")`
with:
`clazz.getName().contains(".log4j.") && clazz.getName().endsWith(".JndiLookup")`
like proposed in the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-1000017158
> We were in such a hurry to get patches out that nothing was brought back to the master (3.0) branch. I am actually in the process right now of moving the JNDI pieces to their own log4j-jndi module, which is in keeping with the more modular approach we are trying to take with 3.0. Because the Interpolator is in log4j-core it will have to take a slightly different approach to determining whether JndiLookup should be included. I'm not sure I see the value of changing it in the release-2.x branch. We have no plans to move the class anywhere else.
This could be a job well suite to a service loader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999887585
We were in such a hurry to get patches out that nothing was brought back to the master (3.0) branch. I am actually in the process right now of moving the JNDI pieces to their own log4j-jndi module, which is in keeping with the more modular approach we are trying to take with 3.0. Because the Interpolator is in log4j-core it will have to take a slightly different approach to determining whether JndiLookup should be included.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #649: Remove requirement of JndiLookup class to exist
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #649:
URL: https://github.com/apache/logging-log4j2/pull/649#issuecomment-999792045
You might not have noticed the master branch is labeled 3.0.0-SNAPSHOT. The branch for 2.x is called release-2.x.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org