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 2022/08/20 10:21:48 UTC
[GitHub] [logging-log4j2] boris-unckel opened a new pull request, #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
boris-unckel opened a new pull request, #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006
Fixes https://issues.apache.org/jira/browse/LOG4J2-3579
--
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] boris-unckel commented on pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on PR #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006#issuecomment-1222628283
I'm closing this PR for the reasons described here and as discussed on the mailing list. Please check https://github.com/apache/logging-log4j2/pull/1007 for the ongoing review.
--
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] boris-unckel commented on pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on PR #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006#issuecomment-1221502184
@ppkarwasz
> Since `AccessController#doPrivileged` is another caller sensitive method, I think that your PR should do the same and call it through `MethodHandles.Lookup`.
Thanks for your feedback. I have changed this accordingly.
I have serious doubts the current fix is a good idea in general. Obviously addressing Java 8 and Java 11 consumers the SecurityManager is a must have, causing a regression between 2.17.2 and 2.18. I would expect a major version jump indicating a "no support" situation including explicit documentation. But:
The class is available to all the user land: It makes it possible to run code (service loading) with privileged rights (in terms of WildFly: server side - "AllPermission"). Now "all world" can load any Service (even remote?). I have not tested it yet, but I would assume it is better to fix it at all current usage points of the public methods. Then only fixed Services can be loaded privileged, but not any. (For WildFly it would be only for classes visible for the log4j2 module 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] boris-unckel commented on pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on PR #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006#issuecomment-1221529876
@ppkarwasz I have prepared a caller based solution: https://github.com/apache/logging-log4j2/pull/1007 I think it's better to avoid a general purpose privileged execution utility.
--
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] ppkarwasz commented on pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006#issuecomment-1221390120
Hi @boris-unckel, thank You for noticing and solving this `SecurityManager` problem.
The idea behind the `ServiceLoaderUtil` is to work around the caller sensitivity of the `ServiceLoader` methods by using the `MethodHandles.Lookup` parameter to create a lambda in the caller's domain.
Since `AccessController#doPrivileged` is another caller sensitive method, I think that your PR should do the same and call it through `MethodHandles.Lookup`.
--
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] ppkarwasz commented on pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006#issuecomment-1221536060
@boris-unckel, I totally share your concerns about the usage of `AccessController.doPrivileged`, but I am not sure if #1007 solves them.
If we were to introduce additional `AccessController.doPrivileged`, we need to audit our code to make sure the services we obtain through privileged calls can not be abused by unprivileged callers. E.g. I think that the `SystemPropertiesPropertySource` class can be used by anyone to obtain the value of privileged system properties, e.g. "log4j2.keystorePassword".
I started a thread (cf. [https://lists.apache.org/thread/2j0dyccsm0ddjy60ydd3f19flotjfjxd](https://lists.apache.org/thread/2j0dyccsm0ddjy60ydd3f19flotjfjxd)) on our mailing list to discuss the problem. Personally I almost never use a `SecurityManager` in my day job and I suppose this also applies to most of the team: it would be nice if you could expose your ideas on the mailing list.
--
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] jamezp commented on pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
jamezp commented on PR #1006:
URL: https://github.com/apache/logging-log4j2/pull/1006#issuecomment-1222467901
I'm a +1 for the second (#1007) approach compared to this. It is more of a pain to ensure it's always used in a privileged action, but it's definitely more secure. There may be cases where an implementation or user might *not* want it privileged.
WRT the `SecurityManager` as a whole. Yes it's deprecated in Java 17 and proposed for removal. However, there are some users that are required to use a `SecurityManager` for compliance reasons.
--
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] boris-unckel closed pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
boris-unckel closed pull request #1006: LOG4J2-3579 Add privileged execution for ServiceLoading
URL: https://github.com/apache/logging-log4j2/pull/1006
--
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