Back up and restore SecurityDescriptors on Windows #4708

aneesh-n · 2024-02-24T20:43:18Z

What does this PR change? What problem does it solve?

Restic did not back up SecurityDescriptors of files on Windows.
Restic now backs up and restores SecurityDescriptors (which includes owner, group, discretionary access control list (DACL), system access control list (SACL)) when backing up files and folders on Windows. This requires the user to be a member of backup operators or the application must be run as admin.
If that is not the case, only the current user's owner, group and DACL will be backed up and during restore only the DACL of the backed file will be restored while the current user's owner and group will be set during the restore.

We may want to add a backup/restore flag for disabling Security descriptor for Windows as there may be valid cases where users do not want to backup/restore security descriptors of files in Windows.
This can be done as a separate PR.

Was the change previously discussed in an issue or on the forum?

Follow-up to #4611

Checklist

I have read the contribution guidelines.
I have enabled maintainer edits.
I have added tests for all code changes.
I have added documentation for relevant changes (in the manual).
There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
I have run gofmt on the code in all commits.
All commit messages are formatted in the same style as the other commits in the repo.
I'm done! This pull request is ready for review.

internal/fs/sd_windows.go

internal/restic/node_windows.go

internal/restic/node.go

MichaelEischer

Sorry for taking so long to review this PR, somehow it always ended up in my "it would take too long to review the PR today" category :-( . I've added quite a few comments below, although most of them are just nitpicks.

The only larger issue is the encoding of the SECURITY_DESCRIPTOR struct. I don't think it's a good idea to store the raw data structure. Instead, I think the PR should use the SDDL encoding, see the comment for more details.

changelog/unreleased/pull-4708

doc/040_backup.rst

internal/restic/node_windows.go

internal/restic/node.go

internal/fs/sd_windows.go

MichaelEischer · 2024-04-28T13:58:52Z

internal/fs/sd_windows.go

+	return string(utf16.Decode(displayNameBuffer[:displayBufSize]))
+}
+
+func adjustTokenPrivileges(token windows.Token, releaseAll bool, input *byte, outputSize uint32, output *byte, requiredSize *uint32) (success bool, err error) {


I guess those functions are autogenerated?

Probably, these were copied from winio.

I've found the source file: https://github.com/microsoft/go-winio/blob/main/zsyscall_windows.go . I don't want to see copied code ever again that does not clearly state where it originates from. It's just ridiculous that I have to search for the original source of these functions.

Omitting that information also forces me to waste time on reviewing known good code.

To make matter worse, there have been a few small changes to those functions (apparently to fix linter warnings), which when compared to the latest upstream version of the autogenerated code seem to be buggy. See my other comments for details.

That's a little harsh. I wrote this code 2 years ago before I decided to submit it back to restic.

The upstream code was changed only last month - microsoft/go-winio@3c9576c
Eg.
- r0, _, _ := syscall.Syscall(procHcsFormatWritableLayerVhd.Addr(), 1, uintptr(handle), 0, 0)
+ r0, _, _ := syscall.SyscallN(procHcsFormatWritableLayerVhd.Addr(), uintptr(handle))

The changes I made 2 years ago were to fix the deprecations -
Deprecated: Use SyscallN instead.

Extra 0s at the end of SyscallN are ignored and hence this slip wasn't caught, and it also doesn't cause any bugs, though I agree it should be cleaned.

Also, I get your point on including links to the source and will try to do that, this was missed because the code was created from upstream a long time ago.

That's a little harsh.

Let's just say that I was rather annoyed after noticing that I have to diff the syscall wrappers and search for the corresponding source code.

I wrote this code 2 years ago before I decided to submit it back to restic.

That perfectly explains why the code looks the way it does. Thanks for contributing those changes!

internal/fs/sd_windows_test.go

internal/fs/sd_windows.go

MichaelEischer · 2024-05-05T18:53:26Z

internal/fs/sd_windows.go

+	}
+	r0, _, e1 := syscall.SyscallN(procAdjustTokenPrivileges.Addr(), uintptr(token), uintptr(_p0), uintptr(unsafe.Pointer(input)), uintptr(outputSize), uintptr(unsafe.Pointer(output)), uintptr(unsafe.Pointer(requiredSize)))
+	success = r0 != 0
+	if !success {


The autogenerated code contains if true { here. Why deviate here?

This windows api always returns an error even in case of success, warnings (partial success) and error cases.

Full success - When we call this with admin permissions, it returns DNS_ERROR_RCODE_NO_ERROR (0). This gets translated to errErrorEinval and ultimately in adjustTokenPrivileges, it gets ignored.

Partial success - If we call this api without admin privileges, privileges related to SACLs do not get set and though the api returns success, it returns an error - golang.org/x/sys/windows.ERROR_NOT_ALL_ASSIGNED (1300)
This gets propagated up the call hierarchy and then gets printed as a debug log ("error enabling backup/restore priviledge") in enableRestorePrivilege or enableBackupPrivilege.
Now that we have changed the handling in those functions to print debug log, it will start printing debug logs even in case ERROR_NOT_ALL_ASSIGNED was thrown, which is a warning and in our case is expected in case we run restic without admin permissions. In that case adjustPrivileges will return a PrivilegeError even though other privileges other than SACL access were still set.

Instead of eating the warning on one extreme or throwing a Privilege error on the other extreme, I can handle this warning in adjustPrivileges and add a debug statement -

debug.Log("Not all requested privileges were fully set: %v. AdjustTokenPrivileges returned warning: %v", privileges, err)
This debug statement will get logged when we call restic without admin permissions.

The debug.Log is fine.

Judging from https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-adjusttokenprivileges if true { is in fact the correct variant (even though it looks weird. However, calling errnoErr doesn't really make sense in that case), as GetLastError can return either ERROR_SUCCESS or ERROR_NOT_ALL_ASSIGNED if adjustTokenPrivileges succeeded.

MichaelEischer · 2024-05-05T19:04:44Z

internal/fs/sd_windows.go

+	return string(utf16.Decode(displayNameBuffer[:displayBufSize]))
+}
+
+func adjustTokenPrivileges(token windows.Token, releaseAll bool, input *byte, outputSize uint32, output *byte, requiredSize *uint32) (success bool, err error) {


I've found the source file: https://github.com/microsoft/go-winio/blob/main/zsyscall_windows.go . I don't want to see copied code ever again that does not clearly state where it originates from. It's just ridiculous that I have to search for the original source of these functions.

Omitting that information also forces me to waste time on reviewing known good code.

To make matter worse, there have been a few small changes to those functions (apparently to fix linter warnings), which when compared to the latest upstream version of the autogenerated code seem to be buggy. See my other comments for details.

internal/fs/sd_windows.go

Change lowerPrivileges from bool to atomic.Bool. Add missing cleanup from upstream go-winio. Add handling for ERROR_NOT_ALL_ASSIGNED warning.

MichaelEischer

LGTM. Thanks!

aneesh-n added 6 commits February 24, 2024 13:16

Create helper for SecurityDescriptor related functions

09ce1b4

Fix CombineErrors and fillExtendedAttr error handling

e3e59fe

Add support for backup/restore of security descriptors

70cf8e3

Add test cases for security descriptors

90916f5

Update docs for security descriptors

c0a1b9a

Add changelog and fix lint error

5764300

aneesh-n commented Feb 24, 2024

View reviewed changes

internal/fs/sd_windows.go Show resolved Hide resolved

Clean up SecurityDescriptor helper

062d408

aneesh-n commented Feb 24, 2024

View reviewed changes

internal/restic/node_windows.go Show resolved Hide resolved

aneesh-n commented Feb 24, 2024

View reviewed changes

internal/restic/node.go Show resolved Hide resolved

MichaelEischer requested changes Apr 28, 2024

View reviewed changes

aneesh-n added 3 commits April 29, 2024 14:40

Merge branch 'master' into windows-securitydesc

3f76b90

Fix review comments

08c6945

Fix review comments for privileges and security flags

672f6cd

MichaelEischer reviewed May 5, 2024

View reviewed changes

Fix review comments

a4fd1b9

Change lowerPrivileges from bool to atomic.Bool. Add missing cleanup from upstream go-winio. Add handling for ERROR_NOT_ALL_ASSIGNED warning.

MichaelEischer approved these changes May 12, 2024

View reviewed changes

MichaelEischer added this pull request to the merge queue May 12, 2024

Merged via the queue into restic:master with commit 92221c2 May 12, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Back up and restore SecurityDescriptors on Windows #4708

Back up and restore SecurityDescriptors on Windows #4708

aneesh-n commented Feb 24, 2024 •

edited

MichaelEischer left a comment

MichaelEischer Apr 28, 2024

aneesh-n Apr 29, 2024

MichaelEischer May 5, 2024

aneesh-n May 6, 2024

MichaelEischer May 12, 2024

MichaelEischer May 5, 2024

aneesh-n May 6, 2024 •

edited

MichaelEischer May 12, 2024

MichaelEischer May 5, 2024

MichaelEischer left a comment

Back up and restore SecurityDescriptors on Windows #4708

Back up and restore SecurityDescriptors on Windows #4708

Conversation

aneesh-n commented Feb 24, 2024 • edited

What does this PR change? What problem does it solve?

Was the change previously discussed in an issue or on the forum?

Checklist

MichaelEischer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneesh-n May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelEischer left a comment

Choose a reason for hiding this comment

aneesh-n commented Feb 24, 2024 •

edited

aneesh-n May 6, 2024 •

edited