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

[BUG] Somehow security plugin rewrites permissions for other plugins which use Bouncy Castle #3213

Closed
willyborankin opened this issue Aug 21, 2023 · 13 comments
Labels
bug Something isn't working

Comments

@willyborankin
Copy link
Collaborator

willyborankin commented Aug 21, 2023

What is the bug?

Use case:
2 plugins which use BC.OpenSearch starts plugins on the node in the alphabetic order.
The plugin which starts first uses right permissions for PC to execute its operations.
After the sec plugin started permissions changed as result the first plugin can't use its permissions.

What is the expected behavior?
Permissions should not be changed.

What is your host/environment?

  • OS: 2.9 only
@willyborankin willyborankin added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 21, 2023
@stephen-crawford stephen-crawford changed the title [BUG] Somehow security plugin rewrites permissions for other plugins which use BC [BUG] Somehow security plugin rewrites permissions for other plugins which use Bouncy Castle Aug 21, 2023
@stephen-crawford
Copy link
Contributor

[Triage] Hi @willyborankin, thank you for filing this issue. The first step to addressing this issue may be determining a mitigation strategy based on the potential for the Security plugin to restrict other components. Seems the first item for this issue will be determining the extent of the issue and whether we can work around the problem. Leaving untriaged pending actionable tasks--but for the interim this will require diagnosis.

@stephen-crawford stephen-crawford removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Aug 21, 2023
@peternied
Copy link
Member

Could be related to the recent bump in the plugin policy?

@willyborankin
Copy link
Collaborator Author

Reproduced it with 1.3.12 something is on OS side

@willyborankin willyborankin reopened this Aug 22, 2023
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Aug 22, 2023
@reta
Copy link
Collaborator

reta commented Aug 22, 2023

@peternied @cwperks @scrawfor99 so we find out the issue which is essentially related to the way Java allows to add the security providers. The security plugin registers the BouncyCastleProvider using Security.addProvider method call:

Security.addProvider(new BouncyCastleProvider());

This is global per JVM and, in fact, could replaced by other plugin later on. That is one issue but the another one is that if any other plugin invokes JCE APIs, like for example here:

 at java.base/javax.crypto.Cipher.init(Cipher.java:1296)
 at io.aiven.elasticsearch.repositories.security.Decryption.createDecryptingCipher(Decryption.java:52)

It will be using the BouncyCastleProvider provider registered by security plugin and AccessController will use permission checks applied to security plugin in this context (because of the protection domain file:/opt/opensearch/plugins/opensearch-security/bcprov-jdk15to18-1.75.jar of the class). So in this case, if security plugin does not have the permission required by the caller, the invocation will fail, like in this example:

access: access denied ("java.security.SecurityPermission" "getProperty.org.bouncycastle.rsa.max_size")

@reta
Copy link
Collaborator

reta commented Aug 22, 2023

The problem seems in general unsolvable if relying on Security.addProvider. As one option, the security plugin may widen up the permissions that are required by BouncyCastle, for example:

permission java.security.SecurityPermission "getProperty.org.bouncycastle.*";

It may partially solve the issue.

@cwperks
Copy link
Member

cwperks commented Aug 23, 2023

@willyborankin could the JJWT issue experienced on 2.x be the same problem here?

We made a PR to wrap the call to create the JJWT JwtParser with AccessController.doPrivileged(...) because there was a permissions issue: #3189

It was complaining it couldn't find the following permission:

Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "getClassLoader")

but this permission is defined in the plugin-security.policy file here. If it's granted in the policy file, then is that policy not being taken into effect somehow?

Recapping the permissions issues we've seen recently:

@willyborankin
Copy link
Collaborator Author

@cwperks Yes looks closer. I took a look in JJWT DefaultJwtParserBuilder it uses ServiceLoader which is ok. I'm wandering what was changed in OS.

@reta
Copy link
Collaborator

reta commented Aug 23, 2023

@cwperks I think these issues are unrelated:

  • Jwt needed AccessController.doPrivileged (you fixed that)
  • Saml needs to use Cleaner.create(<thread factory>) instead of Cleaner.create() so we could provide the one OS managed one (will be looking into that with @willyborankin )

@cwperks
Copy link
Member

cwperks commented Aug 23, 2023

Ok, I see why the JJWT AccessController.doPrivileged(...) change was not required on main now. Sorry to bring that up in this issue, they are unrelated.

The reason it was not required on main (but was on 2.x) is because the fix for the SAML permissions issue after upgrade to OpenSAML 4.3.0 wrapped the HttpAuthenticator.newInstance call in AccessController.doPrivileged which includes running the HttpJwtAuthenticator's constructor. So it is transitively wrapped in AccessController.doPrivileged(...).

Relevant lines: df07bea#diff-f1cdacfc69b9ee7a3348e9a20a39ea0c284eff6386f3c660fc58a5874e8e071aR402-R406

final String clazz = authImplMap.computeIfAbsent(clazzOrShortcut + "_" + type, k -> clazzOrShortcut);
return AccessController.doPrivileged((PrivilegedAction<T>) () -> {
    SpecialPermission.check();
    return ReflectionHelper.instantiateAAA(clazz, settings, configPath);
});

@peternied peternied added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 28, 2023
@peternied
Copy link
Member

[Triage] @reta could you provide details on what next steps look like for this issue?

@peternied peternied removed the triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. label Aug 28, 2023
@cwperks
Copy link
Member

cwperks commented Sep 5, 2023

FYI This is now blocking the build on main:

Caused by: java.lang.InternalError: cannot create instance of org.bouncycastle.jcajce.provider.digest.GOST3411$Mappings : java.security.AccessControlException: access denied ("java.security.SecurityPermission" "putProviderProperty.BC")
	at org.bouncycastle.jce.provider.BouncyCastleProvider.loadServiceClass(Unknown Source) ~[bcprov-jdk15to18-1.75.jar:1.75.0.0]
	at org.bouncycastle.jce.provider.BouncyCastleProvider.loadAlgorithms(Unknown Source) ~[bcprov-jdk15to18-1.75.jar:1.75.0.0]
	at org.bouncycastle.jce.provider.BouncyCastleProvider.setup(Unknown Source) ~[bcprov-jdk15to18-1.75.jar:1.75.0.0]
	at org.bouncycastle.jce.provider.BouncyCastleProvider.access$000(Unknown Source) ~[bcprov-jdk15to18-1.75.jar:1.75.0.0]
	at org.bouncycastle.jce.provider.BouncyCastleProvider$1.run(Unknown Source) ~[bcprov-jdk15to18-1.75.jar:1.75.0.0]
	at java.security.AccessController.doPrivileged(AccessController.java:318) ~[?:?]
	at org.bouncycastle.jce.provider.BouncyCastleProvider.<init>(Unknown Source) ~[bcprov-jdk15to18-1.75.jar:1.75.0.0]
	at org.opensearch.security.OpenSearchSecurityPlugin$2.run(OpenSearchSecurityPlugin.java:338) ~[?:?]
	at java.security.AccessController.doPrivileged(AccessController.java:318) ~[?:?]
	at org.opensearch.security.OpenSearchSecurityPlugin.<init>(OpenSearchSecurityPlugin.java:334) ~[?:?]
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?]
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77) ~[?:?]
	at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?]
	at java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) ~[?:?]
	at java.lang.reflect.Constructor.newInstance(Constructor.java:480) ~[?:?]
	at org.opensearch.plugins.PluginsService.loadPlugin(PluginsService.java:782) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	... 15 more

Example: https://github.com/cwperks/security/actions/runs/6086141514/job/16511809613

@reta
Copy link
Collaborator

reta commented Sep 5, 2023

Hm ... we have the security policy set:

  permission java.security.SecurityPermission "putProviderProperty.BC";

I think we may need to add one for bouncycastle jars as well:

grant codeBase "${codebase.bcprov-jdk15to18}" {
    // BouncyCastle permissions
  permission java.security.SecurityPermission "putProviderProperty.BC";
  permission java.security.SecurityPermission "insertProvider.BC";
  permission java.security.SecurityPermission "removeProviderProperty.BC";
};

?

@stephen-crawford
Copy link
Contributor

[Triage] Resolved by #3289.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants