Skip to content

Windows Backup Privilege Tweaks#5424

Merged
MichaelEischer merged 3 commits into
restic:masterfrom
Crazycatz00:sebackup-fixes
Nov 16, 2025
Merged

Windows Backup Privilege Tweaks#5424
MichaelEischer merged 3 commits into
restic:masterfrom
Crazycatz00:sebackup-fixes

Conversation

@Crazycatz00

@Crazycatz00 Crazycatz00 commented Jun 20, 2025 •
edited
Loading

Copy link
Copy Markdown

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

This PR enables backup semantics for file EA opens and attempts to enable SeBackupPrivilege when first beginning a backup.

When enabled, SeBackupPrivilege allows read-only opening that bypasses ACLs; this can permit a restricted account with the proper privilege to back up all users' files without changing file-system permissions. Go supports backup semantics for read-only opens since 405275 / v1.20 and restic currently enables the privilege to backup security descriptors, so such backups mostly work already, with two issues:

  1. Reading extended attributes fails for files when SeBackupPrivilege allows access which would have been denied without the privilege.
    This is because windows.CreateFile is used directly without the appropriate flag.
    (Directories already have the flag as they require it to get a handle at all.)
  2. SeBackupPrivilege is only enabled upon reading the first security descriptor; accesses before this are subject to normal ACLs and may fail. This always affects the scanner, and the actual backup will error & skip initial items until one has permissive enough ACLs.

An example of both issues:

> rem Running in cmd.exe as Administrator
> set RESTIC_PASSWORD=0000
> restic init -r ".\repo"
> mkdir ".\data"

> rem Create directory "b" accessible only to the SYSTEM account
> mkdir ".\data\b" && echo Test > ".\data\b\b.txt" && icacls ".\data\b" /inheritance:r /grant SYSTEM:F

> restic backup -r ".\repo" -nvv ".\data\b"
open repository
repository 5403b32f opened (version 2, compression level auto)
no parent snapshot found, will read all files
load index files
[0:00]          0 index files loaded
start scan on [.\data\b]
start backup on [.\data\b]
scan: openfile for readdirnames failed: open \\?\D:\Test\data\b: Access is denied.
scan finished in 0.003s: 0 files, 0 B
error: incomplete metadata for data\b\b.txt: get EA failed while opening file handle for path data\b\b.txt, with: Access is denied.
new       /data/b/b.txt, saved in 0.003s (8 B added)
new       /data/b/, saved in 0.004s (0 B added, 0 B stored, 412 B metadata)
new       /data/, saved in 0.007s (0 B added, 0 B stored, 429 B metadata)

Files:           1 new,     0 changed,     0 unmodified
Dirs:            2 new,     0 changed,     0 unmodified
Data Blobs:      1 new
Tree Blobs:      3 new
Would add to the repository: 1.778 KiB (1.380 KiB stored)

processed 1 files, 8 B in 0:00
Warning: at least one source file could not be read

> cd ".\data" && restic backup -r "..\repo" -nvv "b"
open repository
repository 5403b32f opened (version 2, compression level auto)
no parent snapshot found, will read all files
load index files
[0:00]          0 index files loaded
start scan on [b]
start backup on [b]
scan: openfile for readdirnames failed: open \\?\D:\Test\data\b: Access is denied.
scan finished in 0.001s: 0 files, 0 B
error: openfile for readdirnames failed: open \\?\D:\Test\data\b: Access is denied.
Fatal: unable to save snapshot: snapshot is empty

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

No.

Checklist

  • I have added tests.
  • [ ] 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'm done! This pull request is ready for review.

@Crazycatz00

Crazycatz00 commented Jul 7, 2025

Copy link
Copy Markdown
Author

Thinking on this, should restic automatically try to enable privileges that cause it to bypass access permissions? Should it be enabled via flag, like VSS is by --use-fs-snapshot?
It's using the privilege for its intended purpose (backup), but it may be unexpected that restic can access files the running (administrator) user normally can't.

@jtru

jtru commented Aug 15, 2025

Copy link
Copy Markdown
Contributor

This is a great feature that I would very much like to have asap, but I think it would be better to have it as an explicit opt-in. Otherwise, I expect all kinds of "endpoint protection" and anti-virus snake oil to trip over restic executables in the near future, which would be a real hassle. Better let users explicitly enable the backup privilege in case they know they may actually attain it.

@ProactiveServices

ProactiveServices commented Aug 15, 2025

Copy link
Copy Markdown
Contributor

Thinking on this, should restic automatically try to enable privileges that cause it to bypass access permissions?

It's not bypassing anything, it's using the access paradigms as they were intended and I'd say it is not violating the principle of least surprise. Backup software is the intended use of this privilege.

MichaelEischer
MichaelEischer requested changes Oct 4, 2025
View reviewed changes

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep the privilege management hidden in the fs package, see below for a suggestion how to achieve that.

Comment thread cmd/restic/cmd_backup.go Outdated
Comment thread internal/fs/sd_windows.go Outdated
@Crazycatz00

Crazycatz00 commented Nov 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

This should be a bit more proper. Contained to the internal/fs module, running just once on init(), and made sure it builds for at least Linux via GOOS. Also rebased to latest and added two tests to verify the behavior.

MichaelEischer
MichaelEischer requested changes Nov 16, 2025
View reviewed changes

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I just have two small nits

Comment thread internal/fs/priv_windows_test.go
Comment thread changelog/unreleased/pull-5424 Outdated
MichaelEischer
MichaelEischer approved these changes Nov 16, 2025
View reviewed changes

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MichaelEischer MichaelEischer merged commit 3826167 into restic:master Nov 16, 2025
22 of 24 checks passed
@amrkmn amrkmn mentioned this pull request Jun 9, 2026
Closed
@BrewTestBot BrewTestBot mentioned this pull request Jun 9, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelEischer MichaelEischer MichaelEischer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Crazycatz00 @jtru @ProactiveServices @MichaelEischer