mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-01 19:19:07 +02:00
[client] Do not refuse a caller's upload URL that an MDM policy overrides anyway
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 <host>` 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.
This commit is contained in:
+22
-10
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user