diff --git a/SECURITY.md b/SECURITY.md index 53c9441..1d90206 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -64,6 +64,11 @@ reporters and reviewers have context: - Shell-outs (`apt list --upgradable`, optional `vcgencmd measure_temp`) are invoked with fixed argument lists (no user input is interpolated into shell commands), to avoid command injection. +- Shell-outs are also invoked with an explicit, minimal environment rather + than inheriting the service's own — neither `apt` nor `vcgencmd` needs + anything beyond `PATH` (and, for `apt`, a couple of apt-specific + variables), so `PIMONITOR_API_KEY` is never copied into a child process's + `/proc//environ`. If you believe any of these assumptions are violated by the current implementation, please report it as described above. diff --git a/internal/collector/vcgencmd.go b/internal/collector/vcgencmd.go index 4370696..6314290 100644 --- a/internal/collector/vcgencmd.go +++ b/internal/collector/vcgencmd.go @@ -86,7 +86,15 @@ func (r *vcgencmdRunner) run(ctx context.Context, subcommand string) (string, er ctx, cancel := context.WithTimeout(ctx, vcgencmdTimeout) defer cancel() - out, err := exec.CommandContext(ctx, path, subcommand).Output() + cmd := exec.CommandContext(ctx, path, subcommand) + // Build the child environment explicitly rather than inheriting the + // service's: PiMonitor's own environment may carry PIMONITOR_API_KEY, and + // there is no reason for that secret to be visible in vcgencmd's + // /proc//environ. path is already the absolute location resolved by + // exec.LookPath at detection time, so no PATH is needed either. + cmd.Env = []string{} + + out, err := cmd.Output() if err != nil { return "", fmt.Errorf("run vcgencmd %s: %w", subcommand, err) } diff --git a/internal/collector/vcgencmd_test.go b/internal/collector/vcgencmd_test.go index d9b3fc5..ccb8a6b 100644 --- a/internal/collector/vcgencmd_test.go +++ b/internal/collector/vcgencmd_test.go @@ -3,7 +3,11 @@ package collector import ( "context" "errors" + "fmt" + "os" + "path/filepath" "runtime" + "strings" "testing" "time" ) @@ -88,6 +92,34 @@ func TestVcgencmdRunner_Run_CommandFails(t *testing.T) { } } +func TestVcgencmdRunner_Run_DoesNotLeakParentEnvironment(t *testing.T) { + t.Setenv("PIMONITOR_API_KEY", "test-secret-should-not-leak") + + dir := t.TempDir() + envFile := filepath.Join(dir, "env.out") + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", fmt.Sprintf("env > %q", envFile)) + r := &vcgencmdRunner{detected: true, path: path} + + if _, err := r.run(context.Background(), "measure_temp"); err != nil { + t.Fatalf("run: %v", err) + } + + out, err := os.ReadFile(envFile) + if err != nil { + t.Fatalf("read captured child environment: %v", err) + } + if strings.Contains(string(out), "PIMONITOR_API_KEY") { + t.Fatalf("child environment leaked PIMONITOR_API_KEY: %q", out) + } + // PATH is not required: path is already the absolute location resolved + // by exec.LookPath at detection time, so its absence here confirms the + // environment was built explicitly rather than inherited (PWD is set by + // the shell itself, not inherited, so its presence is expected). + if strings.Contains(string(out), "PATH=") { + t.Fatalf("child environment unexpectedly contains PATH: %q", out) + } +} + func TestNewVcgencmdRunner_NilClockDefaultsToTimeNow(t *testing.T) { r := newVcgencmdRunner(nil)