Isolate the cert proof helper from the service environment and cap its output

client/internal/certproof/collect_darwin.go

@@ -1,14 +1,11 @@
 package certproof
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
 	"strconv"
-	"strings"
 	"time"
 
 	log "github.com/sirupsen/logrus"
@@ -61,32 +58,21 @@ func collectAsConsoleUser(ctx context.Context, challenges []*proto.CertificateCh
 		return nil, fmt.Errorf("resolve own binary: %w", err)
 	}
 
-	payload, err := json.Marshal(helperRequest(challenges, peerKey))
-	if err != nil {
-		return nil, fmt.Errorf("encode helper request: %w", err)
-	}
-
 	ctx, cancel := context.WithTimeout(ctx, helperTimeout)
 	defer cancel()
 
+	// Absolute paths, because the daemon's PATH is configurable through the service
+	// environment, and sudo selects the user by uid so the name never has to round-trip.
 	uid := strconv.FormatUint(uint64(user.UID), 10)
-	cmd := exec.CommandContext(ctx, "launchctl", "asuser", uid, "sudo", "-u", user.Name, "-H", binary, "posture", "cert-proof")
+	cmd := exec.CommandContext(ctx, "/bin/launchctl", "asuser", uid, "/usr/bin/sudo", "-u", "#"+uid, "-H", binary, "posture", "cert-proof")
-	cmd.Stdin = bytes.NewReader(payload)
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
 
-	log.Infof("certificate posture: asking the desktop session of %q (uid %s) to answer %d challenges", user.Name, uid, len(challenges))
+	log.Debugf("certificate posture: asking the desktop session of uid %s to answer %d challenges", uid, len(challenges))
-	if err := cmd.Run(); err != nil {
+	proofs, err := runHelperCmd(cmd, helperRequest(challenges, peerKey))
-		return nil, fmt.Errorf("run helper as %s: %w: %s", user.Name, err, strings.TrimSpace(stderr.String()))
+	if err != nil {
+		return nil, fmt.Errorf("run helper as uid %s: %w", uid, err)
 	}
-
+	log.Debugf("certificate posture: desktop session of uid %s returned %d proofs", uid, len(proofs))
-	var resp HelperResponse
+	return proofs, nil
-	if err := json.Unmarshal(stdout.Bytes(), &resp); err != nil {
-		return nil, fmt.Errorf("decode helper response: %w", err)
-	}
-	log.Infof("certificate posture: desktop session of %q returned %d proofs", user.Name, len(resp.Proofs))
-	return resp.Proofs, nil
 }
 
 // helperStore is the store the helper reads. On macOS the keychain search list of the

client/internal/certproof/collect_windows.go

@@ -1,13 +1,10 @@
 package certproof
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
-	"strings"
 	"syscall"
 	"time"
 
@@ -68,34 +65,29 @@ func collectAsDesktopUser(ctx context.Context, challenges []*proto.CertificateCh
 		return nil, fmt.Errorf("resolve own binary: %w", err)
 	}
 
-	payload, err := json.Marshal(helperRequest(challenges, peerKey))
+	// The user's own environment, not the service's: the service environment may carry
+	// secrets such as a setup key that the signed-in user must not be able to read.
+	env, err := user.Token.Environ(false)
 	if err != nil {
-		return nil, fmt.Errorf("encode helper request: %w", err)
+		return nil, fmt.Errorf("build environment of %s: %w", user.Name, err)
 	}
 
 	ctx, cancel := context.WithTimeout(ctx, helperTimeout)
 	defer cancel()
 
 	cmd := exec.CommandContext(ctx, binary, "posture", "cert-proof")
+	cmd.Env = env
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Token:         syscall.Token(user.Token),
 		HideWindow:    true,
 		CreationFlags: windows.CREATE_NO_WINDOW,
 	}
-	cmd.Stdin = bytes.NewReader(payload)
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
 
