From 0641f0d7bb41e5a02388d4a673422070de33ab0c Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Thu, 1 Oct 2026 08:13:59 +0200 Subject: [PATCH] Isolate the cert proof helper from the service environment and cap its output --- client/internal/certproof/collect_darwin.go | 32 ++---- client/internal/certproof/collect_windows.go | 30 ++---- client/internal/certproof/helper_run.go | 105 +++++++++++++++++++ client/internal/certproof/helper_run_test.go | 73 +++++++++++++ 4 files changed, 198 insertions(+), 42 deletions(-) create mode 100644 client/internal/certproof/helper_run.go create mode 100644 client/internal/certproof/helper_run_test.go diff --git a/client/internal/certproof/collect_darwin.go b/client/internal/certproof/collect_darwin.go index b52f00e50..a70db7544 100644 --- a/client/internal/certproof/collect_darwin.go +++ b/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.Stdin = bytes.NewReader(payload) - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr + cmd := exec.CommandContext(ctx, "/bin/launchctl", "asuser", uid, "/usr/bin/sudo", "-u", "#"+uid, "-H", binary, "posture", "cert-proof") - log.Infof("certificate posture: asking the desktop session of %q (uid %s) to answer %d challenges", user.Name, uid, len(challenges)) - if err := cmd.Run(); err != nil { - return nil, fmt.Errorf("run helper as %s: %w: %s", user.Name, err, strings.TrimSpace(stderr.String())) + log.Debugf("certificate posture: asking the desktop session of uid %s to answer %d challenges", uid, len(challenges)) + proofs, err := runHelperCmd(cmd, helperRequest(challenges, peerKey)) + if err != nil { + return nil, fmt.Errorf("run helper as uid %s: %w", uid, err) } - - var resp HelperResponse - 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 + log.Debugf("certificate posture: desktop session of uid %s returned %d proofs", uid, len(proofs)) + return proofs, nil } // helperStore is the store the helper reads. On macOS the keychain search list of the diff --git a/client/internal/certproof/collect_windows.go b/client/internal/certproof/collect_windows.go index 074662fdb..8e1737864 100644 --- a/client/internal/certproof/collect_windows.go +++ b/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)) - if err := cmd.Run(); err != nil { - return nil, fmt.Errorf("run helper as %s: %w: %s", user.Name, err, strings.TrimSpace(stderr.String())) + log.Debugf("certificate posture: asking session %d to answer %d challenges", user.Session, len(challenges)) + proofs, err := runHelperCmd(cmd, helperRequest(challenges, peerKey)) + if err != nil { + return nil, fmt.Errorf("run helper in session %d: %w", user.Session, err) } - - var resp HelperResponse - 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 + log.Debugf("certificate posture: session %d returned %d proofs", user.Session, len(proofs)) + return proofs, nil } diff --git a/client/internal/certproof/helper_run.go b/client/internal/certproof/helper_run.go new file mode 100644 index 000000000..9345b50f3 --- /dev/null +++ b/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() +} diff --git a/client/internal/certproof/helper_run_test.go b/client/internal/certproof/helper_run_test.go new file mode 100644 index 000000000..be4de9cbf --- /dev/null +++ b/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") +}