8272317: jstatd has dependency on Security Manager which needs to be removed #6919

kevinjwalls · 2021-12-22T18:14:43Z

Remove the use of Security Manager from jstatd.
Add use of an ObjectInputFilter to restrict RMI.

Also we can undo the property-setting Launcher.gmk change from: 8279007: jstatd fails to start because SecurityManager is disabled
..as that is no longer needed.

Docs/man page update to follow (JDK-8278619).

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8272317: jstatd has dependency on Security Manager which needs to be removed
JDK-8279891: jstatd has dependency on Security Manager which needs to be removed (CSR)

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to e49b9fe
Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to e49b9fe

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6919

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

Using diff file

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

…removed

…cmgr

kevinjwalls · 2021-12-22T18:15:46Z

/label serviceability

bridgekeeper · 2021-12-22T18:15:47Z

👋 Welcome back kevinw! 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 · 2021-12-22T18:18:28Z

@kevinjwalls
The serviceability label was successfully added.

mlbridge · 2021-12-22T18:22:26Z

Webrevs

mlbridge · 2021-12-22T19:11:37Z

Mailing list message from Bernd Eckenfels on serviceability-dev:

Hello,

Is it safe to allow generic proxy objects, could they not execute arbritrary backend methods? Are the invocation handlers filtered indirectly?

What about those inner classes, are they stable? Could the whole protocol maybe changed to a different protocol?

Gruss
Bernd

--
http://bernd.eckenfels.net
________________________________
Von: serviceability-dev <serviceability-dev-retn at openjdk.java.net> im Auftrag von Kevin Walls <kevinw at openjdk.java.net>
Gesendet: Wednesday, December 22, 2021 7:22:26 PM
An: serviceability-dev at openjdk.java.net <serviceability-dev at openjdk.java.net>
Betreff: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed

Remove the use of Security Manager from jstatd.
Add use of an ObjectInputFilter to restrict RMI.

Also we can undo the property-setting Launcher.gmk change from: 8279007: jstatd fails to start because SecurityManager is disabled
..as that is no longer needed.

Docs/man page update to follow (JDK-8278619).

-------------

Commit messages:
- Remove jstad launcher property setting to allow Security Manager.
- Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr
- Add ObjectInputFilter
- Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr
- 8272317: jstatd has dependency on Security Manager which needs to be removed

Changes: https://git.openjdk.java.net/jdk/pull/6919/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6919&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272317
Stats: 27 lines in 4 files changed: 4 ins; 15 del; 8 mod
Patch: https://git.openjdk.java.net/jdk/pull/6919.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6919/head:pull/6919

PR: https://git.openjdk.java.net/jdk/pull/6919
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20211222/dfe85dab/attachment-0001.htm>

mlchung · 2021-12-22T21:41:13Z

src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java

@@ -47,6 +48,8 @@
    private static boolean startRegistry = true;
    private static RemoteHost remoteHost;

+    private static final String rmiFilterPattern = "sun.jvmstat.monitor.remote.RemoteVm;com.sun.proxy.jdk.proxy1.$Proxy1;com.sun.proxy.jdk.proxy1.$Proxy2;java.lang.reflect.Proxy;java.rmi.server.RemoteObjectInvocationHandler;java.rmi.server.RemoteObject;!*";


The class name of the dynamic proxy is generated at runtime and can be different. As Bernd commented, the proxy classes cannot/should not be listed in the filter pattern.

OK thanks - I was trying the minimal pattern to overcome rejections such as the following, captured in logs on different runs:

ObjectInputFilter REJECTED: class com.sun.proxy.jdk.proxy1.$Proxy1, array length: -1, nRefs: 2, depth: 1, bytes: 84, ex: n/a

ObjectInputFilter REJECTED: class com.sun.proxy.jdk.proxy1.$Proxy2, array length: -1, nRefs: 2, depth: 1, bytes: 84, ex: n/a

