jbonofre opened a new pull request, #2878:
URL: https://github.com/apache/karaf/pull/2878

   ## Summary
   
   `InstanceServiceImpl` builds the child-instance launch command by 
concatenating the caller-supplied `javaOpts` unquoted into a single string that 
is ultimately executed via `/bin/sh` (`ScriptUtils`/`ProcessImpl`). A 
`javaOpts` value containing shell metacharacters (`;`, `|`, `` ` ``, `$(...)`, 
etc.) is therefore interpreted by the shell rather than passed to the JVM as an 
option, allowing arbitrary OS command execution as the Karaf process user. This 
is reachable through `instance:create`, `instance:start`, `instance:restart`, 
`instance:change-opts`, and the equivalent `InstancesMBean` JMX operations.
   
   Separately, the `instance` shell scope ships with no 
`org.apache.karaf.command.acl.instance.cfg` at all (unlike 
`bundle`/`config`/`feature`/`jaas`/`kar`/`scope_bundle`/`shell`/`system`), so 
with no ACL configured for that scope, any authenticated shell user — not just 
`admin` — can invoke `instance:*` commands by default.
   
   ## Changes
   
   - `InstanceServiceImpl`: reject any `javaOpts` value that isn't valid 
JVM-option syntax (letters, digits, whitespace, and `_-+.,:=/@*`) before it is 
used to build the launch command line, in both `doStart()` and the self-restart 
branch of `restartInstance()`. This closes the injection at the two points 
where the shell command string is actually constructed, independent of which 
caller supplied the value.
   - Add `org.apache.karaf.command.acl.instance.cfg` (new resource, wired into 
the child-instance scaffolding copy list) and the matching `<config 
name="org.apache.karaf.command.acl.instance">` block in 
`assemblies/features/standard/src/main/feature/feature.xml`, so the `instance` 
scope is no longer unauthenticated-by-default. State-changing operations 
require `admin`; `list`/`status` require `viewer`.
   
   ## Test plan
   
   - [x] `mvn -pl instance,util test` passes, including tests that launch a 
real JVM subprocess through `doStart`/`ProcessImpl` with legitimate `javaOpts` 
(e.g. `-Dprop="key"`), confirming the validation doesn't reject normal usage.
   - [x] Verified `assemblies/features/standard/src/main/feature/feature.xml` 
is still well-formed XML.
   - [ ] Manual check recommended: `instance:create -o '-Xmx64m; id' x` now 
throws `IllegalArgumentException` instead of spawning a shell.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to