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

JDK24: Permanently Disable the Security Manager #20625

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Nov 18, 2024

  • Throw an error on initialization if java.security.manager attempts to add a security manager
  • configure System.setSecurityManager to always throw an UnsupportedOperationException
  • System.getSecurityManager will always return null since a security manager can't be set
  • Update java.security.* javadocs
  • Exclude unused helper methods in java.security.* including native code
  • Exclude unused helper methods and natives in java.lang.Class
  • Disable functional tests that rely on a security manager

Related: #20563

<disables>
<disable>
<comment>https://github.com/eclipse-openj9/openj9/issues/20563</comment>
<version>24+</version>
Copy link
Contributor Author

@theresa-m theresa-m Nov 18, 2024

Choose a reason for hiding this comment

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

@llxia is this the preferred way to disable tests from running in future versions? The security tests should run for JDK 11-23 only. I've mostly seen the block used in temporarily disabled tests so I wanted to check since this would be a permanent change.

Copy link
Contributor

@llxia llxia Nov 19, 2024

Choose a reason for hiding this comment

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

disable is for temporary excludes. In this case, we should set <version>[11, 23]</version> . Example code: https://github.com/adoptium/TKG/blob/master/examples/jdkVersion/playlist.xml#L39

@theresa-m theresa-m force-pushed the fix_20563 branch 3 times, most recently from 888855d to 7ab4eda Compare November 19, 2024 16:24
@theresa-m theresa-m marked this pull request as ready for review November 20, 2024 15:14
@tajila
Copy link
Contributor

tajila commented Nov 20, 2024

@JasonFengJ9 Please review these changes

Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

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

The natives related to checkPermission() can be ifdef out for JDK24+ such as

Java_java_security_AccessController_getAccSnapshot(JNIEnv* env, jclass jsAccessController, jint startingFrame, jboolean forDoPrivilegedWithCombiner)

@@ -1265,6 +1265,10 @@ 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

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.

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved
test/functional/testVars.mk Outdated Show resolved Hide resolved
@pshipton
Copy link
Member

FYI #20655

@theresa-m theresa-m force-pushed the fix_20563 branch 2 times, most recently from fce9383 to fc9e6eb Compare November 22, 2024 21:28
Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

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

In addition, all AccessController.doPrivileged_XXX usages can be removed for JDK24+ such as

