On Thu, 28 Mar 2024 14:12:08 GMT, Andrew Dinn <ad...@openjdk.org> wrote:
>>> It looks like the test only inspects a thread and a java object. Perhaps >>> you could add tests for additional VM objects. Maybe grab a frame PC from a >>> thread stack trace. >> >> Yes - added a couple of metadata tests, and a compiled method test, which >> gets an address from Events info. We know that should resolve to a compiled >> method (if we have such an event, so this copes if there are none). >> >> >> Also the VM.info and Thread.print runs with the CommandExecutor are now >> silent. They are long, and max out the test log file and make it truncate. >> That output could mainly be useful if the regexes can't match items, but >> the output format should be reproducible in a new run. Also if we fail to >> find something we need, like a thread id, it will print the full output it >> searched. > > @kevinjwalls Hi Kevin. Sorry to offer a drive-by comment but I have been > watching this thread and I can understand why @tstuefe is expressing his > concern about the potential for security issues when it comes to using jcmd > to expose JVM internals. > > Exposure of details like a thread/stack/metadata value address or a datum > embedded at such a location does look to me like something bad agents might > be able to profit from. The danger is not just being able to retrieve > specific details of the layout or content of JVM structures themselves, but > the opportunity to use that knowledge to upgrade a weak security hole like, > say, a memory exposure, into a stronger targeted attack that knows where or > to what it might want to apply the crowbar. > > So, first off, please understand that I am not suggesting there *is* a > problem here. I am happy to accept your conclusion that your proposed jcmd > changes do not expose new data to users who ought not to be able to view > either via local or via remote accesses. However, not being an expert when it > comes to the jcmd/DCmd implementation, I'm not sure I really understand why > that is so from your current explanation. In particular I don't understand > what you said about visibility of different commands for local and remote > access. > > That confession of my own ignorance is not really of any immediate importance > as I have no intention of trying to review this change. However, I still feel > it might be useful if you could summarize on this JIRA precisely what > security safeguards are currently in place when it comes to running specific > jcmds/DCmds and why that means exposure of JVM internal details via jcmd, > whether locally or remotely via JMX, will only be to a safe audience of > users. That would help any maintainer/developer who refers to this JIRA to > follow how those safeguards apply to the proposed commands (in particular it > might be of great help to those performing and assessing the correctness of > backports). However, it would probably also as a prophylactic to ensure that > any future development work does not inadvertently lead to unexpected > exposure, which I think is the thing Thomas is more worried about. Indeed, as > Thomas suggested, a clear statement of what policies and mechanisms are (or > should be) in plac e to ensure jcmd content is securely viewable might be a good starting point for you and/or Thomas to raise follow-up JIRAs if needed to: > 1. add comments to the code base to ensure devs to not inadvertently add or > modify new DCmds ... Hi @adinn Drive-bys are welcome. 8-) Providing new crowbars to attackers is not something we want to do. jcmd is protected by the attach api and is not open to other users. We are trusting in that. If you can satisfy the attach API connection, you have all the jcmds/DiagnosticCommands available. No limits. Likewise you can use other tools to examine abritrary memory in the target process, take a gcore, kill it, etc... Why will VM.inspect not be available remotely? Because this marks the jcmd as "hidden": src/hotspot/share/services/diagnosticCommand.cpp: DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<VMInspectDCmd>(full_export, true, true /* hidden */ )); (The new test for VM.inspect checks it does not appear in the jcmd help output, to ensure it stays hidden.) In JMX, DiagnosticCommandMBean(Impl) does not expose hidden commands: The route for that is DiagnosticCommandImpl.java calling its native getDiagnosticCommands(), that gets into management.cpp's jmm_GetDiagnosticCommands() calling DCmdFactory::DCmd_list(). In diagnosticFramework.cpp, this iterates DCmdFactories, choosing those which are not hidden. Querying over JMX I cannot see or access vmInspect, like I can see vmInfo, gcClassHistogram etc... through the exposed DiagnosticCommandMBean. That "hidden" flag could be more obvious or better documented perhaps. Also I don't think we actually say in the DiagnosticCommandMBean API docs that it provides access to the DiagnosticCommand implementations which are also exposed by the jcmd tool. These concerns could be addressed in a separate PR if desired. (As well as the hidden flag, we have flags to implement different categories of diagnostic command. these are not widely used and I think the hidden flag is a better fit, as it keeps it out of the jcmd general help. This command should be documented in other guides in due course.). ------------- PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2025665216
