Skip to content

fix(restore): keep setuid, setgid and sticky bits when replaying modes - #15

Open
codeAnqiang-ma wants to merge 1 commit into
edaywalid:mainfrom
codeAnqiang-ma:fix/mode-bits
Open

fix(restore): keep setuid, setgid and sticky bits when replaying modes#15
codeAnqiang-ma wants to merge 1 commit into
edaywalid:mainfrom
codeAnqiang-ma:fix/mode-bits

Conversation

@codeAnqiang-ma

Copy link
Copy Markdown

Fixes #14

The bug

The shim journals a mode as st_mode & 07777 (undo_shim.c:816 for chmod, undo_shim.c:1134 for rmdir), so the recorded value can carry setuid, setgid or sticky. Replay cast that straight to an os.FileMode:

// restore.go, OpRmdir undo
if err = os.MkdirAll(field(0), os.FileMode(mode)); err == nil {
	err = os.Chmod(field(0), os.FileMode(mode))
}

// restore.go, OpChmod
err = os.Chmod(field(0), os.FileMode(mode))

os.FileMode keeps the permission bits exactly where POSIX has them, which is why 3-digit modes have always worked, but it encodes setuid, setgid and sticky as flags of its own (ModeSetuid, ModeSetgid, ModeSticky) rather than at POSIX's bits 11/10/9. os.Chmod passes through Perm() plus those three flags, so on a cast value it never sees them: os.FileMode(0o4755).Perm() is 0o755 and os.FileMode(0o4755)&os.ModeSetuid is zero.

The result is a silent partial restore. Undoing a chmod on a setuid binary prints

mode /tmp/x/tool (4755 -> 755)
restored 1 change(s)

and leaves the file 0755. A sticky directory recorded as 1777 and recreated by undoing an rmdir comes back 0777. The preview promises the recorded mode and the summary counts the entry as restored, so nothing signals that the bits are gone — which matters, because these are exactly the bits whose loss is a security-relevant change rather than a cosmetic one.

The journal is intact; only the replay side drops them.

The fix

One converter from the journal's POSIX encoding to Go's, used at the three sites that apply a recorded mode (MkdirAll and Chmod when recreating a removed directory, and Chmod for chmod entries — the last covers both undo and redo, since they differ only in which field they read).

MkdirAll's mode is still subject to the process umask, so the following os.Chmod is what actually lands the special bits. That was already the shape of the code; it just had nothing to land.

Tests

Three tests in internal/restore:

  • TestModeFromOctalKeepsHighBits — table over the converter, asserting each of the three bits survives and Perm() is untouched.
  • TestRmdirUndoRestoresStickyDirectory — replays a real rmdir <path> 1777 entry and stats the result.
  • TestChmodUndoRestoresSetuid — replays a real chmod <path> 4755 755 entry and stats the result.

The two replay tests fail on main. Before:

$ go test ./internal/restore/ -run 'TestRmdirUndoRestoresStickyDirectory|TestChmodUndoRestoresSetuid' -v
=== RUN   TestRmdirUndoRestoresStickyDirectory
    restore_test.go:185: recreated directory lost the sticky bit: drwxrwxrwx
--- FAIL: TestRmdirUndoRestoresStickyDirectory (0.01s)
=== RUN   TestChmodUndoRestoresSetuid
    restore_test.go:212: restored file lost the setuid bit: -rwxr-xr-x
--- FAIL: TestChmodUndoRestoresSetuid (0.01s)
FAIL

Note that the permission assertions in both tests passed even then: 0777 and 0755 were correct, only the high bit was missing. That is the whole bug.

After:

$ gofmt -l .
$ go vet ./...
$ go test ./...
ok  	github.com/edaywalid/undo/cmd/undo	(cached)
ok  	github.com/edaywalid/undo/internal/journal	(cached)
ok  	github.com/edaywalid/undo/internal/restore	0.445s
ok  	github.com/edaywalid/undo/internal/session	(cached)

gofmt -l . and go vet ./... printed nothing.

This is also why the existing suite never caught it: every mode in the fixtures is three digits, 640 / 644 / 600 in the e2e cases and 755 in TestRmdirUndoRecreatesDirectory, and a plain cast is correct for all of those.

Not run locally: make test, i.e. test/e2e.sh and test/hook.sh. I am on macOS, where shim/undo_shim.c does not compile (O_LARGEFILE and other Linux/glibc-isms), so make cannot produce libundo.so and the shim-driven suites cannot start. This change is confined to the Go replay side and touches no shim code or shell hook; CI covers those suites on Linux.

If you would like an e2e case alongside these — a 4755 file through the real shim in test/e2e.sh — say the word and I will add one, though I would not be able to run it locally to confirm it before you do.


Found and fixed with AI assistance. I reproduced the bit loss locally, verified every cited line, and reviewed all conclusions before opening this.

The shim journals st_mode & 07777, so a chmod entry or a removed
directory can carry setuid, setgid or sticky. Replay cast that value
straight to os.FileMode, which keeps the permission bits where POSIX
has them but encodes those three as flags of its own, and os.Chmod only
honours the flags. A 4755 binary came back 0755 and a 1777 directory
came back 0777, while the preview printed the recorded mode and the
summary counted the entry as restored.

Translate the journal value into Go's encoding at the three sites that
apply a recorded mode.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setuid/setgid/sticky bits are silently dropped when replaying chmod and rmdir entries

1 participant