I think the proxy classes need to be there. The RemoteHost API has a parameter of type RemoteVm which is a stub to an RMI remote object, which consists of a proxy and a handler. The proxy's interface list is filtered by the serialization filter so somebody can't just pass a proxy for anything.

The name of the proxy class probably does need to be wildcarded though.

@mlchung The Proxy class passed to the filter has been created in this VM from the interfaces listed.
The interfaces have already been filtered prior to creating the proxy.
The Proxy classes can safely be allowed based on a wildcard of the name. (As Stuart said).

seanjmullan · 2021-12-23T14:17:37Z

/label security

openjdk · 2021-12-23T14:18:31Z

@seanjmullan
The security label was successfully added.

kevinjwalls · 2022-01-07T20:25:46Z

Thanks for the comments -

The proxy objects are needed in the filter for this to work at all.

The proxy names/numbers and innner class names/numbers are predictable and stable IF we are using jstatd and jstat as standalone programs.

But they are unstable if there is other relevant activity in the VM process, e.g. a JMX connection comes in before they are created. We should wildcard the proxy names to work in such a situation: com.sun.proxy.jdk.proxy*

I'll mention also that jstatd has always been an experimental feature. The man page warns about lack of authentication and advises usage with caution.

…n this JVM changes the nameing/numbering of proxy classes.

kevinjwalls · 2022-01-19T12:51:26Z

