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

Consider VM-configuration when determining if SecurityManager may be set #216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link

@HannesWell HannesWell commented Dec 15, 2024

Since Java-12 users can configure a JVM to disallow the installation of a SecurityManager at runtime. The default value of the configuration has changed over the following versions and since Java-24 it's finally not possible anymore under any circumstances to install a SecurityManager at runtime (see JEP-486).

To quote from JEP-411:

Since Java 12, the end user has been able to prevent [dynamic installations of a Security Manager] by setting
the system property java.security.manager to disallow on the command line (java -Djava.security.manager=disallow ...)
-- this causes System::setSecurityManager to throw an UnsupportedOperationException. Starting in Java 18,
the default value of java.security.manager will be disallow[...].`

To determine if setting a SecurityManager is allowed, it's therefore not sufficient to just check the version of the running JVM, if it's a VM for Java-12 up to 23 (including). In case of the latter it has to be tested explicitly if setting a SecurityManager is supported.

@HannesWell
Copy link
Author

I have now changed this to be less intrusive and not relying on deprecated (SecurityManager)-API.

On the other hand, the previous approach would also handle the case if a security-manager is installed initially that does not permit another SecurityManager to be set.

@jaikiran
Copy link
Member

Hello Hannes, I haven't had a chance to look at this in detail, but looking at the linked issue, I think what should be done here is, only change the implementation of isSetSecurityManagerAllowed() method to:

public static boolean isSetSecurityManagerAllowed() {
    // for Java 18 or higher Ant will not set the SecurityManager 
    if (isJava18OrHigher) {
        return false;
    }
    // for other versions (lower than Java 18), if "java.security.manager"
    // system property is set to "disallow", Ant will not set the SecurityManager
    return !"disallow".equals(System.getProperty("java.security.manager"));
}

That way Ant will continue to not set the SecurityManager for Java 18 or higher and for lesser versions, Ant will then not set the SecurityManager, if the system property is set to "disallow".

Since Java-12 users can configure a JVM to disallow the installation of
a SecurityManager at runtime. The default value of the configuration has
changed over the following versions and since Java-24 it's finally not
possible anymore under any circumstances to install a SecurityManager at
runtime (see JEP-486[1]).

To quote from JEP-411[0]:
"""
Since Java 12, the end user has been able to prevent [dynamic
installations of a Security Manager] by setting the system property
java.security.manager to disallow on the command line (java
-Djava.security.manager=disallow ...) -- this causes
System::setSecurityManager to throw an UnsupportedOperationException.
Starting in Java 18, the default value of java.security.manager will be
disallow[...].
"""
To determine if setting a SecurityManager is allowed, it's therefore not
sufficient to just check the version of the running JVM, if it's a VM
for Java-12 up to 23 (including). In case of the latter it has to be
tested explicitly if setting a SecurityManager is supported.

[0] - https://openjdk.org/jeps/411
[1] - https://openjdk.org/jeps/486
@HannesWell HannesWell force-pushed the detailed-set-security-manager-check branch from 159e7e9 to 6b1d0c7 Compare December 17, 2024 23:14
@HannesWell
Copy link
Author

Thanks for the quick feedback.

That way Ant will continue to not set the SecurityManager for Java 18 or higher and for lesser versions, Ant will then not set the SecurityManager, if the system property is set to "disallow".

Not setting it at all for Java-18 or later sounds good. But below Java-12 it wasn't possible to disable it, therefore I think it would be good to always set it then. Furthermore I think it would be good to cache the result.
I have updated the PR to incorporate that, but if you prefer exactly your suggestion I can change it to it as well. Just let me know. :)

@jaikiran
Copy link
Member

Hello Hannes, caching the value is fine and what you propose here is reasonable. My only suggestion would be to remove the reference and usage of Java 12 check. I think if Java version is lesser than 18, then we should merely rely on the system property value instead of doing additional version checks. Something like (the following untested code):

// Ant will not set the SecurityManager if Java version is 18 or higher.
// For versions lower than Java 18, Ant will not set the SecurityManager 
// if the "java.security.manager" system property is set to "disallow".
private static final boolean IS_SET_SECURITYMANAGER_ALLOWED = !JavaEnvUtils.isAtLeastJavaVersion("18")
                && !"disallow".equals(System.getProperty("java.security.manager"));

The reason I say we should avoid the check for Java 12 or such is because although the "allow" and "disallow" values for the "java.security.manager" system property were introduced in that version, there has been efforts to allow those values in older releases too. More details in https://bugs.openjdk.org/browse/JDK-8301118. So in context of Ant, it would be better to leave out that extra check for Java 12.

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.

2 participants