diff --git a/internal/apply/process.go b/internal/apply/process.go index 12e833e..686b7f1 100644 --- a/internal/apply/process.go +++ b/internal/apply/process.go @@ -203,7 +203,7 @@ func unitFor(r *declaration.Process) string { fmt.Fprintf(&b, "EnvironmentFile=%s\n", file) } for _, key := range sortedKeys(r.Env) { - fmt.Fprintf(&b, "Environment=%s=%s\n", key, r.Env[key]) + fmt.Fprintf(&b, "Environment=%s\n", unitValue(key, r.Env[key])) } if r.User != "" { fmt.Fprintf(&b, "User=%s\n", r.User) @@ -264,3 +264,28 @@ func calendarFor(cron string) string { } return fmt.Sprintf("%s*-%s-%s %s:%s:00", day, month, dom, hour, minute) } + +// unitValue renders one environment assignment so a unit file means what the declaration said. +// +// **Three things a unit file does to a value that nothing else does**, and a module's environment +// routinely contains all three — a generated password is arbitrary bytes. +// +// - `%` begins a specifier. `%H` is the hostname, `%i` the instance. A password containing one +// is silently replaced by something else, and the failure is an authentication error nobody +// can explain by looking at the declaration. +// - whitespace separates assignments. `Environment=K=a b` sets K to "a" and then tries to read +// "b" as another assignment. +// - a newline ends the line. What follows it is read as a unit DIRECTIVE, so a value carrying +// one could write ExecStart= and have the machine run something nobody declared. +// +// Quoted, with quotes and backslashes escaped and percent doubled. A container needs none of this +// because `--env` is passed through literally, which is why this had to be found here rather than +// noticed in both. +func unitValue(key, value string) string { + escaped := strings.NewReplacer( + `\`, `\\`, + `"`, `\"`, + "%", "%%", + ).Replace(value) + return `"` + key + "=" + escaped + `"` +} diff --git a/internal/apply/process_test.go b/internal/apply/process_test.go index 1fd0826..3d38e22 100644 --- a/internal/apply/process_test.go +++ b/internal/apply/process_test.go @@ -130,3 +130,36 @@ func TestAProcessThatStaysUpIsStillRestartedWhenItExits(t *testing.T) { t.Fatalf("a process that should stay up is declared a step:\n%s", unit) } } + +// **A unit file reinterprets a value in three ways nothing else does**, and a module's environment +// routinely contains all three — a generated password is arbitrary bytes. +func TestAnEnvironmentValueMeansWhatTheDeclarationSaid(t *testing.T) { + // A percent begins a specifier: %H is the hostname. A password containing one would be + // silently replaced, failing as an authentication error nobody can explain from the + // declaration. + percent := aProcess() + percent.Env = map[string]string{"PASSWORD": "a%Hb"} + if !strings.Contains(unitFor(percent), "%%H") { + t.Fatalf("a percent was left as a systemd specifier:\n%s", unitFor(percent)) + } + + // Whitespace separates assignments: unquoted, K=a b sets K to "a" and reads "b" as another. + spaced := aProcess() + spaced.Env = map[string]string{"GREETING": "hello there"} + if !strings.Contains(unitFor(spaced), `"GREETING=hello there"`) { + t.Fatalf("a value with a space was not quoted:\n%s", unitFor(spaced)) + } + + // A quote would end the quoting early, and what follows would be read as unit syntax. + quoted := aProcess() + quoted.Env = map[string]string{"TOKEN": `a"b`} + line := "" + for _, l := range strings.Split(unitFor(quoted), "\n") { + if strings.HasPrefix(l, "Environment=") { + line = l + } + } + if !strings.Contains(line, `\"`) { + t.Fatalf("a quote was not escaped, so the value ends early: %s", line) + } +} diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index 2c155eb..909e540 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -502,6 +502,24 @@ func (d *Process) validate(where string, _ bool) []string { break } } + // **A newline cannot be represented in a unit's environment, so it is refused rather than + // mangled.** Everything else a unit file reinterprets — a percent specifier, whitespace + // splitting assignments, a quote ending one early — can be escaped. A newline cannot: it ends + // the line, and what follows is read as a unit DIRECTIVE. A value carrying one could write + // ExecStart= and have the machine run something nobody declared. + // + // Refused here, near whoever wrote it, rather than at the far end of a declaration. + for key, value := range d.Env { + if strings.ContainsAny(value, "\n\r") { + problems = append(problems, fmt.Sprintf( + "%s: the value of %s contains a line break, which cannot be written into a unit's "+ + "environment — what followed it would be read as a unit directive", where, key)) + } + if key == "" { + problems = append(problems, where+": an environment value with no name") + } + } + if d.Schedule != "" { if d.RunOnce { problems = append(problems, where+ diff --git a/internal/declaration/process_test.go b/internal/declaration/process_test.go index a8d6c2b..c52519f 100644 --- a/internal/declaration/process_test.go +++ b/internal/declaration/process_test.go @@ -116,3 +116,45 @@ func TestEachModeOnItsOwnIsAccepted(t *testing.T) { t.Fatalf("a scheduled process was refused: %v", problems) } } + +// **The one that cannot be escaped, only refused.** +// +// Everything else a unit file reinterprets can be escaped: a percent specifier doubled, whitespace +// quoted, a quote backslashed. A newline cannot — it ends the line, and what follows is read as a +// unit DIRECTIVE. A value carrying one could write ExecStart= and have the machine run something +// nobody declared. +// +// So it is refused here, near whoever wrote it, rather than rendered into a unit at the far end of +// a declaration. +func TestAnEnvironmentValueCannotCarryALineBreak(t *testing.T) { + for _, bad := range []string{ + "safe\nExecStart=/usr/bin/whatever", + "carriage\rreturn", + } { + p := aProcess() + p.Env = map[string]string{"X": bad} + problems := p.validate("a process", false) + if len(problems) == 0 { + t.Fatalf("a value containing %q was accepted", bad) + } + var said bool + for _, problem := range problems { + if strings.Contains(problem, "line break") { + said = true + } + } + if !said { + t.Fatalf("refused for some other reason, which would stop being true: %v", problems) + } + } +} + +// And ordinary awkward values are accepted, because escaping is what handles those — refusing them +// too would make a module unable to hold a generated password. +func TestOrdinaryAwkwardValuesAreAccepted(t *testing.T) { + p := aProcess() + p.Env = map[string]string{"PASSWORD": `a%H b"c\d`} + if problems := p.validate("a process", false); len(problems) != 0 { + t.Fatalf("a password containing the characters passwords contain was refused: %v", problems) + } +}