diff --git a/client/android/client.go b/client/android/client.go index 1a8dd7d09..501d7f77c 100644 --- a/client/android/client.go +++ b/client/android/client.go @@ -301,7 +301,7 @@ func (c *Client) DebugBundle(platformFiles PlatformFiles, anonymize bool) (strin uploadCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - key, err := debug.UploadDebugBundle(uploadCtx, types.DefaultBundleURL, cfg.ManagementURL.String(), path) + key, err := debug.UploadDebugBundle(uploadCtx, types.DefaultBundleURL, cfg.ManagementURL.String(), path, false) if err != nil { return "", fmt.Errorf("upload debug bundle: %w", err) } diff --git a/client/cmd/debug.go b/client/cmd/debug.go index 57e75f663..7ddc3afc4 100644 --- a/client/cmd/debug.go +++ b/client/cmd/debug.go @@ -29,8 +29,9 @@ const errCloseConnection = "Failed to close connection: %v" var ( logFileCount uint32 systemInfoFlag bool - uploadBundleFlag bool - uploadBundleURLFlag string + uploadBundleFlag bool + uploadBundleURLFlag string + uploadBundleInsecureFlag bool ) var debugCmd = &cobra.Command{ @@ -174,10 +175,11 @@ func debugBundle(cmd *cobra.Command, _ []string) error { } if uploadBundleFlag { request.UploadURL = uploadBundleURLFlag + request.UploadInsecure = uploadBundleInsecureFlag } resp, err := client.DebugBundle(cmd.Context(), request) if err != nil { - return fmt.Errorf("failed to bundle debug: %v", status.Convert(err).Message()) + return daemonCallError("bundle debug", err) } cmd.Printf("Local file:\n%s\n", resp.GetPath()) @@ -373,10 +375,11 @@ func runForDuration(cmd *cobra.Command, args []string) error { } if uploadBundleFlag { request.UploadURL = uploadBundleURLFlag + request.UploadInsecure = uploadBundleInsecureFlag } resp, err := client.DebugBundle(cmd.Context(), request) if err != nil { - return fmt.Errorf("failed to bundle debug: %v", status.Convert(err).Message()) + return daemonCallError("bundle debug", err) } if needsRestoreUp { @@ -524,10 +527,12 @@ func init() { debugBundleCmd.Flags().BoolVarP(&systemInfoFlag, "system-info", "S", true, "Adds system information to the debug bundle") debugBundleCmd.Flags().BoolVarP(&uploadBundleFlag, "upload-bundle", "U", false, "Uploads the debug bundle to a server") debugBundleCmd.Flags().StringVar(&uploadBundleURLFlag, "upload-bundle-url", types.DefaultBundleURL, "Service URL to get an URL to upload the debug bundle") + debugBundleCmd.Flags().BoolVar(&uploadBundleInsecureFlag, "upload-bundle-insecure", false, "Allow uploading to an http or untrusted-TLS upload server (self-hosted); requires root") forCmd.Flags().Uint32VarP(&logFileCount, "log-file-count", "C", 1, "Number of rotated log files to include in debug bundle") forCmd.Flags().BoolVarP(&systemInfoFlag, "system-info", "S", true, "Adds system information to the debug bundle") forCmd.Flags().BoolVarP(&uploadBundleFlag, "upload-bundle", "U", false, "Uploads the debug bundle to a server") forCmd.Flags().StringVar(&uploadBundleURLFlag, "upload-bundle-url", types.DefaultBundleURL, "Service URL to get an URL to upload the debug bundle") + forCmd.Flags().BoolVar(&uploadBundleInsecureFlag, "upload-bundle-insecure", false, "Allow uploading to an http or untrusted-TLS upload server (self-hosted); requires root") forCmd.Flags().Bool("capture", false, "Capture packets during the debug duration and include in bundle") } diff --git a/client/configs/configs.go b/client/configs/configs.go index 8f9c3ba28..a1ecf0feb 100644 --- a/client/configs/configs.go +++ b/client/configs/configs.go @@ -6,6 +6,11 @@ import ( "runtime" ) +// UILogFile is the file name the desktop UI writes its log to. It is defined +// here so the UI (writer), the daemon's RegisterUILog validation, and the debug +// bundle collector all share one definition. +const UILogFile = "gui-client.log" + var StateDir string func init() { diff --git a/client/internal/debug/debug.go b/client/internal/debug/debug.go index 2de1023e9..0f81844f6 100644 --- a/client/internal/debug/debug.go +++ b/client/internal/debug/debug.go @@ -229,7 +229,6 @@ scutil_dns.txt (macOS only): const ( clientLogFile = "client.log" - uiLogFile = "gui-client.log" errorLogFile = "netbird.err" stdoutLogFile = "netbird.out" @@ -248,6 +247,20 @@ type MetricsExporter interface { Export(w io.Writer) error } +// LogOpener opens a log file for inclusion in the bundle. It exists so that log +// files whose path was supplied by an IPC caller can be opened under a check +// the daemon defines, instead of being opened with the daemon's privileges +// unconditionally. +type LogOpener func(path string) (*os.File, error) + +func openLogFile(path string) (*os.File, error) { + f, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("open %s: %w", path, err) + } + return f, nil +} + type BundleGenerator struct { anonymizer *anonymize.Anonymizer @@ -257,6 +270,7 @@ type BundleGenerator struct { syncResponse *mgmProto.SyncResponse logPath string uiLogPath string + uiLogOpener LogOpener tempDir string statePath string cpuProfile []byte @@ -285,14 +299,20 @@ type GeneratorDependencies struct { SyncResponse *mgmProto.SyncResponse LogPath string UILogPath string // Absolute path to the desktop UI's gui-client.log, reported via RegisterUILog. Empty if no UI registered one. - TempDir string // Directory for temporary bundle zip files. If empty, os.TempDir() is used. - StatePath string // Path to the state file. If empty, the ServiceManager default path is used. - CPUProfile []byte - CapturePath string - RefreshStatus func() - ClientMetrics MetricsExporter - DaemonVersion string - CliVersion string + // UILogOpener opens the UI log and its rotated siblings. The path comes from + // a local IPC caller, so the daemon must not open it with plain os.Open: the + // opener is where the caller's right to that file is enforced. Defaults to + // os.Open, which is only correct where the path is not caller-supplied + // (mobile). + UILogOpener LogOpener + TempDir string // Directory for temporary bundle zip files. If empty, os.TempDir() is used. + StatePath string // Path to the state file. If empty, the ServiceManager default path is used. + CPUProfile []byte + CapturePath string + RefreshStatus func() + ClientMetrics MetricsExporter + DaemonVersion string + CliVersion string } func NewBundleGenerator(deps GeneratorDependencies, cfg BundleConfig) *BundleGenerator { @@ -302,6 +322,11 @@ func NewBundleGenerator(deps GeneratorDependencies, cfg BundleConfig) *BundleGen logFileCount = 1 } + uiLogOpener := deps.UILogOpener + if uiLogOpener == nil { + uiLogOpener = openLogFile + } + return &BundleGenerator{ anonymizer: anonymize.NewAnonymizer(anonymize.DefaultAddresses()), @@ -310,6 +335,7 @@ func NewBundleGenerator(deps GeneratorDependencies, cfg BundleConfig) *BundleGen syncResponse: deps.SyncResponse, logPath: deps.LogPath, uiLogPath: deps.UILogPath, + uiLogOpener: uiLogOpener, tempDir: deps.TempDir, statePath: deps.StatePath, cpuProfile: deps.CPUProfile, @@ -996,11 +1022,11 @@ func (g *BundleGenerator) addLogfile() error { logDir := filepath.Dir(g.logPath) - if err := g.addSingleLogfile(g.logPath, clientLogFile); err != nil { + if err := g.addSingleLogfile(openLogFile, g.logPath, clientLogFile); err != nil { return fmt.Errorf("add client log file to zip: %w", err) } - g.addRotatedLogFiles(logDir, clientLogPrefix) + g.addRotatedLogFiles(openLogFile, logDir, clientLogPrefix) stdErrLogPath := filepath.Join(logDir, errorLogFile) stdoutLogPath := filepath.Join(logDir, stdoutLogFile) @@ -1009,11 +1035,11 @@ func (g *BundleGenerator) addLogfile() error { stdoutLogPath = darwinStdoutLogPath } - if err := g.addSingleLogfile(stdErrLogPath, errorLogFile); err != nil { + if err := g.addSingleLogfile(openLogFile, stdErrLogPath, errorLogFile); err != nil { log.Warnf("Failed to add %s to zip: %v", errorLogFile, err) } - if err := g.addSingleLogfile(stdoutLogPath, stdoutLogFile); err != nil { + if err := g.addSingleLogfile(openLogFile, stdoutLogPath, stdoutLogFile); err != nil { log.Warnf("Failed to add %s to zip: %v", stdoutLogFile, err) } @@ -1030,18 +1056,18 @@ func (g *BundleGenerator) addUILog() error { return nil } - if err := g.addSingleLogfile(g.uiLogPath, uiLogFile); err != nil { + if err := g.addSingleLogfile(g.uiLogOpener, g.uiLogPath, configs.UILogFile); err != nil { return fmt.Errorf("add UI log file to zip: %w", err) } - g.addRotatedLogFiles(filepath.Dir(g.uiLogPath), uiLogPrefix) + g.addRotatedLogFiles(g.uiLogOpener, filepath.Dir(g.uiLogPath), uiLogPrefix) return nil } // addSingleLogfile adds a single log file to the archive -func (g *BundleGenerator) addSingleLogfile(logPath, targetName string) error { - logFile, err := os.Open(logPath) +func (g *BundleGenerator) addSingleLogfile(open LogOpener, logPath, targetName string) error { + logFile, err := open(logPath) if err != nil { return fmt.Errorf("open log file %s: %w", targetName, err) } @@ -1066,8 +1092,8 @@ func (g *BundleGenerator) addSingleLogfile(logPath, targetName string) error { } // addSingleLogFileGz adds a single gzipped log file to the archive -func (g *BundleGenerator) addSingleLogFileGz(logPath, targetName string) error { - f, err := os.Open(logPath) +func (g *BundleGenerator) addSingleLogFileGz(open LogOpener, logPath, targetName string) error { + f, err := open(logPath) if err != nil { return fmt.Errorf("open gz log file %s: %w", targetName, err) } @@ -1114,7 +1140,7 @@ func (g *BundleGenerator) addSingleLogFileGz(logPath, targetName string) error { // addRotatedLogFiles adds rotated log files to the bundle based on logFileCount. // prefix is the base log name without extension (e.g. "client", "gui-client"); // the glob matches both files rotated by us and by logrotate on linux. -func (g *BundleGenerator) addRotatedLogFiles(logDir, prefix string) { +func (g *BundleGenerator) addRotatedLogFiles(open LogOpener, logDir, prefix string) { if g.logFileCount == 0 { return } @@ -1154,9 +1180,9 @@ func (g *BundleGenerator) addRotatedLogFiles(logDir, prefix string) { for i := 0; i < maxFiles; i++ { name := filepath.Base(files[i]) if strings.HasSuffix(name, ".gz") { - err = g.addSingleLogFileGz(files[i], name) + err = g.addSingleLogFileGz(open, files[i], name) } else { - err = g.addSingleLogfile(files[i], name) + err = g.addSingleLogfile(open, files[i], name) } if err != nil { log.Warnf("failed to add rotated log %s: %v", name, err) diff --git a/client/internal/debug/debug_ios.go b/client/internal/debug/debug_ios.go index a07c23dbd..001d64241 100644 --- a/client/internal/debug/debug_ios.go +++ b/client/internal/debug/debug_ios.go @@ -27,7 +27,7 @@ func (g *BundleGenerator) addPlatformLog() error { } swiftLogPath := filepath.Join(filepath.Dir(g.logPath), swiftLogFile) - if err := g.addSingleLogfile(swiftLogPath, swiftLogFile); err != nil { + if err := g.addSingleLogfile(openLogFile, swiftLogPath, swiftLogFile); err != nil { // The Swift log is best-effort: the app may not have written it yet. log.Warnf("failed to add %s to debug bundle: %v", swiftLogFile, err) } diff --git a/client/internal/debug/debug_logfiles_test.go b/client/internal/debug/debug_logfiles_test.go index 3749b3e9b..31420711f 100644 --- a/client/internal/debug/debug_logfiles_test.go +++ b/client/internal/debug/debug_logfiles_test.go @@ -97,7 +97,7 @@ func runAddRotatedLogFilesPrefix(t *testing.T, dir, prefix string, logFileCount archive: zip.NewWriter(&buf), logFileCount: logFileCount, } - g.addRotatedLogFiles(dir, prefix) + g.addRotatedLogFiles(openLogFile, dir, prefix) require.NoError(t, g.archive.Close()) zr, err := zip.NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len())) diff --git a/client/internal/debug/uilog_test.go b/client/internal/debug/uilog_test.go new file mode 100644 index 000000000..103e98c6f --- /dev/null +++ b/client/internal/debug/uilog_test.go @@ -0,0 +1,64 @@ +package debug + +import ( + "archive/zip" + "bytes" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/configs" +) + +// bundleEntries generates a bundle with the given generator and returns the +// set of entry names in the resulting archive. +func bundleEntries(t *testing.T, g *BundleGenerator) map[string]struct{} { + t.Helper() + + path, err := g.Generate() + require.NoError(t, err) + t.Cleanup(func() { _ = os.Remove(path) }) + + data, err := os.ReadFile(path) + require.NoError(t, err) + + zr, err := zip.NewReader(bytes.NewReader(data), int64(len(data))) + require.NoError(t, err) + + names := make(map[string]struct{}, len(zr.File)) + for _, f := range zr.File { + names[f.Name] = struct{}{} + } + return names +} + +func TestBundleIncludesUILogWhenOpenerAllows(t *testing.T) { + path := filepath.Join(t.TempDir(), configs.UILogFile) + require.NoError(t, os.WriteFile(path, []byte("gui log"), 0600)) + + g := NewBundleGenerator(GeneratorDependencies{ + UILogPath: path, + UILogOpener: openLogFile, + }, BundleConfig{}) + + require.Contains(t, bundleEntries(t, g), configs.UILogFile) +} + +// A UILogOpener that refuses (as the ownership check does for a foreign file) +// keeps the UI log out of the bundle without failing bundle generation. +func TestBundleExcludesUILogWhenOpenerRefuses(t *testing.T) { + path := filepath.Join(t.TempDir(), configs.UILogFile) + require.NoError(t, os.WriteFile(path, []byte("secret"), 0600)) + + g := NewBundleGenerator(GeneratorDependencies{ + UILogPath: path, + UILogOpener: func(string) (*os.File, error) { + return nil, fmt.Errorf("not owned by the caller") + }, + }, BundleConfig{}) + + require.NotContains(t, bundleEntries(t, g), configs.UILogFile) +} diff --git a/client/internal/debug/upload.go b/client/internal/debug/upload.go index cdf52409d..88fde6d6f 100644 --- a/client/internal/debug/upload.go +++ b/client/internal/debug/upload.go @@ -3,10 +3,12 @@ package debug import ( "context" "crypto/sha256" + "crypto/tls" "encoding/json" "fmt" "io" "net/http" + neturl "net/url" "os" "github.com/netbirdio/netbird/upload-server/types" @@ -14,20 +16,80 @@ import ( const maxBundleUploadSize = 50 * 1024 * 1024 -func UploadDebugBundle(ctx context.Context, url, managementURL, filePath string) (key string, err error) { - response, err := getUploadURL(ctx, url, managementURL) +// requireHTTPS refuses any URL the daemon would fetch or upload to that is not +// https. The daemon runs as root and the bundle carries its logs and state, so a +// plaintext hop is a place to intercept the bundle or the presigned redirect. +// The server-side gate already enforces this for the desktop path; this also +// covers the mobile and job-runner callers that reach this package directly. +// Skipped when the caller opted into an insecure upload (self-hosted server). +func requireHTTPS(what, rawURL string) error { + parsed, err := neturl.Parse(rawURL) + if err != nil { + return fmt.Errorf("parse %s: %w", what, err) + } + if parsed.Scheme != "https" { + return fmt.Errorf("%s must use https, got scheme %q", what, parsed.Scheme) + } + return nil +} + +// uploadClient returns the HTTP client for the upload requests. The default +// client verifies TLS and refuses a redirect that would downgrade to a non-https +// hop, so a bundle can never leave over http after an https start. The insecure +// variant accepts http and untrusted certificates, and is only reachable for a +// privileged caller that passed --upload-bundle-insecure (see +// requirePrivilegeForUploadURL). +func uploadClient(insecure bool) *http.Client { + if !insecure { + return &http.Client{CheckRedirect: rejectInsecureRedirect} + } + return &http.Client{ + Transport: &http.Transport{ + //nolint:gosec // opt-in, privileged, self-hosted upload servers + TLSClientConfig: &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}, + }, + } +} + +// rejectInsecureRedirect refuses a redirect to a non-https target and keeps the +// standard library's 10-hop limit that a custom CheckRedirect would otherwise +// disable. +func rejectInsecureRedirect(req *http.Request, via []*http.Request) error { + if req.URL.Scheme != "https" { + return fmt.Errorf("refusing redirect to non-https URL %s", req.URL.Redacted()) + } + if len(via) >= 10 { + return fmt.Errorf("stopped after 10 redirects") + } + return nil +} + +func UploadDebugBundle(ctx context.Context, url, managementURL, filePath string, insecure bool) (key string, err error) { + if !insecure { + if err := requireHTTPS("upload service URL", url); err != nil { + return "", err + } + } + + response, err := getUploadURL(ctx, url, managementURL, insecure) if err != nil { return "", err } - err = upload(ctx, filePath, response) + if !insecure { + if err := requireHTTPS("upload URL from service", response.URL); err != nil { + return "", err + } + } + + err = upload(ctx, filePath, response, insecure) if err != nil { return "", err } return response.Key, nil } -func upload(ctx context.Context, filePath string, response *types.GetURLResponse) error { +func upload(ctx context.Context, filePath string, response *types.GetURLResponse, insecure bool) error { fileData, err := os.Open(filePath) if err != nil { return fmt.Errorf("open file: %w", err) @@ -52,7 +114,7 @@ func upload(ctx context.Context, filePath string, response *types.GetURLResponse req.ContentLength = stat.Size() req.Header.Set("Content-Type", "application/octet-stream") - putResp, err := http.DefaultClient.Do(req) + putResp, err := uploadClient(insecure).Do(req) if err != nil { return fmt.Errorf("upload failed: %v", err) } @@ -65,16 +127,23 @@ func upload(ctx context.Context, filePath string, response *types.GetURLResponse return nil } -func getUploadURL(ctx context.Context, url string, managementURL string) (*types.GetURLResponse, error) { - id := getURLHash(managementURL) - getReq, err := http.NewRequestWithContext(ctx, "GET", url+"?id="+id, nil) +func getUploadURL(ctx context.Context, serviceURL string, managementURL string, insecure bool) (*types.GetURLResponse, error) { + parsed, err := neturl.Parse(serviceURL) + if err != nil { + return nil, fmt.Errorf("parse upload service URL: %w", err) + } + q := parsed.Query() + q.Set("id", getURLHash(managementURL)) + parsed.RawQuery = q.Encode() + + getReq, err := http.NewRequestWithContext(ctx, "GET", parsed.String(), nil) if err != nil { return nil, fmt.Errorf("create GET request: %w", err) } getReq.Header.Set(types.ClientHeader, types.ClientHeaderValue) - resp, err := http.DefaultClient.Do(getReq) + resp, err := uploadClient(insecure).Do(getReq) if err != nil { return nil, fmt.Errorf("get presigned URL: %w", err) } diff --git a/client/internal/debug/upload_test.go b/client/internal/debug/upload_test.go index f224b8d3f..f3927cb81 100644 --- a/client/internal/debug/upload_test.go +++ b/client/internal/debug/upload_test.go @@ -5,6 +5,7 @@ import ( "errors" "net" "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -43,7 +44,7 @@ func TestUpload(t *testing.T) { fileContent := []byte("test file content") err := os.WriteFile(file, fileContent, 0640) require.NoError(t, err) - key, err := UploadDebugBundle(context.Background(), testURL+types.GetURLPath, testURL, file) + key, err := UploadDebugBundle(context.Background(), testURL+types.GetURLPath, testURL, file, true) require.NoError(t, err) id := getURLHash(testURL) require.Contains(t, key, id+"/") @@ -79,3 +80,47 @@ func waitForServer(t *testing.T, addr string) { } t.Fatalf("server did not start listening on %s in time", addr) } + +func TestRequireHTTPS(t *testing.T) { + require.NoError(t, requireHTTPS("upload URL", "https://upload.example/path")) + require.Error(t, requireHTTPS("upload URL", "http://upload.example/path")) + require.Error(t, requireHTTPS("upload URL", "ftp://upload.example/path")) + require.Error(t, requireHTTPS("upload URL", "://malformed")) +} + +func TestRejectInsecureRedirect(t *testing.T) { + httpsReq, err := http.NewRequest(http.MethodGet, "https://a.example/", nil) + require.NoError(t, err) + require.NoError(t, rejectInsecureRedirect(httpsReq, nil), "https redirect target must be allowed") + + httpReq, err := http.NewRequest(http.MethodGet, "http://a.example/", nil) + require.NoError(t, err) + require.Error(t, rejectInsecureRedirect(httpReq, nil), "http redirect target must be refused") + + require.Error(t, rejectInsecureRedirect(httpsReq, make([]*http.Request, 10)), "the 10-redirect limit must be enforced") +} + +// The secure client refuses to follow an https response that redirects to http, +// so a bundle can't be downgraded onto plaintext mid-flight. +func TestUploadClientRefusesHTTPSToHTTPRedirect(t *testing.T) { + plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(plain.Close) + + secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, plain.URL, http.StatusFound) + })) + t.Cleanup(secure.Close) + + client := uploadClient(false) + // Trust the test server's cert without disabling verification globally. + client.Transport = secure.Client().Transport + + resp, err := client.Get(secure.URL) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err, "redirect from https to http must be refused") + require.Contains(t, err.Error(), "non-https") +} diff --git a/client/internal/ipcauth/ownedfile.go b/client/internal/ipcauth/ownedfile.go new file mode 100644 index 000000000..be7bf4864 --- /dev/null +++ b/client/internal/ipcauth/ownedfile.go @@ -0,0 +1,63 @@ +package ipcauth + +import ( + "fmt" + "os" +) + +// OpenOwnedFile opens path for reading on behalf of the IPC caller identified by +// id, and fails unless the opened file is a regular file that id owns. +// +// It exists for the paths a local caller hands to the daemon over the IPC. The +// daemon runs as root, so opening such a path unchecked lets any local user read +// any file through it. Ownership is the invariant that keeps the daemon from +// reading, with its own privileges, a file the caller could not read itself: a +// symlink or hard link planted at the path resolves to a file someone else owns +// and is refused. +// +// The check is made against the open descriptor rather than the path, so +// swapping the path between the check and the read cannot change the answer. +// +// A privileged caller is exempt: it can read the file directly, so refusing it +// here would protect nothing. The regular-file requirement still applies to +// everyone, since a fifo or device planted at the path is never a log file. +func OpenOwnedFile(id Identity, path string) (*os.File, error) { + f, err := openForRead(path) + if err != nil { + return nil, err + } + + if err := checkOwnership(id, f); err != nil { + if cerr := f.Close(); cerr != nil { + return nil, fmt.Errorf("%w (close: %v)", err, cerr) + } + return nil, err + } + + return f, nil +} + +func checkOwnership(id Identity, f *os.File) error { + info, err := f.Stat() + if err != nil { + return fmt.Errorf("stat %s: %w", f.Name(), err) + } + + if !info.Mode().IsRegular() { + return fmt.Errorf("%s is not a regular file", f.Name()) + } + + if IsPrivilegedCaller(id) { + return nil + } + + owned, err := fileOwnedBy(id, f) + if err != nil { + return fmt.Errorf("read owner of %s: %w", f.Name(), err) + } + if !owned { + return fmt.Errorf("%s is not owned by the caller (%s)", f.Name(), id) + } + + return nil +} diff --git a/client/internal/ipcauth/ownedfile_test.go b/client/internal/ipcauth/ownedfile_test.go new file mode 100644 index 000000000..b8178ad45 --- /dev/null +++ b/client/internal/ipcauth/ownedfile_test.go @@ -0,0 +1,64 @@ +package ipcauth + +import ( + "io" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/require" +) + +// otherIdentity is an unprivileged caller that owns nothing the test creates. +func otherIdentity(t *testing.T) Identity { + t.Helper() + if runtime.GOOS == "windows" { + return Identity{SID: "S-1-5-21-1-2-3-1001"} + } + return Identity{UID: uint32(os.Geteuid() + 1), GID: uint32(os.Getegid() + 1)} +} + +func TestOpenOwnedFileReadsFileOwnedByCaller(t *testing.T) { + path := filepath.Join(t.TempDir(), "gui-client.log") + require.NoError(t, os.WriteFile(path, []byte("hello"), 0600)) + + id, err := CurrentProcessIdentity() + require.NoError(t, err) + + f, err := OpenOwnedFile(id, path) + require.NoError(t, err) + t.Cleanup(func() { _ = f.Close() }) + + content, err := io.ReadAll(f) + require.NoError(t, err) + require.Equal(t, "hello", string(content)) +} + +func TestOpenOwnedFileRefusesFileOwnedByAnother(t *testing.T) { + path := filepath.Join(t.TempDir(), "gui-client.log") + require.NoError(t, os.WriteFile(path, []byte("secret"), 0600)) + + _, err := OpenOwnedFile(otherIdentity(t), path) + require.ErrorContains(t, err, "not owned by the caller") +} + +func TestOpenOwnedFileRefusesNonRegularFile(t *testing.T) { + dir := t.TempDir() + + // The caller owns the directory, so this is the regular-file requirement + // talking, not the ownership check. + id, err := CurrentProcessIdentity() + require.NoError(t, err) + + _, err = OpenOwnedFile(id, dir) + require.ErrorContains(t, err, "not a regular file") +} + +func TestOpenOwnedFileRefusesMissingFile(t *testing.T) { + id, err := CurrentProcessIdentity() + require.NoError(t, err) + + _, err = OpenOwnedFile(id, filepath.Join(t.TempDir(), "absent.log")) + require.Error(t, err) +} diff --git a/client/internal/ipcauth/ownedfile_unix.go b/client/internal/ipcauth/ownedfile_unix.go new file mode 100644 index 000000000..4a6afcea7 --- /dev/null +++ b/client/internal/ipcauth/ownedfile_unix.go @@ -0,0 +1,35 @@ +//go:build !windows + +package ipcauth + +import ( + "fmt" + "os" + "syscall" +) + +// openForRead opens a caller-supplied path without following a symlink at its +// final component and without blocking: a fifo planted at the path would +// otherwise stall the open until a writer appears, and the daemon holds a lock +// while it collects the file. +func openForRead(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, fmt.Errorf("open %s: %w", path, err) + } + return f, nil +} + +func fileOwnedBy(id Identity, f *os.File) (bool, error) { + info, err := f.Stat() + if err != nil { + return false, err + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return false, fmt.Errorf("no owner information in %T", info.Sys()) + } + + return stat.Uid == id.UID, nil +} diff --git a/client/internal/ipcauth/ownedfile_unix_test.go b/client/internal/ipcauth/ownedfile_unix_test.go new file mode 100644 index 000000000..9e7831991 --- /dev/null +++ b/client/internal/ipcauth/ownedfile_unix_test.go @@ -0,0 +1,57 @@ +//go:build !windows + +package ipcauth + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// A symlink is the shape the arbitrary-read attempt takes: the caller owns the +// link, the file it points at belongs to someone else. +func TestOpenOwnedFileRefusesSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.log") + require.NoError(t, os.WriteFile(target, []byte("secret"), 0600)) + + link := filepath.Join(dir, "gui-client.log") + require.NoError(t, os.Symlink(target, link)) + + id, err := CurrentProcessIdentity() + require.NoError(t, err) + + _, err = OpenOwnedFile(id, link) + // O_NOFOLLOW on a symlink reports ELOOP on Linux/Darwin and EMLINK on FreeBSD. + if !errors.Is(err, syscall.ELOOP) && !errors.Is(err, syscall.EMLINK) { + t.Fatalf("symlink open: got %v, want ELOOP or EMLINK", err) + } +} + +// A fifo would block the open until a writer showed up, stalling the daemon +// while it holds its lock. +func TestOpenOwnedFileRefusesFifoWithoutBlocking(t *testing.T) { + path := filepath.Join(t.TempDir(), "gui-client.log") + require.NoError(t, syscall.Mkfifo(path, 0600)) + + id, err := CurrentProcessIdentity() + require.NoError(t, err) + + done := make(chan error, 1) + go func() { + _, err := OpenOwnedFile(id, path) + done <- err + }() + + select { + case err := <-done: + require.ErrorContains(t, err, "not a regular file") + case <-time.After(5 * time.Second): + t.Fatal("opening a fifo blocked") + } +} diff --git a/client/internal/ipcauth/ownedfile_windows.go b/client/internal/ipcauth/ownedfile_windows.go new file mode 100644 index 000000000..19acaec4d --- /dev/null +++ b/client/internal/ipcauth/ownedfile_windows.go @@ -0,0 +1,59 @@ +//go:build windows + +package ipcauth + +import ( + "fmt" + "os" + + "golang.org/x/sys/windows" +) + +// openForRead opens a caller-supplied path without following a reparse point at +// it. FILE_FLAG_OPEN_REPARSE_POINT is the Windows analogue of O_NOFOLLOW: it +// opens a symlink/junction itself rather than its target, so the regular-file +// check in checkOwnership refuses a link the caller planted to redirect the +// read. FILE_FLAG_BACKUP_SEMANTICS lets a directory open too (as os.Open does), +// so a directory planted at the path is refused as non-regular rather than +// erroring here. The share mode matches os.Open so a log being written stays +// openable. +func openForRead(path string) (*os.File, error) { + p, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, fmt.Errorf("convert path %s: %w", path, err) + } + + handle, err := windows.CreateFile( + p, + windows.GENERIC_READ, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_OPEN_REPARSE_POINT|windows.FILE_FLAG_BACKUP_SEMANTICS, + 0, + ) + if err != nil { + return nil, fmt.Errorf("open %s: %w", path, err) + } + + return os.NewFile(uintptr(handle), path), nil +} + +// fileOwnedBy compares the file's owner SID with the caller's. Files an elevated +// process creates are owned by BUILTIN\Administrators rather than by the user, +// but such a caller is privileged and never reaches this check. +func fileOwnedBy(id Identity, f *os.File) (bool, error) { + // x/sys/windows GetSecurityInfo frees the OS buffer itself and returns a + // Go-heap copy, so there is nothing to LocalFree here. + sd, err := windows.GetSecurityInfo(windows.Handle(f.Fd()), windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION) + if err != nil { + return false, fmt.Errorf("read security info: %w", err) + } + + owner, _, err := sd.Owner() + if err != nil { + return false, fmt.Errorf("read owner: %w", err) + } + + return id.SID != "" && owner.String() == id.SID, nil +} diff --git a/client/internal/ipcauth/ownedfile_windows_test.go b/client/internal/ipcauth/ownedfile_windows_test.go new file mode 100644 index 000000000..ab68fdf39 --- /dev/null +++ b/client/internal/ipcauth/ownedfile_windows_test.go @@ -0,0 +1,78 @@ +//go:build windows + +package ipcauth + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +// fileOwnerSID reads the owner SID of path the same way OpenOwnedFile does, so +// the test can construct an Identity that matches (or deliberately does not). +func fileOwnerSID(t *testing.T, path string) string { + t.Helper() + f, err := os.Open(path) + require.NoError(t, err) + t.Cleanup(func() { _ = f.Close() }) + + sd, err := windows.GetSecurityInfo(windows.Handle(f.Fd()), windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION) + require.NoError(t, err) + owner, _, err := sd.Owner() + require.NoError(t, err) + return owner.String() +} + +// The allow branch of fileOwnedBy is the SID-equality path the legitimate GUI +// flow depends on. Running elevated, a created file is owned by +// BUILTIN\Administrators; an Identity carrying that SID with Elevated=false and +// no groups is unprivileged by IsPrivileged (which reads the token, not the +// SID's RID), so this exercises the real GetSecurityInfo equality rather than +// the privileged-caller shortcut. +func TestOpenOwnedFileWindowsOwnerMatchAllows(t *testing.T) { + path := filepath.Join(t.TempDir(), "gui-client.log") + require.NoError(t, os.WriteFile(path, []byte("hello"), 0600)) + + ownerSID := fileOwnerSID(t, path) + id := Identity{SID: ownerSID} + require.False(t, id.IsPrivileged(), "identity built from the owner SID must be unprivileged for this to test the match path") + + f, err := OpenOwnedFile(id, path) + require.NoError(t, err) + _ = f.Close() +} + +func TestOpenOwnedFileWindowsOwnerMismatchRefuses(t *testing.T) { + path := filepath.Join(t.TempDir(), "gui-client.log") + require.NoError(t, os.WriteFile(path, []byte("secret"), 0600)) + + other := Identity{SID: "S-1-5-21-9-9-9-9999"} + require.False(t, other.IsPrivileged()) + + _, err := OpenOwnedFile(other, path) + require.ErrorContains(t, err, "not owned by the caller") +} + +// FILE_FLAG_OPEN_REPARSE_POINT must make OpenOwnedFile refuse a symlink the same +// way O_NOFOLLOW does on Unix, so a planted link can't redirect the read to +// another file. Creating a symlink needs a privilege the runner may lack, so the +// test skips rather than fails when it can't. +func TestOpenOwnedFileWindowsRefusesSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.log") + require.NoError(t, os.WriteFile(target, []byte("secret"), 0600)) + + link := filepath.Join(dir, "gui-client.log") + if err := os.Symlink(target, link); err != nil { + t.Skipf("cannot create symlink (privilege not held?): %v", err) + } + + id, err := CurrentProcessIdentity() + require.NoError(t, err) + + _, err = OpenOwnedFile(id, link) + require.Error(t, err, "a symlink must be refused") +} diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index 9289a3910..37d3e5d99 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -262,7 +262,7 @@ func (c *Client) DebugBundle(anonymize bool) (string, error) { uploadCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - key, err := debug.UploadDebugBundle(uploadCtx, types.DefaultBundleURL, cfg.ManagementURL.String(), path) + key, err := debug.UploadDebugBundle(uploadCtx, types.DefaultBundleURL, cfg.ManagementURL.String(), path, false) if err != nil { return "", fmt.Errorf("upload debug bundle: %w", err) } diff --git a/client/jobexec/executor.go b/client/jobexec/executor.go index e29cc8840..9401acacc 100644 --- a/client/jobexec/executor.go +++ b/client/jobexec/executor.go @@ -54,7 +54,7 @@ func (e *Executor) BundleJob(ctx context.Context, debugBundleDependencies debug. } }() - key, err := debug.UploadDebugBundle(ctx, types.DefaultBundleURL, mgmURL, path) + key, err := debug.UploadDebugBundle(ctx, types.DefaultBundleURL, mgmURL, path, false) if err != nil { log.Errorf("failed to upload debug bundle: %v", err) return "", fmt.Errorf("upload debug bundle: %w", err) diff --git a/client/proto/daemon.pb.go b/client/proto/daemon.pb.go index 6fbb09958..d4deeb8ec 100644 --- a/client/proto/daemon.pb.go +++ b/client/proto/daemon.pb.go @@ -2771,14 +2771,18 @@ func (x *ForwardingRulesResponse) GetRules() []*ForwardingRule { // DebugBundler type DebugBundleRequest struct { - state protoimpl.MessageState `protogen:"open.v1"` - Anonymize bool `protobuf:"varint,1,opt,name=anonymize,proto3" json:"anonymize,omitempty"` - SystemInfo bool `protobuf:"varint,3,opt,name=systemInfo,proto3" json:"systemInfo,omitempty"` - UploadURL string `protobuf:"bytes,4,opt,name=uploadURL,proto3" json:"uploadURL,omitempty"` - LogFileCount uint32 `protobuf:"varint,5,opt,name=logFileCount,proto3" json:"logFileCount,omitempty"` - CliVersion string `protobuf:"bytes,6,opt,name=cliVersion,proto3" json:"cliVersion,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + state protoimpl.MessageState `protogen:"open.v1"` + Anonymize bool `protobuf:"varint,1,opt,name=anonymize,proto3" json:"anonymize,omitempty"` + SystemInfo bool `protobuf:"varint,3,opt,name=systemInfo,proto3" json:"systemInfo,omitempty"` + UploadURL string `protobuf:"bytes,4,opt,name=uploadURL,proto3" json:"uploadURL,omitempty"` + LogFileCount uint32 `protobuf:"varint,5,opt,name=logFileCount,proto3" json:"logFileCount,omitempty"` + CliVersion string `protobuf:"bytes,6,opt,name=cliVersion,proto3" json:"cliVersion,omitempty"` + // uploadInsecure allows uploading to an http endpoint or one with an + // untrusted TLS certificate. Restricted to privileged callers; for + // self-hosted upload servers. + UploadInsecure bool `protobuf:"varint,7,opt,name=uploadInsecure,proto3" json:"uploadInsecure,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *DebugBundleRequest) Reset() { @@ -2846,6 +2850,13 @@ func (x *DebugBundleRequest) GetCliVersion() string { return "" } +func (x *DebugBundleRequest) GetUploadInsecure() bool { + if x != nil { + return x.UploadInsecure + } + return false +} + type DebugBundleResponse struct { state protoimpl.MessageState `protogen:"open.v1"` Path string `protobuf:"bytes,1,opt,name=path,proto3" json:"path,omitempty"` @@ -7242,7 +7253,7 @@ const file_daemon_proto_rawDesc = "" + "\x12translatedHostname\x18\x04 \x01(\tR\x12translatedHostname\x128\n" + "\x0etranslatedPort\x18\x05 \x01(\v2\x10.daemon.PortInfoR\x0etranslatedPort\"G\n" + "\x17ForwardingRulesResponse\x12,\n" + - "\x05rules\x18\x01 \x03(\v2\x16.daemon.ForwardingRuleR\x05rules\"\xb4\x01\n" + + "\x05rules\x18\x01 \x03(\v2\x16.daemon.ForwardingRuleR\x05rules\"\xdc\x01\n" + "\x12DebugBundleRequest\x12\x1c\n" + "\tanonymize\x18\x01 \x01(\bR\tanonymize\x12\x1e\n" + "\n" + @@ -7252,7 +7263,8 @@ const file_daemon_proto_rawDesc = "" + "\flogFileCount\x18\x05 \x01(\rR\flogFileCount\x12\x1e\n" + "\n" + "cliVersion\x18\x06 \x01(\tR\n" + - "cliVersion\"}\n" + + "cliVersion\x12&\n" + + "\x0euploadInsecure\x18\a \x01(\bR\x0euploadInsecure\"}\n" + "\x13DebugBundleResponse\x12\x12\n" + "\x04path\x18\x01 \x01(\tR\x04path\x12 \n" + "\vuploadedKey\x18\x02 \x01(\tR\vuploadedKey\x120\n" + diff --git a/client/proto/daemon.proto b/client/proto/daemon.proto index 8d5294eb7..3c31156ec 100644 --- a/client/proto/daemon.proto +++ b/client/proto/daemon.proto @@ -536,6 +536,10 @@ message DebugBundleRequest { string uploadURL = 4; uint32 logFileCount = 5; string cliVersion = 6; + // uploadInsecure allows uploading to an http endpoint or one with an + // untrusted TLS certificate. Restricted to privileged callers; for + // self-hosted upload servers. + bool uploadInsecure = 7; } message DebugBundleResponse { diff --git a/client/server/debug.go b/client/server/debug.go index 0b6ac4b53..60a401b0e 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -7,18 +7,62 @@ import ( "context" "errors" "fmt" + "path/filepath" "runtime/pprof" + "strings" + "time" log "github.com/sirupsen/logrus" + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/internal/debug" + "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/client/proto" mgmProto "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/version" ) // DebugBundle creates a debug bundle and returns the location. -func (s *Server) DebugBundle(_ context.Context, req *proto.DebugBundleRequest) (resp *proto.DebugBundleResponse, err error) { +func (s *Server) DebugBundle(callerCtx context.Context, req *proto.DebugBundleRequest) (resp *proto.DebugBundleResponse, err error) { + if err := requirePrivilegeForUploadURL(callerCtx, req.GetUploadURL(), req.GetUploadInsecure()); err != nil { + return nil, err + } + + // The UI log is opened as whoever asked for this bundle, so a caller only + // collects a log it owns (privileged callers excepted). ok is false on a + // socket that carries no identity, which skips the UI log. + callerID, callerIdentified := ipcauth.CallerIdentity(callerCtx) + + path, managementURL, err := s.generateDebugBundle(req, uiLogOpener(callerID, callerIdentified)) + if err != nil { + return nil, err + } + + if req.GetUploadURL() == "" { + return &proto.DebugBundleResponse{Path: path}, nil + } + + // The upload runs without s.mutex held: it does network I/O to a possibly + // slow destination and must not block the other RPCs that take the lock. The + // bounded context is a backstop against a hung connection. + uploadCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + key, err := debug.UploadDebugBundle(uploadCtx, req.GetUploadURL(), managementURL, path, req.GetUploadInsecure()) + if err != nil { + log.Errorf("failed to upload debug bundle to %s: %v", req.GetUploadURL(), err) + return &proto.DebugBundleResponse{Path: path, UploadFailureReason: err.Error()}, nil + } + + log.Infof("debug bundle uploaded to %s with key %s", req.GetUploadURL(), key) + + return &proto.DebugBundleResponse{Path: path, UploadedKey: key}, nil +} + +// generateDebugBundle builds the bundle under s.mutex and returns its path plus +// the management URL captured under the lock, so the caller can run the upload +// without holding the lock. +func (s *Server) generateDebugBundle(req *proto.DebugBundleRequest, uiOpener debug.LogOpener) (path string, managementURL string, err error) { s.mutex.Lock() defer s.mutex.Unlock() @@ -68,6 +112,7 @@ func (s *Server) DebugBundle(_ context.Context, req *proto.DebugBundleRequest) ( SyncResponse: syncResponse, LogPath: s.logFile, UILogPath: s.uiLogPath, + UILogOpener: uiOpener, CPUProfile: cpuProfileData, CapturePath: capturePath, RefreshStatus: refreshStatus, @@ -82,23 +127,16 @@ func (s *Server) DebugBundle(_ context.Context, req *proto.DebugBundleRequest) ( }, ) - path, err := bundleGenerator.Generate() + path, err = bundleGenerator.Generate() if err != nil { - return nil, fmt.Errorf("generate debug bundle: %w", err) + return "", "", fmt.Errorf("generate debug bundle: %w", err) } - if req.GetUploadURL() == "" { - return &proto.DebugBundleResponse{Path: path}, nil - } - key, err := debug.UploadDebugBundle(context.Background(), req.GetUploadURL(), s.config.ManagementURL.String(), path) - if err != nil { - log.Errorf("failed to upload debug bundle to %s: %v", req.GetUploadURL(), err) - return &proto.DebugBundleResponse{Path: path, UploadFailureReason: err.Error()}, nil + if s.config != nil && s.config.ManagementURL != nil { + managementURL = s.config.ManagementURL.String() } - log.Infof("debug bundle uploaded to %s with key %s", req.GetUploadURL(), key) - - return &proto.DebugBundleResponse{Path: path, UploadedKey: key}, nil + return path, managementURL, nil } // GetLogLevel gets the current logging level for the server. @@ -138,12 +176,34 @@ func (s *Server) SetLogLevel(_ context.Context, req *proto.SetLogLevelRequest) ( // RegisterUILog records the desktop UI's absolute log path so DebugBundle can // collect the GUI log. The daemon runs as root and can't resolve the user's // config dir, so the UI reports it. Last-writer-wins (one UI per socket). -func (s *Server) RegisterUILog(_ context.Context, req *proto.RegisterUILogRequest) (*proto.RegisterUILogResponse, error) { +// +// The path arrives over an IPC any local user can reach and is later opened by +// a root daemon, so it is constrained to the file name the UI writes and to a +// local absolute path. Authorization happens when DebugBundle opens it: the +// bundle refuses a file its requester does not own. A caller the daemon cannot +// identify cannot register a path at all. +func (s *Server) RegisterUILog(callerCtx context.Context, req *proto.RegisterUILogRequest) (*proto.RegisterUILogResponse, error) { + if _, ok := ipcauth.CallerIdentity(callerCtx); !ok { + return nil, gstatus.Error(codes.PermissionDenied, + "registering a UI log path requires a control channel that carries the caller's identity") + } + + path := filepath.Clean(req.GetPath()) + if !filepath.IsAbs(path) || filepath.Base(path) != uiLogFileName { + return nil, gstatus.Errorf(codes.InvalidArgument, "UI log path must be an absolute path ending in %s", uiLogFileName) + } + // filepath.IsAbs accepts a Windows UNC path (\\host\share\...) and a device + // path (\\.\, \\?\); opening one would make the root daemon reach a remote + // or device namespace. Require a plain local path. + if strings.HasPrefix(path, `\\`) { + return nil, gstatus.Error(codes.InvalidArgument, "UI log path must be a local path, not a UNC or device path") + } + s.mutex.Lock() defer s.mutex.Unlock() - s.uiLogPath = req.GetPath() - log.Infof("registered UI log path: %s", s.uiLogPath) + s.uiLogPath = path + log.Infof("registered UI log path %s", s.uiLogPath) return &proto.RegisterUILogResponse{}, nil } diff --git a/client/server/debug_gate.go b/client/server/debug_gate.go new file mode 100644 index 000000000..983a13aaf --- /dev/null +++ b/client/server/debug_gate.go @@ -0,0 +1,99 @@ +//go:build !android && !ios + +package server + +import ( + "context" + "fmt" + "net/url" + "os" + "strings" + + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" + + "github.com/netbirdio/netbird/client/configs" + "github.com/netbirdio/netbird/client/internal/debug" + "github.com/netbirdio/netbird/client/internal/ipcauth" + "github.com/netbirdio/netbird/upload-server/types" +) + +// uiLogFileName is the only file name the daemon accepts as a UI log path. The +// UI (writer), this validation, and the bundle collector all read it from +// configs so they cannot drift. +const uiLogFileName = configs.UILogFile + +// uiLogOpener opens the registered UI log, and its rotated siblings, on behalf +// of the caller requesting the bundle: OpenOwnedFile then collects the log only +// when that caller owns it (or is privileged). identified is false on a socket +// that carries no caller identity, in which case nothing is opened. +func uiLogOpener(id ipcauth.Identity, identified bool) debug.LogOpener { + return func(path string) (*os.File, error) { + if !identified { + return nil, fmt.Errorf("bundle requester has no verified identity") + } + return ipcauth.OpenOwnedFile(id, path) + } +} + +// requirePrivilegeForUploadURL restricts where the daemon may send a debug +// bundle. The bundle holds the daemon's own logs and state, and the daemon +// fetches the upload URL itself, so an unrestricted endpoint turns the daemon +// into both an exfiltration channel and a request forwarder that reaches +// services only it can talk to. +// +// The upload service NetBird publishes is open to any caller, since that is what +// the CLI and the desktop UI use. Any other endpoint, self-hosted upload servers +// included, requires a privileged caller. Plaintext is refused for everyone: the +// daemon fetches the URL and then PUTs the bundle to whatever that fetch returns, +// so an http hop is a place to intercept the bundle or the redirect. +// +// insecure relaxes transport security (http, or an untrusted TLS certificate) +// for a self-hosted server. It weakens a root-privileged upload, so it is +// refused for an unprivileged caller regardless of the host. +func requirePrivilegeForUploadURL(ctx context.Context, rawURL string, insecure bool) error { + if rawURL == "" { + return nil + } + + parsed, err := url.Parse(rawURL) + if err != nil { + return gstatus.Errorf(codes.InvalidArgument, "parse upload URL: %v", err) + } + + // --insecure relaxes https to http or an untrusted certificate; it does not + // widen the URL to arbitrary schemes, so a host and http/https are required + // before the insecure branch takes over. + if parsed.Host == "" || (parsed.Scheme != "https" && parsed.Scheme != "http") { + return gstatus.Errorf(codes.InvalidArgument, "upload URL must be http or https with a host") + } + + if insecure { + return denyPrivileged(ctx, + "uploading a debug bundle without transport security (--upload-bundle-insecure)", + ipcauth.ElevatedCommand("netbird debug bundle -U --upload-bundle-insecure --upload-bundle-url ")) + } + + if parsed.Scheme != "https" { + return gstatus.Errorf(codes.InvalidArgument, "upload URL must use https, got scheme %q", parsed.Scheme) + } + + if isDefaultUploadService(parsed) { + return nil + } + + return denyPrivileged(ctx, + "uploading a debug bundle to an upload service other than the default one", + ipcauth.ElevatedCommand("netbird debug bundle -U --upload-bundle-url ")) +} + +// isDefaultUploadService reports whether the URL points at the upload service +// NetBird runs. Only the host is compared: the service's path may differ between +// releases, and the host is what decides who receives the bundle. +func isDefaultUploadService(parsed *url.URL) bool { + defaultURL, err := url.Parse(types.DefaultBundleURL) + if err != nil { + return false + } + return parsed.Scheme == defaultURL.Scheme && strings.EqualFold(parsed.Host, defaultURL.Host) +} diff --git a/client/server/debug_gate_test.go b/client/server/debug_gate_test.go new file mode 100644 index 000000000..e958fc581 --- /dev/null +++ b/client/server/debug_gate_test.go @@ -0,0 +1,157 @@ +//go:build !android && !ios + +package server + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" + + "github.com/netbirdio/netbird/client/internal/ipcauth" + "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/upload-server/types" +) + +func TestRegisterUILogRefusesUnidentifiedCaller(t *testing.T) { + s := &Server{} + + _, err := s.RegisterUILog(noIdentityCtx(), &proto.RegisterUILogRequest{ + Path: filepath.Join(t.TempDir(), uiLogFileName), + }) + + if gstatus.Code(err) != codes.PermissionDenied { + t.Fatalf("code = %v, want PermissionDenied", gstatus.Code(err)) + } +} + +func TestRegisterUILogRefusesForeignPath(t *testing.T) { + secret := "/etc/shadow" + if runtime.GOOS == "windows" { + secret = `C:\Windows\System32\config\SAM` + } + + tests := []struct { + name string + path string + }{ + {"empty", ""}, + {"relative", filepath.Join("netbird", uiLogFileName)}, + {"another file", secret}, + {"directory of the log", t.TempDir()}, + {"unc path", `\\attacker\share\` + uiLogFileName}, + {"device path", `\\.\C:\` + uiLogFileName}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s := &Server{} + + _, err := s.RegisterUILog(userCtx(), &proto.RegisterUILogRequest{Path: tc.path}) + + if gstatus.Code(err) != codes.InvalidArgument { + t.Fatalf("code = %v, want InvalidArgument", gstatus.Code(err)) + } + if s.uiLogPath != "" { + t.Fatalf("path %q was recorded despite the refusal", s.uiLogPath) + } + }) + } +} + +func TestRegisterUILogRecordsPath(t *testing.T) { + s := &Server{} + path := filepath.Join(t.TempDir(), uiLogFileName) + + if _, err := s.RegisterUILog(userCtx(), &proto.RegisterUILogRequest{Path: path}); err != nil { + t.Fatalf("register: %v", err) + } + + if s.uiLogPath != path { + t.Fatalf("path = %q, want %q", s.uiLogPath, path) + } +} + +// The UI log is opened as the bundle requester, so a second local user cannot +// collect a log they do not own, and an unidentified requester collects nothing. +func TestUILogOpenerBindsToRequester(t *testing.T) { + path := filepath.Join(t.TempDir(), uiLogFileName) + if err := os.WriteFile(path, []byte("log line"), 0600); err != nil { + t.Fatalf("write log: %v", err) + } + + // A different unprivileged user than the file's owner: refused. + if _, err := uiLogOpener(unprivilegedIdentity(), true)(path); err == nil { + t.Fatal("expected a file the requester does not own to be refused") + } + + // No verified identity: refused. + if _, err := uiLogOpener(ipcauth.Identity{}, false)(path); err == nil { + t.Fatal("expected an unidentified requester to be refused") + } + + // The requester that owns the file: allowed. The test process created it, so + // its own identity is the owner (and a privileged runner is exempt anyway). + owner, err := ipcauth.CurrentProcessIdentity() + if err != nil { + t.Fatalf("current identity: %v", err) + } + f, err := uiLogOpener(owner, true)(path) + if err != nil { + t.Fatalf("expected the owning requester to be allowed, got %v", err) + } + _ = f.Close() +} + +func TestRequirePrivilegeForUploadURL(t *testing.T) { + tests := []struct { + name string + url string + insecure bool + unprivOK bool + invalid bool + rootAlso bool + }{ + {name: "no upload", url: "", unprivOK: true}, + {name: "default service", url: types.DefaultBundleURL, unprivOK: true}, + {name: "default service, other path", url: "https://upload.debug.netbird.io/other", unprivOK: true}, + {name: "loopback exfiltration endpoint", url: "https://127.0.0.1:8080/upload-url", rootAlso: true}, + {name: "custom upload service", url: "https://attacker.example/upload-url", rootAlso: true}, + {name: "plaintext default host", url: "http://upload.debug.netbird.io/upload-url", invalid: true}, + {name: "plaintext custom host", url: "http://attacker.example/upload-url", invalid: true}, + {name: "unsupported scheme", url: "file:///etc/shadow", invalid: true}, + // insecure relaxes transport security; privileged only, whatever the host. + {name: "insecure http custom", url: "http://selfhosted.local/upload-url", insecure: true, rootAlso: true}, + {name: "insecure https custom", url: "https://selfhosted.local/upload-url", insecure: true, rootAlso: true}, + {name: "insecure default host", url: types.DefaultBundleURL, insecure: true, rootAlso: true}, + // --insecure must not widen the URL to non-http(s) schemes or a hostless URL. + {name: "insecure file scheme", url: "file:///etc/shadow", insecure: true, invalid: true}, + {name: "insecure hostless", url: "https:///upload-url", insecure: true, invalid: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := requirePrivilegeForUploadURL(userCtx(), tc.url, tc.insecure) + + switch { + case tc.invalid: + if gstatus.Code(err) != codes.InvalidArgument { + t.Fatalf("code = %v, want InvalidArgument", gstatus.Code(err)) + } + return + case tc.unprivOK: + assertAllowed(t, err) + return + default: + assertDenied(t, err) + } + + if tc.rootAlso { + assertAllowed(t, requirePrivilegeForUploadURL(rootCtx(), tc.url, tc.insecure)) + } + }) + } +} diff --git a/client/server/server.go b/client/server/server.go index 6e22a76a9..aaab5cc02 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -72,6 +72,9 @@ type Server struct { // RegisterUILog. Guarded by mutex. Consumed by DebugBundle so the bundle // can collect the GUI log even though the daemon runs as root and can't // resolve the user's config dir. Last-writer-wins (one UI per socket). + // DebugBundle opens it on behalf of the bundle requester and refuses a file + // that caller does not own, so a local user cannot read another user's log + // or a root-only file through it. uiLogPath string oauthAuthFlow oauthAuthFlow diff --git a/client/ui/uilogpath.go b/client/ui/uilogpath.go index 96fbb9637..6fa400e01 100644 --- a/client/ui/uilogpath.go +++ b/client/ui/uilogpath.go @@ -8,21 +8,19 @@ import ( log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/client/configs" "github.com/netbirdio/netbird/client/ui/guilog" ) -// uiLogFileName must stay in sync with the daemon's "gui-client*.log.*" glob -// for rotated siblings (addUILog in client/internal/debug). -const uiLogFileName = "gui-client.log" - // uiLogPath returns the GUI log path with native separators, since the daemon -// opens it directly for debug-bundle collection. +// opens it directly for debug-bundle collection. The file name comes from +// configs.UILogFile so the daemon validates and collects the same name. func uiLogPath() (string, error) { dir, err := os.UserConfigDir() if err != nil { return "", err } - return filepath.Join(dir, "netbird", uiLogFileName), nil + return filepath.Join(dir, "netbird", configs.UILogFile), nil } // newDebugLog builds the GUI debug log, disabled when userSetLogFile is set