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
  • 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?

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
/*[MSG "K0637", "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("K0637")); //$NON-NLS-1$
}
/*[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.

I think throwErrorOnInit isn't needed, the Error can be moved around Line 1278 whenever java.security.manager is detected.

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 liked the variable since the -Djava.security.manager=disallow case should not throw an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

The message should be prefixed with "Error: "; see SecurityManagerWarnings.java.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is from java.lang.Error. The example output from https://openjdk.org/jeps/486 is:

java.lang.Error: A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported.
        at java.lang.System.initPhase3(java.base@24/System.java:2067)

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+.

@@ -25,6 +25,7 @@
#include "j9.h"
#include "j9port.h"

#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.

This file can be removed 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.

How do I do that?

Copy link
Member

Choose a reason for hiding this comment

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

It can be decorated w/ if(JAVA_SPEC_VERSION LESS 24)

${CMAKE_CURRENT_SOURCE_DIR}/common/acccont.c

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to do the equivalent for UMA builds.

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, I added that and removed the directives from acccont.c.

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 missed the UMA comment, working on that too.

@@ -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
/*[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*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Space before */, please - here and on line 1431.

@@ -44,30 +46,30 @@
@SuppressWarnings("removal")
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */
public final class AccessController {
/*[IF JAVA_SPEC_VERSION >= 24]*/
private static AccessControlContext ACC_NO_PERM = new AccessControlContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, please add final.

static final int OBJS_INDEX_ACC = 0;
static final int OBJS_INDEX_PDS = 1;
static final int OBJS_ARRAY_SIZE = 3;
static final int OBJS_INDEX_PERMS_OR_CACHECHECKED = 2;

private static native void initializeInternal();
private static native void initializeInternal();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since initializeInternal() is no longer used; the implementation should be removed.

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 will be removed by the cmake/uma changes to exclude acccont.c

Comment on lines 873 to 875
/*[ELSE] JAVA_SPEC_VERSION >= 24*/
return doPrivileged(action, doPrivilegedWithCombinerHelper(null));
/*[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.

Space before */ on lines 873, 875, 904, 906, etc.

Comment on lines 954 to 967
/*[IF JAVA_SPEC_VERSION < 24]*/
checkPermsNPE(perms);
/*[ENDIF] JAVA_SPEC_VERSION < 24 */
T result = action.run();
/*[IF JAVA_SPEC_VERSION < 24]*/
keepalive(context);
keepalive(perms);
/*[ENDIF] JAVA_SPEC_VERSION < 24 */
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest this be simplified to remove the unnecessary local for jdk24+

/*[IF JAVA_SPEC_VERSION < 24]*/
	checkPermsNPE(perms);
	T result = action.run();
	keepalive(context);
	keepalive(perms);
	return result;
/*[ELSE] JAVA_SPEC_VERSION < 24 */
	return action.run();
/*[ENDIF] JAVA_SPEC_VERSION < 24 */

@@ -25,6 +25,7 @@
#include "j9.h"
#include "j9port.h"

#if 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.

Don't forget to do the equivalent for UMA builds.

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

#define STACK_WALK_STATE_MAGIC (void *)1
Copy link
Contributor

Choose a reason for hiding this comment

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

Tabs should be used for indentation only. Please use spaces elsewhere.
This macro and the other STACK_WALK_* macros are missing parentheses:

#define STACK_WALK_STATE_MAGIC ((void *)1)

Comment on lines 4 to 22
Copyright IBM Corp. and others 2024

This program and the accompanying materials are made available under
the terms of the Eclipse Public License 2.0 which accompanies this
distribution and is available at https://www.eclipse.org/legal/epl-2.0/
or the Apache License, Version 2.0 which accompanies this distribution and
is available at https://www.apache.org/licenses/LICENSE-2.0.

This Source Code may also be made available under the following
Secondary Licenses when the conditions for such availability set
forth in the Eclipse Public License, v. 2.0 are satisfied: GNU
General Public License, version 2 with the GNU Classpath
Exception [1] and GNU General Public License, version 2 with the
OpenJDK Assembly Exception [2].

[1] https://www.gnu.org/software/classpath/license.html
[2] https://openjdk.org/legal/assembly-exception.html

SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 OR GPL-2.0-only WITH OpenJDK-assembly-exception-1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't indent these lines.

Copy link
Contributor Author

@theresa-m theresa-m Dec 3, 2024

Choose a reason for hiding this comment

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

These indents seem to be standard for licensing in command line tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Common perhaps, but less than ideal.

Comment on lines 57 to 60
<echo value=" "/>
<echo value="#######################################################"/>
<echo value="Running tests with command line options: $currentMode$"/>
<echo value=" "/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change value=" " to value="" (the space isn't helpful).

test/functional/testVars.mk Show resolved Hide resolved
@theresa-m theresa-m force-pushed the fix_20563 branch 3 times, most recently from f876cb1 to 66ebb8e Compare December 4, 2024 19:49
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]>
@theresa-m
Copy link
Contributor Author

This is ready for another review. I'm working on building this with uma locally to test out those changes.

@@ -930,11 +957,15 @@ private static void checkPermsNPE(Permission... perms) {
public static <T> T doPrivileged(PrivilegedAction<T> action,
AccessControlContext context, Permission... perms)
{
/*[IF 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.

Please remove the space after ].

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