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.

Sign up for GitHub

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

JDK24: Permanently Disable the Security Manager #20625

Merged
merged 11 commits into from
Dec 10, 2024
+350 −93
Merged

JDK24: Permanently Disable the Security Manager #20625

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e54eb6e
Set security manager only for JDK 18-23
theresa-m Nov 18, 2024
df22624
Disable cmdLineTester_J9securityTests for 24+
theresa-m Nov 18, 2024
ef69cd5
Disable cmdLineTester_ProxyFieldAccess for 24+
theresa-m Nov 18, 2024
143dd3f
Split DataHelperTests reliant on the security manager
theresa-m Nov 18, 2024
22dddaa
Update criu_nonPortable SecurityManager error for 24+
theresa-m Nov 18, 2024
b63281e
Don't fail on TestAttachAPI SecurityException for 24
theresa-m Nov 22, 2024
3bed57d
Disable security manager in java.lang.System
theresa-m Nov 22, 2024
e6eb7a3
Exclude URLClassLoaderTests NonExistJarTests.CheckForError1
theresa-m Nov 28, 2024
ff753fe
Exclude methods that are no longer used in Java 24
theresa-m Dec 5, 2024
b798307
Disable System.security reflection test
theresa-m Dec 6, 2024
edb99e8
Supress warning for System.initSecurityManager
theresa-m Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions jcl/src/java.base/share/classes/com/ibm/oti/util/ExternalMessages-MasterIndex.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,8 @@ K0A02="Bootstrap method returned null."
K0B00="The Security Manager is deprecated and will be removed in a future release"
K0B01="Library name must not contain a file path: {0}"
K0B02="Enabling a SecurityManager currently unsupported when -XX:+EnableCRIUSupport is specified"
K0B03="Setting a Security Manager is not supported"
K0B04="A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."

#java.lang.Throwable
K0C00="Non-standard List class not permitted in suppressedExceptions serial stream"
Expand Down
9 changes: 8 additions & 1 deletion jcl/src/java.base/share/classes/java/lang/Class.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3204,7 +3204,9 @@ public boolean desiredAssertionStatus() {
* array of not more than maxDepth Classes representing the classes of
* running methods on the stack (including native methods). Frames
* representing the VM implementation of java.lang.reflect are not included
* in the list. If stopAtPrivileged is true, the walk will terminate at any
* in the list.
/*[IF JAVA_SPEC_VERSION < 24]
* If stopAtPrivileged is true, the walk will terminate at any
* frame running one of the following methods:
*
* <code><ul>
Expand All @@ -3215,6 +3217,7 @@ public boolean desiredAssertionStatus() {
* </ul></code>
*
* If one of the doPrivileged methods is found, the walk terminate and that frame is NOT included in the returned array.
/*[ENDIF] JAVA_SPEC_VERSION < 24
*
* Notes: <ul>
* <li> This method operates on the defining classes of methods on stack.
Expand All @@ -3225,7 +3228,11 @@ public boolean desiredAssertionStatus() {
*</ul>
*
* @param maxDepth maximum depth to walk the stack, -1 for the entire stack
/*[IF JAVA_SPEC_VERSION >= 24]
* @param stopAtPrivileged has no effect
/*[ELSE] JAVA_SPEC_VERSION >= 24
* @param stopAtPrivileged stop at privileged classes
/*[ENDIF] JAVA_SPEC_VERSION >= 24
* @return the array of the most recent classes on the stack
*/
@CallerSensitive
Expand Down
26 changes: 26 additions & 0 deletions jcl/src/java.base/share/classes/java/lang/System.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,9 @@ static void checkTmpDir() {

/*[IF JAVA_SPEC_VERSION >= 9]*/
static void initSecurityManager(ClassLoader applicationClassLoader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initSecurityManager() can be removed at

openj9/jcl/src/java.base/share/classes/java/lang/ClassLoader.java

Line 297 in 48709bf

System.initSecurityManager(applicationClassLoader);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still needed since initSecurityManager is used to detect settings of the java.security.manager property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initSecurityManager() reads the system property java.security.manager, and sets throwUOEFromSetSM which can be skipped within setSecurityManager().
System.initSecurityManager(applicationClassLoader) seems not needed for JDK24+.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will still be needed to throw an exception on startup for illegal java.security.manager manager settings triggered by throwErrorOnInit .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static void initSecurityManager(ClassLoader applicationClassLoader) {
static void initSecurityManager(ClassLoader applicationClassLoader) {
String javaSecurityManager = internalGetProperties().getProperty("java.security.manager"); //$NON-NLS-1$
if (null == javaSecurityManager) {
/*[IF JAVA_SPEC_VERSION >= 18]*/
throwUOEFromSetSM = true;
/*[ELSE] JAVA_SPEC_VERSION >= 18 */
/* Do nothing. */
/*[ENDIF] JAVA_SPEC_VERSION >= 18 */
} else if ("disallow".equals(javaSecurityManager)) { //$NON-NLS-1$
/*[IF JAVA_SPEC_VERSION > 11]*/
throwUOEFromSetSM = true;
/*[ELSE] JAVA_SPEC_VERSION > 11 */
/* Do nothing. */
/*[ENDIF] JAVA_SPEC_VERSION > 11 */
} else {
/*[IF JAVA_SPEC_VERSION >= 24]*/
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$
/*[ELSE] JAVA_SPEC_VERSION >= 24 */
if ("allow".equals(javaSecurityManager)) { //$NON-NLS-1$
/* Do nothing. */
} else {
/*[IF JAVA_SPEC_VERSION >= 17]*/
initialErr.println("WARNING: A command line option has enabled the Security Manager"); //$NON-NLS-1$
initialErr.println("WARNING: The Security Manager is deprecated and will be removed in a future release"); //$NON-NLS-1$
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */
if (javaSecurityManager.isEmpty() || "default".equals(javaSecurityManager)) { //$NON-NLS-1$
setSecurityManager(new SecurityManager());
} else {
try {
Constructor<?> constructor = Class.forName(javaSecurityManager, true, applicationClassLoader).getConstructor();
constructor.setAccessible(true);
setSecurityManager((SecurityManager)constructor.newInstance());
} catch (Throwable e) {
/*[MSG "K0631", "JVM can't set custom SecurityManager due to {0}"]*/
throw new Error(Msg.getString("K0631", e.toString()), e); //$NON-NLS-1$
}
}
}
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify the benefits of this approach over the existing change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability, the proposed change has 4 pairs of /*[IF JAVA_SPEC_VERSION >= 24]*/ and an unnecessary local variable throwErrorOnInit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we have is correct. This could be refactored to eliminate that local variable, but I think that should be done separately (if at all), and I can see ways forward that are even better.

/*[IF JAVA_SPEC_VERSION >= 24]*/
boolean throwErrorOnInit = false;
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
String javaSecurityManager = internalGetProperties().getProperty("java.security.manager"); //$NON-NLS-1$
if (null == javaSecurityManager) {
/*[IF JAVA_SPEC_VERSION >= 18]*/
Expand All @@ -1273,14 +1276,21 @@ static void initSecurityManager(ClassLoader applicationClassLoader) {
/* Do nothing. */
/*[ENDIF] JAVA_SPEC_VERSION >= 18 */
} else if ("allow".equals(javaSecurityManager)) { //$NON-NLS-1$
/*[IF JAVA_SPEC_VERSION >= 24]*/
throwErrorOnInit = true;
/*[ELSE] JAVA_SPEC_VERSION >= 24 */
/* Do nothing. */
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
} else if ("disallow".equals(javaSecurityManager)) { //$NON-NLS-1$
/*[IF JAVA_SPEC_VERSION > 11]*/
throwUOEFromSetSM = true;
/*[ELSE] JAVA_SPEC_VERSION > 11 */
/* Do nothing. */
/*[ENDIF] JAVA_SPEC_VERSION > 11 */
} else {
/*[IF JAVA_SPEC_VERSION >= 24]*/
throwErrorOnInit = true;
/*[ELSE] JAVA_SPEC_VERSION >= 24 */
/*[IF JAVA_SPEC_VERSION >= 17]*/
initialErr.println("WARNING: A command line option has enabled the Security Manager"); //$NON-NLS-1$
initialErr.println("WARNING: The Security Manager is deprecated and will be removed in a future release"); //$NON-NLS-1$
theresa-m marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -1297,7 +1307,14 @@ static void initSecurityManager(ClassLoader applicationClassLoader) {
throw new Error(Msg.getString("K0631", e.toString()), e); //$NON-NLS-1$
}
}
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
}
/*[IF JAVA_SPEC_VERSION >= 24]*/
if (throwErrorOnInit) {
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$
}
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
keithc-ca marked this conversation as resolved.
Show resolved Hide resolved
}
/*[ENDIF] JAVA_SPEC_VERSION >= 9 */

Expand All @@ -1315,17 +1332,25 @@ static boolean allowSecurityManager() {
*
* @param s the new security manager
*
/*[IF JAVA_SPEC_VERSION >= 24]
* @throws UnsupportedOperationException always
/*[ELSE] JAVA_SPEC_VERSION >= 24
* @throws SecurityException if the security manager has already been set and its checkPermission method doesn't allow it to be replaced.
/*[IF JAVA_SPEC_VERSION > 11]
* @throws UnsupportedOperationException if s is non-null and a special token "disallow" has been set for system property "java.security.manager"
* which indicates that a security manager is not allowed to be set dynamically.
/*[ENDIF] JAVA_SPEC_VERSION > 11
/*[ENDIF] JAVA_SPEC_VERSION >= 24
*/
/*[IF JAVA_SPEC_VERSION >= 17]*/
@Deprecated(since="17", forRemoval=true)
@CallerSensitive
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */
public static void setSecurityManager(final SecurityManager s) {
/*[IF JAVA_SPEC_VERSION >= 24]*/
/*[MSG "K0B03", "Setting a Security Manager is not supported"]*/
throw new UnsupportedOperationException(Msg.getString("K0B03")); //$NON-NLS-1$
/*[ELSE] JAVA_SPEC_VERSION >= 24 */
/*[IF CRIU_SUPPORT]*/
if (openj9.internal.criu.InternalCRIUSupport.isCRIUSupportEnabled()) {
/*[MSG "K0B02", "Enabling a SecurityManager currently unsupported when -XX:+EnableCRIUSupport is specified"]*/
Expand Down Expand Up @@ -1403,6 +1428,7 @@ public Void run() {
currentSecurity.checkPermission(com.ibm.oti.util.RuntimePermissions.permissionSetSecurityManager);
}
security = s;
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
}

/**
Expand Down
10 changes: 10 additions & 0 deletions jcl/src/java.base/share/classes/java/security/AccessControlContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,13 @@ private boolean debugHelper(Permission perm) {
}

/**
/*[IF JAVA_SPEC_VERSION >= 24]
* Throws java.security.AccessControlException
*
* @param perm is ignored
* @exception java.security.AccessControlException
* is always thrown
/*[ELSE] JAVA_SPEC_VERSION >= 24
* Checks if the permission <code>perm</code> is allowed in this context.
* All ProtectionDomains must grant the permission for it to be granted.
*
Expand All @@ -735,6 +742,7 @@ private boolean debugHelper(Permission perm) {
* thrown when perm is not granted.
* @exception NullPointerException
* if perm is null
/*[ENDIF] JAVA_SPEC_VERSION >= 24
*/
public void checkPermission(Permission perm) throws AccessControlException {
/*[IF JAVA_SPEC_VERSION >= 24]*/
Expand Down Expand Up @@ -946,6 +954,7 @@ ProtectionDomain[] getContext() {
return context;
}

/*[IF JAVA_SPEC_VERSION < 24]*/
/*
* Added to resolve: S6907662, CVE-2010-4465: System clipboard should ensure access restrictions
* Called internally from java.security.ProtectionDomain
Expand All @@ -959,6 +968,7 @@ ProtectionDomain[] getContext() {
this.domainCombiner = acc.domainCombiner;
}
}
/*[ENDIF] JAVA_SPEC_VERSION < 24 */

/*
* Added to resolve: S6907662, CVE-2010-4465: System clipboard should ensure access restrictions
Expand Down
Loading