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

8270380: Change the default value of the java.security.manager system property to disallow #5204

Closed
wants to merge 2 commits into from

Conversation

@wangweij
Copy link
Contributor

@wangweij wangweij commented Aug 20, 2021

This change modifies the default value of the java.security.manager system property from "allow" to "disallow". This means unless it's explicitly set to "allow", any call to System.setSecurityManager() would throw an UOE.

This change was previously announced on the jdk-dev alias and is documented in JEP 411.

The AllowSecurityManager.java and SecurityManagerWarnings.java tests are updated to confirm this behavior change. Two other tests are updated because they were added after JDK-8267184 and do not have -Djava.security.manager=allow on its @run line even it they need to install a SecurityManager at runtime.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8270380: Change the default value of the java.security.manager system property to disallow

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5204/head:pull/5204
$ git checkout pull/5204

Update a local copy of the PR:
$ git checkout pull/5204
$ git pull https://git.openjdk.java.net/jdk pull/5204/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5204

View PR using the GUI difftool:
$ git pr show -t 5204

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5204.diff

… property to disallow

8270380: Change the default value of the java.security.manager system property to disallow
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 20, 2021

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Aug 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 20, 2021

@wangweij The following labels will be automatically applied to this pull request:

  • core-libs
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 20, 2021

Webrevs

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Looks good Max.

One minor suggestion below

@@ -127,8 +125,7 @@
* <tr>
* <th scope="row">null</th>
* <td>None</td>
* <td>Success or throws {@code SecurityException} if not permitted by
* the currently installed security manager</td>
* <td>Always throws {@code UnsupportedOperationException}</td>
Copy link
Contributor

@LanceAndersen LanceAndersen Aug 20, 2021

Choose a reason for hiding this comment

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

Not sure "Always" is needed, could just be "Throws UOE"

Copy link
Contributor Author

@wangweij wangweij Aug 21, 2021

Choose a reason for hiding this comment

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

This is the same as the existing words for "disallow".

Copy link
Contributor

@AlanBateman AlanBateman Aug 21, 2021

Choose a reason for hiding this comment

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

I think I agree with Lance that "Throws UOE" would be clearer in this table.

Copy link
Contributor Author

@wangweij wangweij Aug 21, 2021

Choose a reason for hiding this comment

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

Suggestion accepted.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 20, 2021

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8270380: Change the default value of the java.security.manager system property to disallow

Reviewed-by: lancea, mullan, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 750 new commits pushed to the master branch:

  • 043cde2: 8275319: java.net.NetworkInterface throws java.lang.Error instead of SocketException
  • a91a0a5: 8233724: Remove -Wc++14-compat warning suppression in operator_new.cpp
  • 1271fbf: 8248584: Enable CHECK_UNHANDLED_OOPS for Windows fastdebug builds
  • 135cf3c: 8275439: Remove PrintVtableStats
  • 50a5723: 8274910: Compile in G1 evacuation failure injection code based on define
  • 5454a76: 8275273: Add missing HtmlStyle documentation
  • bd0bed7: 8273317: crash in cmovP_cmpP_zero_zeroNode::bottom_type()
  • 77b2789: 7124287: [macosx] JTableHeader doesn't get focus after pressing F8 key
  • c24fb85: 8275512: Upgrade required version of jtreg to 6.1
  • 926966b: 8275003: Suppress warnings on non-serializable non-transient instance fields in windows mscapi
  • ... and 740 more: https://git.openjdk.java.net/jdk/compare/2ed7b709a197f009632580b17e3b1df34c1ffeb7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 20, 2021
@jaikiran
Copy link
Member

@jaikiran jaikiran commented Aug 23, 2021

A somewhat broader question - I looked at the javadocs of this latest update to SecurityManager in this PR. One thing I'm unclear about is, consider the case where the java.security.manager is not set to anything at the command line. Then in some application code, let's say we have this:

String oldVal = System.getProperty("java.security.manager");
try {
    System.setProperty("java.security.manager", "allow");
    System.setSecurityManager(someSecurityManager);
    .... // do something
} finally {
   System.setProperty("java.security.manager", oldVal);
}

Would this then allow the security manager to be used? In other words, can the value of java.security.manager be changed dynamically at runtime or is it restricted to be set only at launch time (via command line arugment -Djava.security.manager)?

@jaikiran
Copy link
Member

@jaikiran jaikiran commented Aug 23, 2021

It looks to me that the failures reported in the GitHub jobs are genuine and related to this change. It looks like the jtreg framework itself is impacted by this change because it calls the System.setSecurityManager(....) while launching the tests.