static final boolean ENABLED = AccessController.doPrivileged(new PrivilegedAction<Boolean>() {

Yeah, there are lots of them.

@@ -49,25 +51,25 @@ public final class AccessController {
initializeInternal();
}

private static native void initializeInternal();
Copy link
Member

Choose a reason for hiding this comment

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

This native can be removed for JDK24+, I expect J9JavaVM->doPrivilegedMethodID_XXX are not needed along with

jboolean JNICALL Java_java_security_AccessController_initializeInternal(JNIEnv *env, jclass thisClz)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have excluded these and am running a personal build to see if there are any impacted tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have excluded and opened an issue for the one failure I found #20702.

@theresa-m theresa-m force-pushed the fix_20563 branch 3 times, most recently from 72168cc to c9cabc6 Compare November 25, 2024 18:25
@theresa-m
Copy link
Contributor Author

In addition, all AccessController.doPrivileged_XXX usages can be removed for JDK24+ such as

static final boolean ENABLED = AccessController.doPrivileged(new PrivilegedAction<Boolean>() {

Yeah, there are lots of them.

Do you mind if I do this in a second pull request? This change set is already getting large.

@JasonFengJ9
Copy link
Member

In addition, all AccessController.doPrivileged_XXX usages can be removed for JDK24+
Yeah, there are lots of them.

Do you mind if I do this in a second pull request? This change set is already getting large.

Sounds good.

@@ -1265,6 +1265,10 @@ 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.

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 */
}
}

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved
jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved
@@ -637,6 +651,7 @@ private static int getNewAuthorizedState(AccessControlContext acc, ProtectionDom
}
return newAuthorizedState;
}
/*[ENDIF] JAVA_SPEC_VERSION < 24 */
Copy link
Member

Choose a reason for hiding this comment

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

This can be moved to L722 to include toArrayOfProtectionDomains().

checkPermsNPE(perms);
/*[ENDIF] JAVA_SPEC_VERSION < 24 */
Copy link
Member

Choose a reason for hiding this comment

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

keepalive() can be removed for JDK24+.

runtime/jcl/common/acccont.c Outdated Show resolved Hide resolved
@@ -55,10 +55,12 @@ typedef enum {
#define STACK_WALK_STATE_LIMITED_DOPRIVILEGED (void *)2
#define STACK_WALK_STATE_FULL_DOPRIVILEGED (void *)3

#if JAVA_SPEC_VERSION < 24
Copy link
Member

Choose a reason for hiding this comment

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

ObjsArraySizeNindex, STACK_WALK_STATE_LIMITED_DOPRIVILEGED and STACK_WALK_STATE_FULL_DOPRIVILEGED can be removed for JDK24+.

@theresa-m theresa-m force-pushed the fix_20563 branch 2 times, most recently from ac1f215 to 4f82040 Compare December 2, 2024 18:01
@theresa-m theresa-m force-pushed the fix_20563 branch 2 times, most recently from 25b945a to f876cb1 Compare December 4, 2024 19:46
@theresa-m theresa-m force-pushed the fix_20563 branch 2 times, most recently from acbcf39 to 70eefab Compare December 5, 2024 14:16
@theresa-m
Copy link
Contributor Author

I compiled these changes successfully on my machine on versions next and 21 with --with-cmake=no.

runtime/jcl/common/java_lang_Class.cpp Show resolved Hide resolved
runtime/jcl/module.xml Outdated Show resolved Hide resolved
runtime/jcl/exports.cmake Outdated Show resolved Hide resolved
@theresa-m theresa-m force-pushed the fix_20563 branch 2 times, most recently from bd88c01 to 1952037 Compare December 5, 2024 17:47
@keithc-ca
Copy link
Contributor

Please update the commit message for 1952037; it makes the bulk of the non-test changes, but doesn't describe them very well.

@theresa-m theresa-m force-pushed the fix_20563 branch 2 times, most recently from b8478cd to 563f9b2 Compare December 5, 2024 19:14
@keithc-ca
Copy link
Contributor

Jenkins test sanity amac jdk21,jdknext

Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

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

JVM_DoPrivileged(JNIEnv* env, jobject java_security_AccessController, jobject action, jboolean unknown, jboolean isExceptionAction)

This native and its helper methods can be removed for JDK24+.

"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedAction;Ljava/security/AccessControlContext;)Ljava/lang/Object;",
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedExceptionAction;Ljava/security/AccessControlContext;)Ljava/lang/Object;",
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedAction;Ljava/security/AccessControlContext;[Ljava/security/Permission;)Ljava/lang/Object;",
"java/security/AccessController.doPrivileged(Ljava/security/PrivilegedExceptionAction;Ljava/security/AccessControlContext;[Ljava/security/Permission;)Ljava/lang/Object;",
"java/lang/NullPointerException.fillInStackTrace()Ljava/lang/Throwable;",

We need a JIT reviewer to remove these.

These changes could be in a separated PR.

@@ -857,7 +874,11 @@ public static <T> T doPrivileged (PrivilegedExceptionAction<T> action, AccessCon
*/
@CallerSensitive
public static <T> T doPrivilegedWithCombiner(PrivilegedAction<T> action) {
/*[IF JAVA_SPEC_VERSION >= 24]*/
Copy link
Member

Choose a reason for hiding this comment

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

Please be consistent with the indention pattern with earlier JPP decorations.

@@ -884,9 +905,14 @@ public static <T> T doPrivilegedWithCombiner(PrivilegedAction<T> action) {
public static <T> T doPrivilegedWithCombiner(PrivilegedExceptionAction<T> action)
throws PrivilegedActionException
{
/*[IF JAVA_SPEC_VERSION >= 24]*/
Copy link
Member

Choose a reason for hiding this comment

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

Similar indention pattern comment.

@@ -962,8 +993,12 @@ public static <T> T doPrivileged(PrivilegedAction<T> action,
public static <T> T doPrivilegedWithCombiner(PrivilegedAction<T> action,
AccessControlContext context, Permission... perms)
{
/*[IF JAVA_SPEC_VERSION >= 24]*/
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

@@ -1037,10 +1076,15 @@ public static <T> T doPrivilegedWithCombiner(PrivilegedExceptionAction<T> action
AccessControlContext context, Permission... perms)
throws PrivilegedActionException
{
/*[IF JAVA_SPEC_VERSION >= 24]*/
Copy link
Member

Choose a reason for hiding this comment

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

Also here

@@ -44,21 +44,25 @@ typedef enum {
STATE_IMPLIED = 1
} StackWalkingStates;

#define STACK_WALK_STATE_MAGIC ((void *)1)

#if JAVA_SPEC_VERSION < 24
Copy link
Member

@JasonFengJ9 JasonFengJ9 Dec 5, 2024

Choose a reason for hiding this comment

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

typedef struct DoPrivilegedMethodArgs {
UDATA frameCounter; /* the frame just walked */
j9object_t accControlContext; /* arg0EA[-1] - AccessControlContext */
j9object_t permissions; /* arg0EA[-2] - Limited permission array */
struct DoPrivilegedMethodArgs* next; /* next DoPrivilegedMethodArgs structure */
} DoPrivilegedMethodArgs;

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.

Thanks Jason. I made updates from your feedback.

Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

Tests are failing: see https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6650.

The symptoms suggest a stray extra quote, but I don't see where it originates.

Comment on lines 31 to 33
#if JAVA_SPEC_VERSION < 24
#include "java_lang_Class.h"
#endif /* JAVA_SPEC_VERSION < 24 */
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this include should be conditional. Ideally, it would be first (unconditionally), followed by includes needed for the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this exclude and fixed the test failure.

to a different job and exclude from 24+.

Signed-off-by: Theresa Mammarella <[email protected]>
- Throw an error on initialization if java.security.manager attempts to add a security manager
- configure setSecurityManager to always throw an UnsupportedOperationException
- getSecurityManager will always return null since a security manager can't be set

Signed-off-by: Theresa Mammarella <[email protected]>
... for further investigation.

Signed-off-by: Theresa Mammarella <[email protected]>
Exclude helper methods related to removing the
security manager including:
- AccessController.initializeInternal native in acccont.c
- native helper methods in java_lang_class
- doPriviled*id fields from J9JavaVM struct
- unused helper methods in java.security.AccessController
- unused helper methods in java.security.AccessControlContext
- unused helper methods in java.lang.Class

java.security comment update and exclude helper methods

Signed-off-by: Theresa Mammarella <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants