fix(remote): close the heredoc breakout in the bootstrap script

cli/remote/bootstrap.go

@@ -33,6 +33,18 @@ func BootstrapCommand(opts BootstrapOptions) (string, error) {
 	if opts.ClientKeyPEM == "" {
 		return "", fmt.Errorf("client key is required")
 	}
+	// rubyQuote keeps these out of Ruby's reach, but the client.rb it builds
+	// is delivered inside a shell heredoc one layer down. A newline can close
+	// that heredoc early and turn the remainder into commands run through
+	// sudo on the target, so reject it here rather than try to escape it.
+	for _, field := range []struct{ name, value string }{
+		{"node name", opts.NodeName},
+		{"server URL", opts.ServerURL},
+	} {
+		if strings.ContainsAny(field.value, "\n\r") {
+			return "", fmt.Errorf("refusing a %s containing a newline: %q", field.name, field.value)
+		}
+	}
 	if opts.BootstrapURL == "" {
 		opts.BootstrapURL = DefaultBootstrapURL
 	}
@@ -52,6 +64,18 @@ func BootstrapCommand(opts BootstrapOptions) (string, error) {
 	if opts.BootstrapVersion != "" {
 		installArgs = " -v " + shellQuote(opts.BootstrapVersion)
 	}
+	writeKey, err := writeFileCommand(prefix, "/etc/cinc/client.pem", opts.ClientKeyPEM)
+	if err != nil {
+		return "", err
+	}
+	writeClientRB, err := writeFileCommand(prefix, "/etc/cinc/client.rb", clientRB)
+	if err != nil {
+		return "", err
+	}
+	writeFirstBoot, err := writeFileCommand(prefix, "/etc/cinc/first-boot.json", string(firstBoot))
+	if err != nil {
+		return "", err
+	}
 	commands := []string{
 		"set -e",
 		// Trust assumption: this pipes the installer script straight into a
@@ -63,10 +87,10 @@ func BootstrapCommand(opts BootstrapOptions) (string, error) {
 		// world-readable on the target (tee would otherwise create it with the
 		// remote umask, typically 0644, until the chmod ran).
 		prefix + "install -m 0600 /dev/null /etc/cinc/client.pem",
-		writeFileCommand(prefix, "/etc/cinc/client.pem", opts.ClientKeyPEM),
+		writeKey,
 		prefix + "chmod 0600 /etc/cinc/client.pem",
-		writeFileCommand(prefix, "/etc/cinc/client.rb", clientRB),
-		writeFileCommand(prefix, "/etc/cinc/first-boot.json", string(firstBoot)),
+		writeClientRB,
+		writeFirstBoot,
 		prefix + "cinc-client -j /etc/cinc/first-boot.json",
 	}
 	return strings.Join(commands, "\n"), nil
@@ -111,8 +135,20 @@ func rubyQuote(s string) string {
 	return "'" + s + "'"
 }
 
-func writeFileCommand(prefix, path, content string) string {
-	return "cat <<'CINC_EOF' | " + prefix + "tee " + shellQuote(path) + " >/dev/null\n" + content + "\nCINC_EOF"
+// heredocDelimiter terminates every file the bootstrap script writes. A line
+// in the body equal to it would close the heredoc early and leave the rest of
+// the body to be executed as shell, so writeFileCommand refuses that content
+// outright. Callers validate their own inputs; this is the backstop that
+// makes the heredoc safe regardless.
+const heredocDelimiter = "CINC_EOF"
+
+func writeFileCommand(prefix, path, content string) (string, error) {
+	for _, line := range strings.Split(content, "\n") {
+		if strings.TrimRight(line, "\r") == heredocDelimiter {
+			return "", fmt.Errorf("refusing to write %s: its content contains a %s line, which would end the heredoc early", path, heredocDelimiter)
+		}
+	}
+	return "cat <<'" + heredocDelimiter + "' | " + prefix + "tee " + shellQuote(path) + " >/dev/null\n" + content + "\n" + heredocDelimiter, nil
 }
 
 func shellQuote(s string) string {

cli/remote/bootstrap_test.go

@@ -99,6 +99,52 @@ func TestClientRBEscapesQuotesAndBackslashes(t *testing.T) {
 	}
 }
 
+// rubyQuote stops a node name interpolating Ruby, but the client.rb it
+// produces is then delivered inside a shell heredoc. A newline in the node
+// name (or the profile's server URL) can close that heredoc early and turn the
+// rest of the value into commands, which run through sudo on the target. The
+// quoting above operates one layer up and cannot see this.
+func TestBootstrapCommandRejectsNewlinesInValues(t *testing.T) {
+	breakout := "web01\nCINC_EOF\nid > /tmp/pwned\ncat <<'CINC_EOF' | sudo tee /dev/null\nx"
+	cases := []struct {
+		name string
+		opts BootstrapOptions
+	}{
+		{"node name", BootstrapOptions{
+			NodeName:     breakout,
+			ServerURL:    "https://cinc.example.test/organizations/acme",
+			ClientKeyPEM: "key",
+		}},
+		{"server URL", BootstrapOptions{
+			NodeName:     "web01",
+			ServerURL:    "https://cinc.example.test/organizations/acme\nCINC_EOF\nid > /tmp/pwned",
+			ClientKeyPEM: "key",
+		}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd, err := BootstrapCommand(tc.opts)
+			if err == nil {
+				t.Fatalf("BootstrapCommand accepted a newline in the %s; generated script:\n%s", tc.name, cmd)
+			}
+			if !strings.Contains(err.Error(), "newline") {
+				t.Errorf("error = %v, want it to name the newline as the problem", err)
+			}
+		})
+	}
+}
+
+// Belt and braces for the heredoc itself: whatever the callers validate, a
+// body that contains the terminator must never be emitted as a heredoc.
+func TestWriteFileCommandRejectsDelimiterInContent(t *testing.T) {
+	if _, err := writeFileCommand("sudo ", "/etc/cinc/client.rb", "harmless\nCINC_EOF\nid\n"); err == nil {
+		t.Error("writeFileCommand accepted content containing its own heredoc terminator")
+	}
+	if _, err := writeFileCommand("sudo ", "/etc/cinc/client.rb", "harmless\n"); err != nil {
+		t.Errorf("ordinary content should still be accepted: %v", err)
+	}
+}
+
 // A run-list bootstrap (no policy) keeps chef_environment as before.
 func TestFirstBootJSONKeepsEnvironmentForRunListBootstrap(t *testing.T) {
 	doc := firstBoot(t, BootstrapOptions{