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

CDI-727 CDI.current() should use privileged block #391

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

antoinesd
Copy link
Contributor

No description provided.

@antoinesd
Copy link
Contributor Author

I was unable to recreate a similar error than in the ticket. We may consider adding a small jar with a fake CDI provider to be in the same condition.
I had to create another SecurityActions class for the api package: not very happy with that: suggestions are welcome

@mkouba
Copy link
Contributor

mkouba commented Jun 27, 2018

I don't think that ServiceLoader.load() requires a privileged block. See also my comment: https://issues.jboss.org/browse/CDI-727?focusedCommentId=13597453&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13597453

I had to create another SecurityActions class for the api package.

No problem. That's a common practice.

@antoinesd
Copy link
Contributor Author

Yes, good point. I forgot that we switched to service loader from 1.2 to 2.0

@manovotn
Copy link
Contributor

I had to create another SecurityActions class for the api package.

No problem. That's a common practice.

Hmm, just genuinely interested - why is it common practice? Just to prevent anyone using it? I thought these SM calls are basically caller-sensitive. So even if it was used by someone else, it shouldn't cause harm. But I guess I am missing something here?

@mkouba
Copy link
Contributor

mkouba commented Jun 27, 2018

@manovotn If you make the SecurityActions public then anyone can use it and thus "hijack" the permissions assigned to Weld.

@manovotn
Copy link
Contributor

Hmm that's exactly what I thought would be addressed by caller sensitivity, apparently I was wrong. Thanks for explanation.

@mkouba
Copy link
Contributor

mkouba commented Jun 27, 2018

@manovotn The point is that a "privileged" caller basically removes the callers up in the stack from the set of callers that will be checked when making access control decisions. Weld's SecurityActions is a privileged caller in this case. So if SecurityActions was public and Foo invoked SecurityActions.getDeclaredMethods() then Foo does not have to have the relevant permissions -> security leak.

Whenever a resource access is attempted, all code traversed by the execution thread up to that point must have permission for that resource access, unless some code on the thread has been marked as "privileged".

See also https://docs.oracle.com/javase/7/docs/technotes/guides/security/doprivileged.html

@thaarok
Copy link

thaarok commented Jun 27, 2018

@antoinesd I just tested this with EntityListenerBeanManagerInjectionTestCase in wildfly testsuite (with WFLY-10125 workaround removed and set to use cdi-api:2.1-SNAPSHOT) and I confirm this fixes the bug. 👍

@antoinesd
Copy link
Contributor Author

Thanks @hkalina. Do you have a suggestion to create a simple test to reproduce your issue? Mine is obviously too basic since it doesn't trigger any AccessControlException without privileged code bloc.

@thaarok
Copy link

thaarok commented Jun 27, 2018

@antoinesd are you sure the test is running with security manager enabled? (is System.getSecurityManager() not null?)

@antoinesd antoinesd merged commit ce2e73c into jakartaee:master Jul 19, 2018
@antoinesd antoinesd deleted the CDI-727 branch July 23, 2018 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants