diff --git a/client/server/debug.go b/client/server/debug.go index 01067f76b..735799618 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -27,8 +27,13 @@ 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) { + // One source of truth for "this request uploads", used by both the gate and + // the local-only early return below. A client older than the upload flag asks + // for an upload by naming a URL and nothing else, so the two must not drift. + wantsUpload := req.GetUpload() || req.GetUploadURL() != "" + mdmUploadURL := s.mdmDebugUploadURL() - if err := requirePrivilegeForUploadURL(callerCtx, req.GetUploadURL(), req.GetUploadInsecure(), req.GetUpload(), mdmUploadURL != ""); err != nil { + if err := requirePrivilegeForUploadURL(callerCtx, req.GetUploadURL(), req.GetUploadInsecure(), wantsUpload, mdmUploadURL != ""); err != nil { return nil, err } @@ -42,7 +47,7 @@ func (s *Server) DebugBundle(callerCtx context.Context, req *proto.DebugBundleRe return nil, err } - if !req.GetUpload() && req.GetUploadURL() == "" { + if !wantsUpload { return &proto.DebugBundleResponse{Path: path}, nil } diff --git a/client/server/debug_gate.go b/client/server/debug_gate.go index 8ebee8678..a29f5ade1 100644 --- a/client/server/debug_gate.go +++ b/client/server/debug_gate.go @@ -58,6 +58,13 @@ func uiLogOpener(id ipcauth.Identity, identified bool) debug.LogOpener { // 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 { + // A named URL is itself a request to upload, since DebugBundle uploads when + // either the flag or the URL is set and a client older than the flag sends + // only the URL. Derived here rather than taken on trust from the caller: + // blanking rawURL below would otherwise turn an absent flag into "no upload + // to weaken" and let an unprivileged caller relax TLS on a root-run upload. + upload = upload || rawURL != "" + if mdmPinned { rawURL = "" } diff --git a/client/server/debug_gate_test.go b/client/server/debug_gate_test.go index 5a10b0131..e844e57bd 100644 --- a/client/server/debug_gate_test.go +++ b/client/server/debug_gate_test.go @@ -133,6 +133,12 @@ func TestRequirePrivilegeForUploadURL(t *testing.T) { {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}, + // A named URL is itself a request to upload: DebugBundle uploads when + // either the flag or the URL is set. A client older than the upload flag + // sends exactly this shape, URL and no flag, and with MDM blanking the URL + // the gate must not read the missing flag as "no upload to weaken". + {name: "insecure, URL set, no upload flag, MDM pinned", url: "https://x.example/upload-url", insecure: true, noUpload: true, mdmPinned: true, rootAlso: true}, + {name: "insecure, URL set, no upload flag", url: "https://x.example/upload-url", insecure: true, noUpload: 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},