Skip to content

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

Merged
antoinesd merged 1 commit intojakartaee:masterfrom
antoinesd:CDI-727
Jul 19, 2018
Merged

CDI-727 CDI.current() should use privileged block#391
antoinesd merged 1 commit intojakartaee:masterfrom
antoinesd:CDI-727

Conversation

@antoinesd
Copy link
Copy Markdown
Contributor

No description provided.

@antoinesd
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@manovotn
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@mkouba
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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