fix: stop scenario metadata from reaching shells and host commands

AGENTS.md

@@ -130,6 +130,7 @@ Most environment variables are read in
 | `PHENIX_DIR`, `PHENIX_TEMP_DIR` | phēnix data and scratch directories |
 | `MM_FILEPATH`, `MM_SOCKET_PATH` | minimega file root and command socket |
 | `PHENIX_CC_*` | miniccc polling rates and timeout grace periods |
+| `PHENIX_SCORCH_HOST_VALIDATORS` | Literal `1` lets the SCORCH `art` and `cc` validators and the `pipe` `via` run scenario-supplied commands on the host; off otherwise |
 
 Read elsewhere: `PHENIX_DRYRUN` (literal `true`) in
 `src/python/phenix_apps/apps/__init__.py`,

CHANGELOG.md

@@ -36,6 +36,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **SCEPTRE App**: A `fep` without a mgmt interface raised `UnboundLocalError`, or reused the previous fep's endpoints.
 - **SCEPTRE App**: A historian on a subnet with no OPC server was configured with an unrelated OPC's tag list and no address to collect from. It now gets no tags and a warning naming the subnet.
 
+### Security
+- **Common**: `run_command()` no longer uses a shell; `mm_compute_cmd()` rejects multi-token names.
+- **Mirror, mgmt_tap, SCORCH**: Metadata is validated before reaching `ovs-vsctl`, minimega or `tshark`; commands run as argument vectors, not `bash -c`.
+- **SCORCH**: kafka's PID file leaves `/tmp`; ssh takes optional `known_hosts`.
+- **SCORCH art/cc/pipe**: **Breaking:** host-side commands need `PHENIX_SCORCH_HOST_VALIDATORS=1`.
+
 ## [2.0.0] - 2026-03-04
 
 ### Changed

src/go/cmd/phenix-app-mirror/util.go

@@ -2,12 +2,34 @@ package main
 
 import (
 	"fmt"
+	"regexp"
+	"strings"
 
 	"github.com/mitchellh/mapstructure"
 
 	"phenix-apps/util"
 )
 
+// ovsTokenRegex matches a single safe token for interpolation into ovs-vsctl
+// commands run on cluster hosts (bridge names, VLAN aliases, interface names).
+var ovsTokenRegex = regexp.MustCompile(`^[A-Za-z0-9_.-]{1,15}$`)
+
+// validateOVSToken ensures a metadata-provided value is a single, safe token
+// before it gets interpolated into an ovs-vsctl command. It explicitly rejects
+// whitespace and the `--` command separator on top of the allowlist regex.
+func validateOVSToken(field, value string) error {
+	if value == "--" || strings.ContainsAny(value, " \t\r\n") || !ovsTokenRegex.MatchString(value) {
+		return fmt.Errorf(
+			"invalid %s %q in mirror app metadata: must match %s and must not be `--`",
+			field,
+			value,
+			ovsTokenRegex,
+		)
+	}
+
+	return nil
+}
+
 func extractMetadata(data map[string]any) (MirrorAppMetadataV1, error) {
 	var (
 		amd     MirrorAppMetadataV1
@@ -31,6 +53,22 @@ func extractMetadata(data map[string]any) (MirrorAppMetadataV1, error) {
 		}
 	}
 
+	// Empty values are allowed here: defaults are applied after decoding (the
+	// experiment default bridge and the `mirror` VLAN alias).
+	if amd.MirrorBridge != "" {
+		err := validateOVSToken("mirrorBridge", amd.MirrorBridge)
+		if err != nil {
+			return amd, err
+		}
+	}
+
+	if amd.MirrorVLAN != "" {
+		err := validateOVSToken("mirrorVLAN", amd.MirrorVLAN)
+		if err != nil {
+			return amd, err
+		}
+	}
+
 	return amd, nil
 }
 
@@ -49,5 +87,12 @@ func extractHostMetadata(data map[string]any) (MirrorHostMetadata, error) {
 		}
 	}
 
+	for _, hil := range hmd.HIL {
+		err := validateOVSToken("hilInterfaces entry", hil)
+		if err != nil {
+			return hmd, err
+		}
+	}
+
 	return hmd, nil
 }

src/go/cmd/phenix-app-mirror/util_test.go