CSR has been approved (https://bugs.openjdk.java.net/browse/JDK-8279891)

mlchung · 2022-01-19T19:56:53Z

Are all the proxy interfaces public? The package in which a proxy class is created may be different depending if whether any proxy interface is in a non-exported and non-open package. com.sun.proxy.jdk.proxy* is the package for non-exported proxy classes. The proxy classes may be public in an unconditionally exported package [1] and its package name is jdk.proxy*.

[1] https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/reflect/Proxy.html#membership

kevinjwalls · 2022-01-20T09:05:36Z

Are all the proxy interfaces public?

sun.jvmstat.monitor.remote.RemoteVm is "public interface RemoteVm extends Remote" and methods in there only return basic types.
This is in the jdk.jstatd module, where I see the module info contains "exports sun.jvmstat.monitor.remote to java.rmi;"

The only other names permitted are proxy/reflect/rmi related.

mlchung · 2022-01-20T16:54:21Z

If sun.jvmstat.monitor.remote.RemoteVm is the only proxy interface, com.sun.proxy.jdk.proxy* should adequately cover the proxy classes created for RemoteVm.

kevinjwalls · 2022-01-20T20:34:30Z

If sun.jvmstat.monitor.remote.RemoteVm is the only proxy interface, com.sun.proxy.jdk.proxy* should adequately cover the proxy classes created for RemoteVm.

Thanks. With that endorsement I think there are no unresolved issues with this change.

openjdk · 2022-01-27T17:38:46Z

@kevinjwalls 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:

8272317: jstatd has dependency on Security Manager which needs to be removed

Reviewed-by: cjplummer, 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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

RogerRiggs

LGTM

test/jdk/sun/tools/jstatd/JstatdTest.java

kevinjwalls · 2022-01-27T18:24:07Z

Many thanks Chris, Roger!

…cmgr

kevinjwalls · 2022-01-28T12:53:29Z

/integrate

openjdk · 2022-01-28T12:54:29Z

Going to push as commit cb8a82e.
Since your change was applied there has been 1 commit pushed to the master branch:

6de90ad: 8280863: Update build README to reflect that MSYS2 is supported

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-28T12:54:44Z

@kevinjwalls Pushed as commit cb8a82e.

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

kevinjwalls added 5 commits December 13, 2021 15:52

8272317: jstatd has dependency on Security Manager which needs to be …

bdbad17

…removed

Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_se…

0ca4bd2

…cmgr

Add ObjectInputFilter

45c84ac

Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_se…

9c344fd

…cmgr

Remove jstad launcher property setting to allow Security Manager.

25ed85e

openjdk bot added rfr Pull request is ready for review serviceability serviceability-dev@openjdk.org labels Dec 22, 2021

mlchung reviewed Dec 22, 2021

View reviewed changes

openjdk bot added the security security-dev@openjdk.org label Dec 23, 2021

Wildcard in object filter to permit proxies, in case other activity i…

e49b9fe

…n this JVM changes the nameing/numbering of proxy classes.

openjdk bot added csr Pull request needs approved CSR before integration and removed csr Pull request needs approved CSR before integration labels Jan 12, 2022

plummercj approved these changes Jan 27, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 27, 2022

RogerRiggs approved these changes Jan 27, 2022

View reviewed changes

test/jdk/sun/tools/jstatd/JstatdTest.java Outdated Show resolved Hide resolved

Copyright update

443b18b

Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_se…

e6f016a

…cmgr

openjdk bot added the integrated Pull request has been integrated label Jan 28, 2022

openjdk bot closed this Jan 28, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8272317: jstatd has dependency on Security Manager which needs to be removed #6919

8272317: jstatd has dependency on Security Manager which needs to be removed #6919

kevinjwalls commented Dec 22, 2021 •

edited by openjdk bot

Loading

kevinjwalls commented Dec 22, 2021

bridgekeeper bot commented Dec 22, 2021

openjdk bot commented Dec 22, 2021

mlbridge bot commented Dec 22, 2021 •

edited

Loading

mlbridge bot commented Dec 22, 2021

mlchung Dec 22, 2021

kevinjwalls Dec 22, 2021

stuart-marks Dec 24, 2021

RogerRiggs Jan 7, 2022

seanjmullan commented Dec 23, 2021

openjdk bot commented Dec 23, 2021

kevinjwalls commented Jan 7, 2022

kevinjwalls commented Jan 19, 2022

mlchung commented Jan 19, 2022

kevinjwalls commented Jan 20, 2022

mlchung commented Jan 20, 2022

kevinjwalls commented Jan 20, 2022

openjdk bot commented Jan 27, 2022 •

edited

Loading

RogerRiggs left a comment

kevinjwalls commented Jan 27, 2022

kevinjwalls commented Jan 28, 2022

openjdk bot commented Jan 28, 2022

openjdk bot commented Jan 28, 2022

8272317: jstatd has dependency on Security Manager which needs to be removed #6919

8272317: jstatd has dependency on Security Manager which needs to be removed #6919

Conversation

kevinjwalls commented Dec 22, 2021 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

kevinjwalls commented Dec 22, 2021

bridgekeeper bot commented Dec 22, 2021

openjdk bot commented Dec 22, 2021

mlbridge bot commented Dec 22, 2021 • edited Loading

Webrevs

mlbridge bot commented Dec 22, 2021

mlchung Dec 22, 2021

Choose a reason for hiding this comment

kevinjwalls Dec 22, 2021

Choose a reason for hiding this comment

stuart-marks Dec 24, 2021

Choose a reason for hiding this comment

RogerRiggs Jan 7, 2022

Choose a reason for hiding this comment

seanjmullan commented Dec 23, 2021

openjdk bot commented Dec 23, 2021

kevinjwalls commented Jan 7, 2022

kevinjwalls commented Jan 19, 2022

mlchung commented Jan 19, 2022

kevinjwalls commented Jan 20, 2022

mlchung commented Jan 20, 2022

kevinjwalls commented Jan 20, 2022

openjdk bot commented Jan 27, 2022 • edited Loading

RogerRiggs left a comment

Choose a reason for hiding this comment

kevinjwalls commented Jan 27, 2022

kevinjwalls commented Jan 28, 2022

openjdk bot commented Jan 28, 2022

openjdk bot commented Jan 28, 2022

kevinjwalls commented Dec 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 22, 2021 •

edited

Loading

openjdk bot commented Jan 27, 2022 •

edited

Loading