-	log.Infof("certificate posture: asking the session of %q (session %d) to answer %d challenges", user.Name, user.Session, len(challenges))
+	log.Debugf("certificate posture: asking session %d to answer %d challenges", user.Session, len(challenges))
-	if err := cmd.Run(); err != nil {
+	proofs, err := runHelperCmd(cmd, helperRequest(challenges, peerKey))
-		return nil, fmt.Errorf("run helper as %s: %w: %s", user.Name, err, strings.TrimSpace(stderr.String()))
+	if err != nil {
+		return nil, fmt.Errorf("run helper in session %d: %w", user.Session, err)
 	}
-
+	log.Debugf("certificate posture: session %d returned %d proofs", user.Session, len(proofs))
-	var resp HelperResponse
+	return proofs, nil
-	if err := json.Unmarshal(stdout.Bytes(), &resp); err != nil {
-		return nil, fmt.Errorf("decode helper response: %w", err)
-	}
-	log.Infof("certificate posture: session of %q returned %d proofs", user.Name, len(resp.Proofs))
-	return resp.Proofs, nil
 }

client/internal/certproof/helper_run.go

@@ -0,0 +1,105 @@
+package certproof
+
+import (
+	"bytes"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"os/exec"
+	"strings"
+
+	log "github.com/sirupsen/logrus"
+
+	"github.com/netbirdio/netbird/shared/management/certposture"
+)
+
+const (
+	maxHelperStdout = 1 << 20
+	maxHelperStderr = 4 << 10
+)
+
+var errHelperOutputTooLarge = errors.New("helper output exceeds the size limit")
+
+// runHelperCmd feeds req to the helper process cmd and returns the proofs it answered.
+// The helper runs as an unprivileged user who can control its output, so both streams
+// are capped and only proofs for a nonce req asked about are kept, one per challenge.
+func runHelperCmd(cmd *exec.Cmd, req HelperRequest) ([]certposture.Proof, error) {
+	payload, err := json.Marshal(req)
+	if err != nil {
+		return nil, fmt.Errorf("encode helper request: %w", err)
+	}
+
+	stdout := &cappedBuffer{limit: maxHelperStdout}
+	stderr := &cappedBuffer{limit: maxHelperStderr}
+	cmd.Stdin = bytes.NewReader(payload)
+	cmd.Stdout = stdout
+	cmd.Stderr = stderr
+
+	if err := cmd.Run(); err != nil {
+		return nil, fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String()))
+	}
+	if stdout.truncated {
+		return nil, errHelperOutputTooLarge
+	}
+
+	var resp HelperResponse
+	if err := json.Unmarshal(stdout.Bytes(), &resp); err != nil {
+		return nil, fmt.Errorf("decode helper response: %w", err)
+	}
+	return requestedProofs(req, resp.Proofs), nil
+}
+
+// requestedProofs keeps the proofs whose nonce belongs to one of req's challenges, at
+// most as many as req has challenges.
+func requestedProofs(req HelperRequest, proofs []certposture.Proof) []certposture.Proof {
+	var kept []certposture.Proof
+	for _, proof := range proofs {
+		if len(kept) == len(req.Challenges) {
+			break
+		}
+		if !req.asked(proof.Nonce) {
+			log.Debugf("certificate posture: dropping helper proof for a nonce that was not requested")
+			continue
+		}
+		kept = append(kept, proof)
+	}
+	return kept
+}
+
+func (r HelperRequest) asked(nonce []byte) bool {
+	for _, challenge := range r.Challenges {
+		if len(challenge.Nonce) > 0 && bytes.Equal(challenge.Nonce, nonce) {
+			return true
+		}
+	}
+	return false
+}
+
+// cappedBuffer keeps the first limit bytes written to it and discards the rest, so a
+// misbehaving child cannot grow the parent's memory without bound. The buffer is a
+// named field rather than embedded: an embedded bytes.Buffer would promote ReadFrom,
+// which io.Copy prefers over Write, bypassing the cap.
+type cappedBuffer struct {
+	buf       bytes.Buffer
+	limit     int
+	truncated bool
+}
+
+func (b *cappedBuffer) Write(p []byte) (int, error) {
+	if room := b.limit - b.buf.Len(); room < len(p) {
+		b.truncated = true
+		if room > 0 {
+			b.buf.Write(p[:room])
+		}
+		return len(p), nil
+	}
+	return b.buf.Write(p)
+}
+
+func (b *cappedBuffer) Bytes() []byte {
+	return b.buf.Bytes()
+}
+
+func (b *cappedBuffer) String() string {
+	return b.buf.String()
+}

client/internal/certproof/helper_run_test.go

@@ -0,0 +1,73 @@
+//go:build !windows && !js
+
+package certproof
+
+import (
+	"encoding/json"
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/netbirdio/netbird/shared/management/certposture"
+)
+
+// fakeHelper is a child process that drains its stdin and then prints script's output,
+// standing in for a helper running in a user session the daemon cannot trust.
+func fakeHelper(t *testing.T, script string) *exec.Cmd {
+	t.Helper()
+	return exec.Command("/bin/sh", "-c", "cat >/dev/null; "+script)
+}
+
+func printJSON(t *testing.T, v any) string {
+	t.Helper()
+	out, err := json.Marshal(v)
+	require.NoError(t, err)
+	return fmt.Sprintf("printf '%%s' '%s'", out)
+}
+
+func TestRunHelperCmd_KeepsOnlyRequestedProofs(t *testing.T) {
+	req := HelperRequest{PeerKey: peerKey, Challenges: []HelperChallenge{{Nonce: []byte("asked")}}}
+	resp := HelperResponse{Proofs: []certposture.Proof{
+		{Nonce: []byte("injected"), Signature: []byte("x")},
+		{Nonce: []byte("asked"), Signature: []byte("first")},
+		{Nonce: []byte("asked"), Signature: []byte("second")},
+	}}
+
+	proofs, err := runHelperCmd(fakeHelper(t, printJSON(t, resp)), req)
+
+	require.NoError(t, err)
+	require.Len(t, proofs, 1, "a helper may not return more proofs than challenges or proofs for nonces it was not asked about")
+	assert.Equal(t, []byte("first"), proofs[0].Signature, "the first proof for the requested nonce is kept")
+}
+
+func TestRunHelperCmd_RejectsOversizedOutput(t *testing.T) {
+	req := HelperRequest{Challenges: []HelperChallenge{{Nonce: []byte("asked")}}}
+	script := fmt.Sprintf("head -c %d /dev/zero", maxHelperStdout+1)
+
+	_, err := runHelperCmd(fakeHelper(t, script), req)
+
+	assert.ErrorIs(t, err, errHelperOutputTooLarge)
+}
+
+func TestRunHelperCmd_CapsStderrInError(t *testing.T) {
+	req := HelperRequest{Challenges: []HelperChallenge{{Nonce: []byte("asked")}}}
+	script := fmt.Sprintf("head -c %d /dev/zero | tr '\\0' 'a' >&2; exit 3", 10*maxHelperStderr)
+
+	_, err := runHelperCmd(fakeHelper(t, script), req)
+
+	require.Error(t, err)
+	assert.LessOrEqual(t, len(err.Error()), maxHelperStderr+100, "a chatty helper must not blow up the daemon's error or log line")
+	assert.True(t, strings.Contains(err.Error(), "exit status 3"), "the exit status is kept: %v", err)
+}
+
+func TestRunHelperCmd_RejectsGarbage(t *testing.T) {
+	req := HelperRequest{Challenges: []HelperChallenge{{Nonce: []byte("asked")}}}
+
+	_, err := runHelperCmd(fakeHelper(t, "printf 'not json'"), req)
+
+	assert.ErrorContains(t, err, "decode helper response")
+}