@@ -0,0 +1,37 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateOVSToken(t *testing.T) {
+	t.Parallel()
+
+	for _, value := range []string{"phenix", "br-0", "eth1.100", "mirror", "a_b", "abcdefghijklmno"} {
+		require.NoError(t, validateOVSToken("field", value), value)
+	}
+
+	for _, value := range []string{"", "--", "two words", "tab\there", "new\nline", "br0;reboot", "abcdefghijklmnop", "$(id)"} {
+		require.Error(t, validateOVSToken("field", value), value)
+	}
+}
+
+func TestExtractMetadataRejectsUnsafeTokens(t *testing.T) {
+	t.Parallel()
+
+	_, err := extractMetadata(map[string]any{"version": "v1", "mirrorBridge": "phenix -- del-br phenix"})
+	require.Error(t, err)
+
+	_, err = extractMetadata(map[string]any{"version": "v1", "mirrorVLAN": "mirror;id"})
+	require.Error(t, err)
+
+	amd, err := extractMetadata(map[string]any{"version": "v1", "mirrorBridge": "phenix", "mirrorVLAN": "mirror"})
+	require.NoError(t, err)
+	assert.Equal(t, "phenix", amd.MirrorBridge)
+
+	_, err = extractHostMetadata(map[string]any{"hilInterfaces": []any{"eth1", "eth2 eth3"}})
+	require.Error(t, err)
+}

src/python/phenix_apps/apps/mgmt_tap/app.py

@@ -1,9 +1,11 @@
 import ipaddress
+import re
 import socket
 
 import minimega
 
 from phenix_apps.apps import AppBase
+from phenix_apps.common.error import AppError
 from phenix_apps.common.logger import logger
 from phenix_apps.common.utils import _mm_init, mm_compute_cmd, mm_host_info
 
@@ -26,6 +28,10 @@ class MgmtTap(AppBase):
 
     DEFAULT_BRIDGE = "phenix"
 
+    # OVS bridge names are single tokens of at most 15 characters; anything
+    # else would end up interpolated into minimega tap commands.
+    BRIDGE_NAME_REGEX = re.compile(r"[A-Za-z0-9_.-]{1,15}")
+
     def __init__(self, name: str, stage: str, dryrun: bool = False) -> None:
         super().__init__(name, stage, dryrun)
         # Check if subnet and namespace is specified in app metadata
@@ -57,16 +63,25 @@ def _get_bridge(self) -> str:
         bridge = self.metadata.get("bridge", None) if self.metadata else None
         if bridge:
             logger.debug(f"Using bridge '{bridge}' from app metadata")
-            return bridge
+            return self._validate_bridge(bridge)
 
         bridge = self.experiment.spec.get("defaultBridge", None)
         if bridge:
             logger.debug(f"Using experiment default bridge '{bridge}'")
-            return bridge
+            return self._validate_bridge(bridge)
 
         logger.debug(f"Falling back to bridge '{self.DEFAULT_BRIDGE}'")
         return self.DEFAULT_BRIDGE
 
+    def _validate_bridge(self, bridge: str) -> str:
+        """Reject bridge names that are not a single, valid OVS bridge token."""
+        if not self.BRIDGE_NAME_REGEX.fullmatch(bridge):
+            raise AppError(
+                f"invalid bridge name '{bridge}': must match "
+                f"'^{self.BRIDGE_NAME_REGEX.pattern}$'"
+            )
+        return bridge
+
     def _get_mm_connection(self) -> minimega.minimega:
         """Get or create minimega connection."""
         if self._mm is None:

src/python/phenix_apps/apps/scorch/art/README.md

@@ -83,6 +83,12 @@ If exit non-zero and `abortOnError` is true, then the component exits as failed.
 If no validator is provided then it is assumed the test succeeded if the atomic
 executor exits cleanly.
 
+> [!IMPORTANT]
+> Validators run as shell scripts on the phenix host, not in the VM, so they are
+> off by default. Set `PHENIX_SCORCH_HOST_VALIDATORS=1` in the phenix
+> environment to run them; otherwise each validator is skipped with a warning
+> and the result is not validated.
+
 `vms` is a list of settings per VM to execute the test on.
 
 ## VM Settings

src/python/phenix_apps/apps/scorch/art/art.py

@@ -1,13 +1,14 @@
-import os
 import subprocess
 import time
 import uuid
+from pathlib import Path
 
 from box import Box
 
 from phenix_apps.apps.scorch import ComponentBase
 from phenix_apps.common import utils
 from phenix_apps.common.logger import logger
+from phenix_apps.common.settings import SCORCH_HOST_VALIDATORS
 
 # This can be changed here to reflect your directory structure on hosts as a default.
 # This is overwritten if a value is provided via goartPath
@@ -102,34 +103,39 @@ def start(self):
             utils.mm_exec_wait(mm, hostname, cmd)
             time.sleep(5)
             logger.info(f"retrieving results: {out_file}")
-            results_file = os.path.join(self.base_dir, f"{hostname}.json")
+            results_file = str(Path(self.base_dir) / f"{hostname}.json")
 
             try:
                 utils.mm_recv(mm, hostname, out_file, results_file)
                 logger.info(f"results_file path: {results_file}")
-                logger.info(f"results_file exists: {os.path.exists(results_file)}")
+                logger.info(f"results_file exists: {Path(results_file).exists()}")
             except Exception as ex:
                 raise RuntimeError(
                     f"failed to get results file from {hostname}: {ex}"
                 ) from ex
 
             validator = self.metadata.get("validator", None)
-            if validator:
+            if validator and not SCORCH_HOST_VALIDATORS:
+                logger.warning(
+                    f"skipping host-side validator for {hostname}: set "
+                    "PHENIX_SCORCH_HOST_VALIDATORS=1 to allow validators to run on the host"
+                )
+            elif validator:
                 logger.info(f"validating results from {hostname}")
 
-                tempfile = f"/tmp/{uuid.uuid4()!s}.sh"
-                with open(tempfile, "w") as tf:
+                tempfile = Path(f"/tmp/{uuid.uuid4()!s}.sh")
+                with tempfile.open("w") as tf:
                     tf.write(validator)
 
                 results = Box.from_json(filename=results_file)
 
                 proc = subprocess.run(
-                    ["sh", tempfile, hostname],
+                    ["sh", str(tempfile), hostname],
                     input=results.Executor.ExecutedCommand.results.encode(),
                     capture_output=True,
                 )
 
-                os.remove(tempfile)
+                tempfile.unlink()
 
                 if proc.returncode != 0:
                     stderr = proc.stderr.decode()

src/python/phenix_apps/apps/scorch/cc/README.md

@@ -38,6 +38,12 @@ metadata:
 > validator script should be written to process STDIN. Anything the validator
 > script writes to STDERR will be available to the user if the validation fails.
 
+> [!IMPORTANT]
+> Validators run as shell scripts on the phenix host, not in the VM, so they are
+> off by default. Set `PHENIX_SCORCH_HOST_VALIDATORS=1` in the phenix
+> environment to run them; otherwise each validator is skipped with a warning
+> and the result is not validated.
+
 ## Types
 - VM-specific command types
   - `exec`: execute a command (`cc exec`)

src/python/phenix_apps/apps/scorch/cc/cc.py

@@ -5,6 +5,7 @@
 from phenix_apps.apps.scorch import ComponentBase
 from phenix_apps.common import utils
 from phenix_apps.common.logger import logger
+from phenix_apps.common.settings import SCORCH_HOST_VALIDATORS
 
 
 class CC(ComponentBase):
@@ -112,7 +113,12 @@ def __run(self, stage: str) -> None:
                         if results["stdout"]:
                             logger.info(f"STDOUT Output: {results['stdout']}")
 
-                        if validator:
+                        if validator and not SCORCH_HOST_VALIDATORS:
+                            logger.warning(
+                                f"skipping host-side validator for command '{cmd.args}' on VM {vm.hostname}: "
+                                "set PHENIX_SCORCH_HOST_VALIDATORS=1 to allow validators to run on the host"
+                            )
+                        elif validator:
                             logger.info(f"validating results from '{cmd.args}'")
 
                             tempfile = f"/tmp/{uuid.uuid4()!s}.sh"

src/python/phenix_apps/apps/scorch/collector/collector.py

@@ -6,7 +6,7 @@
 import yaml
 
 from phenix_apps.apps.scorch import ComponentBase
-from phenix_apps.common import utils
+from phenix_apps.common import error, utils
 from phenix_apps.common.logger import logger
 
 from .csv_gen import gen_csv
@@ -63,9 +63,21 @@ def stop(self):
         sceptre_topo = self.experiment.metadata.annotations.topology
         sceptre_scenario = self.experiment.metadata.annotations.scenario
 
-        topo_data = utils.run_command(f"phenix config get topology/{sceptre_topo}")
-        sc_data = utils.run_command(f"phenix config get scenario/{sceptre_scenario}")
-        exp_data = utils.run_command(f"phenix config get experiment/{self.exp_name}")
+        for annotation in (sceptre_topo, sceptre_scenario):
+            if not annotation or len(annotation.split()) != 1:
+                raise error.AppError(
+                    f"invalid experiment annotation value '{annotation}'"
+                )
+
+        topo_data = utils.run_command(
+            ["phenix", "config", "get", f"topology/{sceptre_topo}"]
+        )
+        sc_data = utils.run_command(
+            ["phenix", "config", "get", f"scenario/{sceptre_scenario}"]
+        )
+        exp_data = utils.run_command(
+            ["phenix", "config", "get", f"experiment/{self.exp_name}"]
+        )
         assert topo_data
         assert sc_data
         assert exp_data