From 6fbfccc2d3a40f848dbe77e858d044cd6dfc6737 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 26 Aug 2024 19:31:21 +0200 Subject: [PATCH 1/3] fs: fix race condition in get/set security descriptor Calling `Load()` twice for an atomic variable can return different values each time. This resulted in trying to read the security descriptor with high privileges, but then not entering the code path to switch to low privileges when another thread has already done so concurrently. --- internal/fs/sd_windows.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/fs/sd_windows.go b/internal/fs/sd_windows.go index 0a73cbe53..bccf74992 100644 --- a/internal/fs/sd_windows.go +++ b/internal/fs/sd_windows.go @@ -48,13 +48,15 @@ func GetSecurityDescriptor(filePath string) (securityDescriptor *[]byte, err err var sd *windows.SECURITY_DESCRIPTOR - if lowerPrivileges.Load() { + // store original value to avoid unrelated changes in the error check + useLowerPrivileges := lowerPrivileges.Load() + if useLowerPrivileges { sd, err = getNamedSecurityInfoLow(filePath) } else { sd, err = getNamedSecurityInfoHigh(filePath) } if err != nil { - if !lowerPrivileges.Load() && isHandlePrivilegeNotHeldError(err) { + if !useLowerPrivileges && isHandlePrivilegeNotHeldError(err) { // If ERROR_PRIVILEGE_NOT_HELD is encountered, fallback to backups/restores using lower non-admin privileges. lowerPrivileges.Store(true) sd, err = getNamedSecurityInfoLow(filePath) @@ -109,14 +111,16 @@ func SetSecurityDescriptor(filePath string, securityDescriptor *[]byte) error { sacl = nil } - if lowerPrivileges.Load() { + // store original value to avoid unrelated changes in the error check + useLowerPrivileges := lowerPrivileges.Load() + if useLowerPrivileges { err = setNamedSecurityInfoLow(filePath, dacl) } else { err = setNamedSecurityInfoHigh(filePath, owner, group, dacl, sacl) } if err != nil { - if !lowerPrivileges.Load() && isHandlePrivilegeNotHeldError(err) { + if !useLowerPrivileges && isHandlePrivilegeNotHeldError(err) { // If ERROR_PRIVILEGE_NOT_HELD is encountered, fallback to backups/restores using lower non-admin privileges. lowerPrivileges.Store(true) err = setNamedSecurityInfoLow(filePath, dacl) From 9c70794886930d12bce2807d0e6c07aee99c69d8 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 26 Aug 2024 19:36:43 +0200 Subject: [PATCH 2/3] fs: fix error handling for retried get/set of security descriptor The retry code path did not filter `ERROR_NOT_SUPPORTED`. Just call the original function a second time to correctly follow the low privilege code path. --- internal/fs/sd_windows.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/internal/fs/sd_windows.go b/internal/fs/sd_windows.go index bccf74992..0004f1809 100644 --- a/internal/fs/sd_windows.go +++ b/internal/fs/sd_windows.go @@ -59,10 +59,7 @@ func GetSecurityDescriptor(filePath string) (securityDescriptor *[]byte, err err if !useLowerPrivileges && isHandlePrivilegeNotHeldError(err) { // If ERROR_PRIVILEGE_NOT_HELD is encountered, fallback to backups/restores using lower non-admin privileges. lowerPrivileges.Store(true) - sd, err = getNamedSecurityInfoLow(filePath) - if err != nil { - return nil, fmt.Errorf("get low-level named security info failed with: %w", err) - } + return GetSecurityDescriptor(filePath) } else if errors.Is(err, windows.ERROR_NOT_SUPPORTED) { return nil, nil } else { @@ -123,10 +120,7 @@ func SetSecurityDescriptor(filePath string, securityDescriptor *[]byte) error { if !useLowerPrivileges && isHandlePrivilegeNotHeldError(err) { // If ERROR_PRIVILEGE_NOT_HELD is encountered, fallback to backups/restores using lower non-admin privileges. lowerPrivileges.Store(true) - err = setNamedSecurityInfoLow(filePath, dacl) - if err != nil { - return fmt.Errorf("set low-level named security info failed with: %w", err) - } + return SetSecurityDescriptor(filePath, securityDescriptor) } else { return fmt.Errorf("set named security info failed with: %w", err) } From 45d05eb691437a06ab8c1079e462421c75b349f3 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 26 Aug 2024 19:43:18 +0200 Subject: [PATCH 3/3] add changelog for security descriptor race condition --- changelog/unreleased/issue-5004 | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/unreleased/issue-5004 diff --git a/changelog/unreleased/issue-5004 b/changelog/unreleased/issue-5004 new file mode 100644 index 000000000..529b65464 --- /dev/null +++ b/changelog/unreleased/issue-5004 @@ -0,0 +1,12 @@ +Bugfix: Fix spurious "A Required Privilege Is Not Held by the Client" error + +On Windows, creating a backup could sometimes print the following error + +``` +error: nodeFromFileInfo [...]: get named security info failed with: a required privilege is not held by the client. +``` + +This has been fixed. + +https://github.com/restic/restic/issues/5004 +https://github.com/restic/restic/pull/5019