fix(symlinkguard): report and remove FIFOs, sockets and devices too (#179) #190
@@ -4,6 +4,7 @@ import (
|
|||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"strings"
|
"strings"
|
||||||
|
"syscall"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
@@ -63,3 +64,13 @@ func TestStartupSymlinkCheckRefusesAnUnlistableBrainRoot(t *testing.T) {
|
|||||||
t.Cleanup(func() { _ = os.Chmod(brain, 0o755) })
|
t.Cleanup(func() { _ = os.Chmod(brain, 0o755) })
|
||||||
assert.Error(t, startupSymlinkCheck(brain))
|
assert.Error(t, startupSymlinkCheck(brain))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// hyperguild#179: a FIFO in the brain is refused at startup with a clear
|
||||||
|
// message, instead of hanging or exiting later in a reader.
|
||||||
|
func TestStartupSymlinkCheckRefusesABrainWithAFIFO(t *testing.T) {
|
||||||
|
brain := t.TempDir()
|
||||||
|
require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "classification.yaml"), 0o644))
|
||||||
|
err := startupSymlinkCheck(brain)
|
||||||
|
require.Error(t, err)
|
||||||
|
assert.Contains(t, err.Error(), "classification.yaml")
|
||||||
|
}
|
||||||
|
|||||||
@@ -0,0 +1,104 @@
|
|||||||
|
package symlinkguard
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"net"
|
||||||
|
"os"
|
||||||
|
"path/filepath"
|
||||||
|
"syscall"
|
||||||
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
// hyperguild#179: the tripwire looked only for symlinks, so a FIFO (or
|
||||||
|
// socket) in the brain passed it. A FIFO at classification.yaml then made
|
||||||
|
// startup exit; at a note path it could hang readers that skip the handle
|
||||||
|
// check.
|
||||||
|
func TestScanReportsFIFOsAndSocketsAsSpecialFiles(t *testing.T) {
|
||||||
|
brain := t.TempDir()
|
||||||
|
require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w"), 0o755))
|
||||||
|
require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "classification.yaml"), 0o644))
|
||||||
|
require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "wiki", "w", "f.md"), 0o644))
|
||||||
|
l, err := net.Listen("unix", filepath.Join(brain, "s.sock"))
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer func() { _ = l.Close() }()
|
||||||
|
require.NoError(t, os.WriteFile(filepath.Join(brain, "wiki", "w", "n.md"), []byte("n"), 0o644))
|
||||||
|
|
||||||
|
r, err := Scan(brain)
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, []string{"classification.yaml", "s.sock", "wiki/w/f.md"}, r.Special)
|
||||||
|
assert.False(t, r.Clean())
|
||||||
|
assert.Contains(t, r.String(), "special file")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Watch removes a FIFO as it removes symlinks, and leaves a regular file
|
||||||
|
// (which Scan never reports) alone.
|
||||||
|
func TestWatchRemovesAFIFOAndLeavesAnUnreportedRegularFile(t *testing.T) {
|
||||||
|
brain := t.TempDir()
|
||||||
|
fifo := filepath.Join(brain, "f.md")
|
||||||
|
require.NoError(t, syscall.Mkfifo(fifo, 0o644))
|
||||||
|
regular := filepath.Join(brain, "n.md")
|
||||||
|
require.NoError(t, os.WriteFile(regular, []byte("n"), 0o644))
|
||||||
|
n := &recNotifier{}
|
||||||
|
ctx, cancel := context.WithCancel(context.Background())
|
||||||
|
defer cancel()
|
||||||
|
go Watch(ctx, brain, 10*time.Millisecond, n)
|
||||||
|
require.Eventually(t, func() bool {
|
||||||
|
_, err := os.Lstat(fifo)
|
||||||
|
return os.IsNotExist(err)
|
||||||
|
}, 2*time.Second, 10*time.Millisecond, "the FIFO is removed")
|
||||||
|
require.Eventually(t, func() bool { return n.count() >= 1 }, 2*time.Second, 10*time.Millisecond)
|
||||||
|
b, err := os.ReadFile(regular)
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, "n", string(b))
|
||||||
|
n.mu.Lock()
|
||||||
|
defer n.mu.Unlock()
|
||||||
|
assert.Contains(t, n.msgs[0], "f.md")
|
||||||
|
}
|
||||||
|
|
||||||
|
// removeSpecial re-checks the type: a path that is a regular file or a
|
||||||
|
// directory at the re-check is not removed. (A swap between the re-check and
|
||||||
|
// the unlink can still remove one; Linux has no conditional unlink. The same
|
||||||
|
// window exists in removeSymlinks.)
|
||||||
|
func TestRemoveSpecialRefusesRegularFilesAndDirectories(t *testing.T) {
|
||||||
|
brain := t.TempDir()
|
||||||
|
require.NoError(t, os.WriteFile(filepath.Join(brain, "n.md"), []byte("n"), 0o644))
|
||||||
|
require.NoError(t, os.MkdirAll(filepath.Join(brain, "d"), 0o755))
|
||||||
|
require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "f.md"), 0o644))
|
||||||
|
removed, failed := removeSpecial(brain, []string{"n.md", "d", "f.md"})
|
||||||
|
assert.Equal(t, []string{"f.md"}, removed)
|
||||||
|
assert.Equal(t, []string{"n.md", "d"}, failed)
|
||||||
|
_, err := os.Stat(filepath.Join(brain, "n.md"))
|
||||||
|
assert.NoError(t, err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// A special file Watch cannot remove (its directory is read-only) stays
|
||||||
|
// reported: the persistent-finding alert names it.
|
||||||
|
func TestWatchReportsASpecialFileItCannotRemove(t *testing.T) {
|
||||||
|
if os.Geteuid() == 0 {
|
||||||
|
t.Skip("root ignores directory permissions")
|
||||||
|
}
|
||||||
|
brain := t.TempDir()
|
||||||
|
ro := filepath.Join(brain, "ro")
|
||||||
|
require.NoError(t, os.MkdirAll(ro, 0o755))
|
||||||
|
require.NoError(t, syscall.Mkfifo(filepath.Join(ro, "f.md"), 0o644))
|
||||||
|
require.NoError(t, os.Chmod(ro, 0o555))
|
||||||
|
t.Cleanup(func() { _ = os.Chmod(ro, 0o755) })
|
||||||
|
n := &recNotifier{}
|
||||||
|
ctx, cancel := context.WithCancel(context.Background())
|
||||||
|
defer cancel()
|
||||||
|
go Watch(ctx, brain, 10*time.Millisecond, n)
|
||||||
|
require.Eventually(t, func() bool { return n.count() >= 1 }, 2*time.Second, 10*time.Millisecond)
|
||||||
|
_, err := os.Lstat(filepath.Join(ro, "f.md"))
|
||||||
|
require.NoError(t, err, "the FIFO could not be removed")
|
||||||
|
n.mu.Lock()
|
||||||
|
defer n.mu.Unlock()
|
||||||
|
assert.Contains(t, n.msgs[0], "ro/f.md")
|
||||||
|
assert.Contains(t, n.msgs[0], "special file")
|
||||||
|
for _, m := range n.msgs {
|
||||||
|
assert.NotContains(t, m, "removed", "a file it could not remove was reported as removed")
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -46,10 +46,16 @@ type Notifier interface {
|
|||||||
type Result struct {
|
type Result struct {
|
||||||
Symlinks []string
|
Symlinks []string
|
||||||
Unreadable []string
|
Unreadable []string
|
||||||
|
// Special holds FIFOs, sockets and devices (hyperguild#179): a reader
|
||||||
|
// that opens one by path can hang, and a FIFO at classification.yaml
|
||||||
|
// stops startup.
|
||||||
|
Special []string
|
||||||
}
|
}
|
||||||
|
|
||||||
// Clean reports whether the scan found nothing to act on.
|
// Clean reports whether the scan found nothing to act on.
|
||||||
func (r Result) Clean() bool { return len(r.Symlinks) == 0 && len(r.Unreadable) == 0 }
|
func (r Result) Clean() bool {
|
||||||
|
return len(r.Symlinks) == 0 && len(r.Unreadable) == 0 && len(r.Special) == 0
|
||||||
|
}
|
||||||
|
|
||||||
// String describes the findings for a log line or alert. Paths are quoted, so
|
// String describes the findings for a log line or alert. Paths are quoted, so
|
||||||
// a name containing a newline cannot blur two findings together.
|
// a name containing a newline cannot blur two findings together.
|
||||||
@@ -59,6 +65,9 @@ func (r Result) String() string {
|
|||||||
if len(r.Symlinks) > 0 {
|
if len(r.Symlinks) > 0 {
|
||||||
parts = append(parts, fmt.Sprintf("%d symlink(s): %s", len(r.Symlinks), q(r.Symlinks)))
|
parts = append(parts, fmt.Sprintf("%d symlink(s): %s", len(r.Symlinks), q(r.Symlinks)))
|
||||||
}
|
}
|
||||||
|
if len(r.Special) > 0 {
|
||||||
|
parts = append(parts, fmt.Sprintf("%d special file(s) (FIFO, socket or device; delete them): %s", len(r.Special), q(r.Special)))
|
||||||
|
}
|
||||||
if len(r.Unreadable) > 0 {
|
if len(r.Unreadable) > 0 {
|
||||||
parts = append(parts, fmt.Sprintf("%d unreadable dir(s): %s", len(r.Unreadable), q(r.Unreadable)))
|
parts = append(parts, fmt.Sprintf("%d unreadable dir(s): %s", len(r.Unreadable), q(r.Unreadable)))
|
||||||
}
|
}
|
||||||
@@ -113,12 +122,16 @@ func Scan(brainDir string) (Result, error) {
|
|||||||
if d.IsDir() && p == rootGit {
|
if d.IsDir() && p == rootGit {
|
||||||
return filepath.SkipDir
|
return filepath.SkipDir
|
||||||
}
|
}
|
||||||
if d.Type()&os.ModeSymlink != 0 {
|
switch {
|
||||||
|
case d.Type()&os.ModeSymlink != 0:
|
||||||
r.Symlinks = append(r.Symlinks, rel)
|
r.Symlinks = append(r.Symlinks, rel)
|
||||||
|
case !d.IsDir() && !d.Type().IsRegular():
|
||||||
|
r.Special = append(r.Special, rel)
|
||||||
}
|
}
|
||||||
return nil
|
return nil
|
||||||
})
|
})
|
||||||
sort.Strings(r.Symlinks)
|
sort.Strings(r.Symlinks)
|
||||||
|
sort.Strings(r.Special)
|
||||||
sort.Strings(r.Unreadable)
|
sort.Strings(r.Unreadable)
|
||||||
if walkErr != nil {
|
if walkErr != nil {
|
||||||
// Only the root's own error reaches here (deeper errors are recorded
|
// Only the root's own error reaches here (deeper errors are recorded
|
||||||
@@ -182,14 +195,20 @@ func Watch(ctx context.Context, brainDir string, interval time.Duration, n Notif
|
|||||||
q.push(fmt.Sprintf("brain symlink tripwire (hyperguild#157): removed %d symlink(s) from the brain dir: %s",
|
q.push(fmt.Sprintf("brain symlink tripwire (hyperguild#157): removed %d symlink(s) from the brain dir: %s",
|
||||||
len(removed), Result{}.quoted(removed)))
|
len(removed), Result{}.quoted(removed)))
|
||||||
}
|
}
|
||||||
key = Result{Symlinks: failed, Unreadable: r.Unreadable}.String()
|
sRemoved, sFailed := removeSpecial(brainDir, r.Special)
|
||||||
|
if len(sRemoved) > 0 {
|
||||||
|
slog.Error("brain symlink tripwire removed special files", "count", len(sRemoved))
|
||||||
|
q.push(fmt.Sprintf("brain symlink tripwire (hyperguild#179): removed %d special file(s) (FIFO, socket or device) from the brain dir: %s",
|
||||||
|
len(sRemoved), Result{}.quoted(sRemoved)))
|
||||||
|
}
|
||||||
|
key = Result{Symlinks: failed, Unreadable: r.Unreadable, Special: sFailed}.String()
|
||||||
}
|
}
|
||||||
if key != reported {
|
if key != reported {
|
||||||
reported = key
|
reported = key
|
||||||
if key != "" {
|
if key != "" {
|
||||||
slog.Error("brain symlink tripwire", "finding", key)
|
slog.Error("brain symlink tripwire", "finding", key)
|
||||||
q.push("brain symlink tripwire (hyperguild#157): " + key +
|
q.push("brain symlink tripwire (hyperguild#157, #179): " + key +
|
||||||
". Readers may follow symlinks out of the brain; fix them.")
|
". Symlinks can lead readers out of the brain and special files can hang them; remove them.")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -322,6 +341,40 @@ func permanent(err error) bool {
|
|||||||
return errors.As(err, &p) && p.Permanent()
|
return errors.As(err, &p) && p.Permanent()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// removeSpecial unlinks each brain-relative FIFO, socket or device through
|
||||||
|
// an os.Root on the brain dir, after re-checking that it is still one: a
|
||||||
|
// path that is a regular file, a directory or a symlink at the re-check is
|
||||||
|
// left alone and reported as failed. The re-check narrows, but cannot close,
|
||||||
|
// the window before the unlink (Linux has no conditional unlink); the same
|
||||||
|
// window exists in removeSymlinks, and only a local writer can use it.
|
||||||
|
func removeSpecial(brainDir string, rels []string) (removed, failed []string) {
|
||||||
|
if len(rels) == 0 {
|
||||||
|
return nil, nil
|
||||||
|
}
|
||||||
|
resolved, err := filepath.EvalSymlinks(brainDir)
|
||||||
|
if err != nil {
|
||||||
|
return nil, rels
|
||||||
|
}
|
||||||
|
root, err := os.OpenRoot(resolved)
|
||||||
|
if err != nil {
|
||||||
|
return nil, rels
|
||||||
|
}
|
||||||
|
defer func() { _ = root.Close() }()
|
||||||
|
for _, rel := range rels {
|
||||||
|
fi, err := root.Lstat(rel)
|
||||||
|
if err != nil || fi.IsDir() || fi.Mode().IsRegular() || fi.Mode()&os.ModeSymlink != 0 {
|
||||||
|
failed = append(failed, rel)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if err := root.Remove(rel); err != nil {
|
||||||
|
failed = append(failed, rel)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
removed = append(removed, rel)
|
||||||
|
}
|
||||||
|
return removed, failed
|
||||||
|
}
|
||||||
|
|
||||||
// removeSymlinks unlinks each brain-relative symlink through an os.Root on
|
// removeSymlinks unlinks each brain-relative symlink through an os.Root on
|
||||||
// the brain dir, after re-checking it is still a symlink. Removing a symlink
|
// the brain dir, after re-checking it is still a symlink. Removing a symlink
|
||||||
// never touches its target. It returns what was removed and what was not.
|
// never touches its target. It returns what was removed and what was not.
|
||||||
|
|||||||
Reference in New Issue
Block a user