2021-08-21T01:41:42.5731927Z stderr:
2021-08-21T01:41:42.5733295Z java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
2021-08-21T01:41:42.5734843Z 	at java.base/java.lang.System.setSecurityManager(System.java:409)
2021-08-21T01:41:42.5736946Z 	at com.sun.javatest.regtest.agent.RegressionSecurityManager.install(RegressionSecurityManager.java:56)
2021-08-21T01:41:42.5739224Z 	at com.sun.javatest.regtest.agent.AgentServer.<init>(AgentServer.java:211)
2021-08-21T01:41:42.5740879Z 	at com.sun.javatest.regtest.agent.AgentServer.main(AgentServer.java:70)
2021-08-21T01:41:42.5741805Z 
2021-08-21T01:41:42.5743348Z TEST RESULT: Error. Agent communication error: java.net.SocketException: Connection reset; check console log for any additional details
2021-08-21T01:41:42.5745030Z --------------------------------------------------
2021-08-21T01:41:51.2539413Z Test results: passed: 5; error: 879
2021-08-21T01:41:59.0887042Z Report written to /home/runner/work/jdk/jdk/build/run-test-prebuilt/test-results/jtreg_test_jdk_tier1_part1/html/report.html
2021-08-21T01:41:59.0892404Z Results written to /home/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_jdk_tier1_part1
2021-08-21T01:41:59.0895498Z Error: Some tests failed or other problems occurred.
2021-08-21T01:41:59.1235748Z Finished running test 'jtreg:test/jdk:tier1_part1'
2021-08-21T01:41:59.1237631Z Test report is stored in build/run-test-prebuilt/test-results/jtreg_test_jdk_tier1_part1
2021-08-21T01:41:59.1650100Z 
2021-08-21T01:41:59.1667939Z ==============================
2021-08-21T01:41:59.1668727Z Test summary
2021-08-21T01:41:59.1669635Z ==============================
2021-08-21T01:41:59.1670187Z    TEST                                              TOTAL  PASS  FAIL ERROR   
2021-08-21T01:41:59.1670795Z >> jtreg:test/jdk:tier1_part1                          884     5     0   879 <<

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Aug 23, 2021

@wangweij there are many tests that can call setSecurityManager() and will presumably need to be fixed before this switch can be applied. And all testing will need to be updated to require jtreg 6.1 (which no longer uses the SM) once it is released.

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 23, 2021

Mailing list message from Alan Bateman on security-dev:

On 23/08/2021 05:45, David Holmes wrote:

:

@wangweij there are many tests that can call setSecurityManager() and will presumably need to be fixed before this switch can be applied. And all testing will need to be updated to require jtreg 6.1 (which no longer uses the SM) once it is released.

Most of the preparation work for the tests was done via JDK-8267184 in
JDK 17.

JDK-8270380 isn't going to be integrated until after the upgrade the
jtreg 6.1 (which I think is one part of Jai's mail).

-Alan.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Aug 23, 2021

Thanks for the clarification @AlanBateman . @wangweij my apologies as I misunderstood the affect this change was going to have on the existing warnings - which is none.

David

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Aug 23, 2021

This issue (https://bugs.openjdk.java.net/browse/JDK-8270380) is blocked by jtreg 6.1 (https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). Precisely, it should be blocked by a JDK bug that updates the jtreg required version to 6.1.

I should have mentioned this in the PR.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Aug 23, 2021

Would this then allow the security manager to be used? In other words, can the value of java.security.manager be changed dynamically at runtime or is it restricted to be set only at launch time (via command line arugment -Djava.security.manager)?

Whether to allow a SecurityManager to be installed later is determined at system initialization time, so there is no use to set it to "allow" inside a program. See

String smProp = System.getProperty("java.security.manager");

The spec in SecurityManager.java uses the words "if the Java virtual machine is started with the java.security.manager system property..." to describe this.

@jaikiran
Copy link
Member

@jaikiran jaikiran commented Aug 23, 2021

Would this then allow the security manager to be used? In other words, can the value of java.security.manager be changed dynamically at runtime or is it restricted to be set only at launch time (via command line arugment -Djava.security.manager)?

Whether to allow a SecurityManager to be installed later is determined at system initialization time, so there is no use to set it to "allow" inside a program. See

String smProp = System.getProperty("java.security.manager");

The spec in SecurityManager.java uses the words "if the Java virtual machine is started with the java.security.manager system property..." to describe this.

Thank you for that clarification @wangweij.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Aug 30, 2021

In the class description of java/lang/SecurityManager I think it would be useful to add a couple of sub-sections,

  1. Setting a Security Manager just before the paragraph that starts with "Environments using a security manager will typically set the security manager at startup." and ends with "The current security manager is returned by the getSecurityManager method."

  2. Checking permissions which starts after the section above and continues to the end.

The reason I think this is useful is that you can then add a link from System.setSecurityManager to the subsection on Setting a Security Manager as I think it will be useful to link those together. The best place for that link is probably in the @implNote where it describes the JDK behavior for the java.security.manager system property.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Aug 31, 2021

New commit pushed. Sections added.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 28, 2021

@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Sep 28, 2021

@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Still waiting for jtreg upgraded to 6.1.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 21, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

Going to push as commit d589b66.
Since your change was applied there have been 769 commits pushed to the master branch:

  • e39bdc9: 8274714: Incorrect verifier protected access error message
  • 45ce06c: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST
  • 60cb27d: 8275426: PretouchTask num_chunks calculation can overflow
  • cd07b3c: 8257534: misc tests failed with "NoClassDefFoundError: Could not initialize class java.util.concurrent.ThreadLocalRandom"
  • 819d2df: 8274794: Print all owned locks in hs_err file
  • c41ce6d: 8275415: Prepare Leak Profiler for Lilliput
  • 0c3eaea: 8168388: GetMousePositionTest fails with the message "Mouse position should not be null"
  • 09f5235: 8275405: Linking error for classes with lambda template parameters and virtual functions
  • a120937: 8274988: G1: refine G1SegmentedArrayAllocOptions and G1CardSetAllocOptions
  • c7a80e6: 8275607: G1: G1CardSetAllocator::drop_all needs to call G1SegmentedArray::drop_all
  • ... and 759 more: https://git.openjdk.java.net/jdk/compare/2ed7b709a197f009632580b17e3b1df34c1ffeb7...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 21, 2021
@openjdk openjdk bot added the integrated label Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@wangweij Pushed as commit d589b66.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@wangweij wangweij deleted the 8270380 branch Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment