From d5a9f5b0ffd2349ed11ff03d5c186cf311c4daa3 Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 29 Sep 2026 14:29:27 +0200 Subject: [PATCH] [client] Do not refuse a caller's upload URL that an MDM policy overrides anyway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The privilege gate ran on the URL the caller named before the resolver applied the MDM policy, so on a managed device an unprivileged `netbird debug bundle -U --upload-bundle-url ` was refused with "requires root" — even though the policy would have discarded that URL and uploaded to the pinned destination. Gating a value that has no effect only turns a working bundle into a denial. Read the policy before the gate and tell the gate the destination is pinned, so it skips the caller-URL branch. --upload-bundle-insecure stays gated either way: relaxing TLS towards the pinned host is a real weakening, and that one is the caller's doing. Reported by cubic on #7514. --- client/server/debug.go | 32 ++++++++++++++++++++++---------- client/server/debug_gate.go | 11 ++++++++++- client/server/debug_gate_test.go | 25 ++++++++++++++++--------- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/client/server/debug.go b/client/server/debug.go index 966b65e56..01067f76b 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -27,7 +27,8 @@ import ( // DebugBundle creates a debug bundle and returns the location. func (s *Server) DebugBundle(callerCtx context.Context, req *proto.DebugBundleRequest) (resp *proto.DebugBundleResponse, err error) { - if err := requirePrivilegeForUploadURL(callerCtx, req.GetUploadURL(), req.GetUploadInsecure(), req.GetUpload()); err != nil { + mdmUploadURL := s.mdmDebugUploadURL() + if err := requirePrivilegeForUploadURL(callerCtx, req.GetUploadURL(), req.GetUploadInsecure(), req.GetUpload(), mdmUploadURL != ""); err != nil { return nil, err } @@ -36,7 +37,7 @@ func (s *Server) DebugBundle(callerCtx context.Context, req *proto.DebugBundleRe // socket that carries no identity, which skips the UI log. callerID, callerIdentified := ipcauth.CallerIdentity(callerCtx) - path, managementURL, publishedUploadURL, mdmUploadURL, err := s.generateDebugBundle(req, uiLogOpener(callerID, callerIdentified)) + path, managementURL, publishedUploadURL, err := s.generateDebugBundle(req, uiLogOpener(callerID, callerIdentified)) if err != nil { return nil, err } @@ -86,7 +87,7 @@ func redactUploadURL(raw string) string { // the management URL and the upload service the management server publishes, // both 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, publishedUploadURL string, mdmUploadURL string, err error) { +func (s *Server) generateDebugBundle(req *proto.DebugBundleRequest, uiOpener debug.LogOpener) (path string, managementURL string, publishedUploadURL string, err error) { s.mutex.Lock() defer s.mutex.Unlock() @@ -155,17 +156,28 @@ func (s *Server) generateDebugBundle(req *proto.DebugBundleRequest, uiOpener deb path, err = bundleGenerator.Generate() if err != nil { - return "", "", "", "", fmt.Errorf("generate debug bundle: %w", err) + return "", "", "", fmt.Errorf("generate debug bundle: %w", err) } - if s.config != nil { - if s.config.ManagementURL != nil { - managementURL = s.config.ManagementURL.String() - } - mdmUploadURL = s.config.DebugBundleUploadURL + if s.config != nil && s.config.ManagementURL != nil { + managementURL = s.config.ManagementURL.String() } - return path, managementURL, publishedUploadURL, mdmUploadURL, nil + return path, managementURL, publishedUploadURL, nil +} + +// mdmDebugUploadURL reports the debug-bundle destination an MDM policy pins on +// this device, empty when none does. Read before the bundle is generated, +// because the privilege gate needs to know whether the caller's own URL can have +// any effect. +func (s *Server) mdmDebugUploadURL() string { + s.mutex.Lock() + defer s.mutex.Unlock() + + if s.config == nil { + return "" + } + return s.config.DebugBundleUploadURL } // GetLogLevel gets the current logging level for the server. diff --git a/client/server/debug_gate.go b/client/server/debug_gate.go index ba473c397..8ebee8678 100644 --- a/client/server/debug_gate.go +++ b/client/server/debug_gate.go @@ -52,7 +52,16 @@ func uiLogOpener(id ipcauth.Identity, identified bool) debug.LogOpener { // for a self-hosted server. It weakens a root-privileged upload, so it is // refused for an unprivileged caller regardless of the host. upload says whether // the request asks for an upload at all; without one there is nothing to weaken. -func requirePrivilegeForUploadURL(ctx context.Context, rawURL string, insecure, upload bool) error { +// +// mdmPinned says an MDM policy already fixes the destination. The caller's URL +// is then discarded before the upload, so gating on it would only turn a bundle +// that was going to the pinned host anyway into a refusal. Transport security +// still is gated: relaxing TLS towards the pinned host is a real weakening. +func requirePrivilegeForUploadURL(ctx context.Context, rawURL string, insecure, upload, mdmPinned bool) error { + if mdmPinned { + rawURL = "" + } + if rawURL == "" { // An empty URL with upload requested is not "no upload": the daemon then // resolves the destination the management server published. Relaxing TLS diff --git a/client/server/debug_gate_test.go b/client/server/debug_gate_test.go index a8ce2da0e..5a10b0131 100644 --- a/client/server/debug_gate_test.go +++ b/client/server/debug_gate_test.go @@ -108,13 +108,14 @@ func TestUILogOpenerBindsToRequester(t *testing.T) { func TestRequirePrivilegeForUploadURL(t *testing.T) { tests := []struct { - name string - url string - insecure bool - noUpload bool - unprivOK bool - invalid bool - rootAlso bool + name string + url string + insecure bool + noUpload bool + mdmPinned bool + unprivOK bool + invalid bool + rootAlso bool }{ {name: "no upload", url: "", unprivOK: true}, // An empty URL resolves to the destination management published, so @@ -126,6 +127,12 @@ func TestRequirePrivilegeForUploadURL(t *testing.T) { {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}, + // With MDM pinning the destination the caller's URL is discarded before + // the upload, so refusing it would only turn a bundle that was going to + // the pinned host anyway into a denial. + {name: "custom URL ignored when MDM pins the destination", url: "https://attacker.example/upload-url", mdmPinned: true, unprivOK: true}, + // Transport security is still the caller's to weaken, pinned or not. + {name: "insecure still gated when MDM pins the destination", url: "https://attacker.example/upload-url", insecure: true, mdmPinned: true, 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}, @@ -140,7 +147,7 @@ func TestRequirePrivilegeForUploadURL(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - err := requirePrivilegeForUploadURL(userCtx(), tc.url, tc.insecure, !tc.noUpload) + err := requirePrivilegeForUploadURL(userCtx(), tc.url, tc.insecure, !tc.noUpload, tc.mdmPinned) switch { case tc.invalid: @@ -156,7 +163,7 @@ func TestRequirePrivilegeForUploadURL(t *testing.T) { } if tc.rootAlso { - assertAllowed(t, requirePrivilegeForUploadURL(rootCtx(), tc.url, tc.insecure, !tc.noUpload)) + assertAllowed(t, requirePrivilegeForUploadURL(rootCtx(), tc.url, tc.insecure, !tc.noUpload, tc.mdmPinned)) } }) }