From 7dfbeda179d0ddb81db54fa4e0dcff189b400215 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Fri, 29 Nov 2024 14:28:17 +0300 Subject: [PATCH] permcheck: imp code --- internal/permcheck/check_unix.go | 57 ++---------- internal/permcheck/migrate_unix.go | 53 ++--------- internal/permcheck/migrate_windows.go | 15 ++-- internal/permcheck/security_unix.go | 121 ++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 104 deletions(-) create mode 100644 internal/permcheck/security_unix.go diff --git a/internal/permcheck/check_unix.go b/internal/permcheck/check_unix.go index 0c467e50..7e0b9716 100644 --- a/internal/permcheck/check_unix.go +++ b/internal/permcheck/check_unix.go @@ -4,15 +4,9 @@ package permcheck import ( "context" - "fmt" - "io/fs" "log/slog" - "os" - "path/filepath" "github.com/AdguardTeam/AdGuardHome/internal/aghos" - "github.com/AdguardTeam/golibs/errors" - "github.com/AdguardTeam/golibs/logutil/slogutil" ) // check is the Unix-specific implementation of [Check]. @@ -27,26 +21,13 @@ func check( ) { dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile) - checkDir(ctx, dirLoggger, workDir) - - checkFile(ctx, fileLogger, confFilePath) - - // TODO(a.garipov): Put all paths in one place and remove this duplication. - checkDir(ctx, dirLoggger, dataDir) - checkDir(ctx, dirLoggger, filepath.Join(dataDir, "filters")) - checkFile(ctx, fileLogger, filepath.Join(dataDir, "sessions.db")) - checkFile(ctx, fileLogger, filepath.Join(dataDir, "leases.json")) - - if dataDir != querylogDir { - checkDir(ctx, dirLoggger, querylogDir) + for _, ent := range entities(workDir, dataDir, statsDir, querylogDir, confFilePath) { + if ent.Value { + checkDir(ctx, dirLoggger, ent.Key) + } else { + checkFile(ctx, fileLogger, ent.Key) + } } - checkFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json")) - checkFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json.1")) - - if dataDir != statsDir { - checkDir(ctx, dirLoggger, statsDir) - } - checkFile(ctx, fileLogger, filepath.Join(statsDir, "stats.db")) } // checkDir checks the permissions of a single directory. The results are @@ -60,29 +41,3 @@ func checkDir(ctx context.Context, l *slog.Logger, dirPath string) { func checkFile(ctx context.Context, l *slog.Logger, filePath string) { checkPath(ctx, l, filePath, aghos.DefaultPermFile) } - -// checkPath checks the permissions of a single filesystem entity. The results -// are logged at the appropriate level. -func checkPath(ctx context.Context, l *slog.Logger, fpath string, want fs.FileMode) { - l = l.With("path", fpath) - s, err := os.Stat(fpath) - if err != nil { - lvl := slog.LevelError - if errors.Is(err, os.ErrNotExist) { - lvl = slog.LevelDebug - } - - l.Log(ctx, lvl, "checking permissions", slogutil.KeyError, err) - - return - } - - // TODO(a.garipov): Add a more fine-grained check and result reporting. - perm := s.Mode().Perm() - if perm == want { - return - } - - permOct, wantOct := fmt.Sprintf("%#o", perm), fmt.Sprintf("%#o", want) - l.WarnContext(ctx, "found unexpected permissions", "perm", permOct, "want", wantOct) -} diff --git a/internal/permcheck/migrate_unix.go b/internal/permcheck/migrate_unix.go index 2c60ce3e..95e12ee9 100644 --- a/internal/permcheck/migrate_unix.go +++ b/internal/permcheck/migrate_unix.go @@ -4,11 +4,8 @@ package permcheck import ( "context" - "fmt" - "io/fs" "log/slog" "os" - "path/filepath" "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" @@ -47,26 +44,13 @@ func migrate( ) { dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile) - chmodDir(ctx, dirLoggger, workDir) - - chmodFile(ctx, fileLogger, confFilePath) - - // TODO(a.garipov): Put all paths in one place and remove this duplication. - chmodDir(ctx, dirLoggger, dataDir) - chmodDir(ctx, dirLoggger, filepath.Join(dataDir, "filters")) - chmodFile(ctx, fileLogger, filepath.Join(dataDir, "sessions.db")) - chmodFile(ctx, fileLogger, filepath.Join(dataDir, "leases.json")) - - if dataDir != querylogDir { - chmodDir(ctx, dirLoggger, querylogDir) + for _, ent := range entities(workDir, dataDir, statsDir, querylogDir, confFilePath) { + if ent.Value { + chmodDir(ctx, dirLoggger, ent.Key) + } else { + chmodFile(ctx, fileLogger, ent.Key) + } } - chmodFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json")) - chmodFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json.1")) - - if dataDir != statsDir { - chmodDir(ctx, dirLoggger, statsDir) - } - chmodFile(ctx, fileLogger, filepath.Join(statsDir, "stats.db")) } // chmodDir changes the permissions of a single directory. The results are @@ -80,28 +64,3 @@ func chmodDir(ctx context.Context, l *slog.Logger, dirPath string) { func chmodFile(ctx context.Context, l *slog.Logger, filePath string) { chmodPath(ctx, l, filePath, aghos.DefaultPermFile) } - -// chmodPath changes the permissions of a single filesystem entity. The results -// are logged at the appropriate level. -func chmodPath(ctx context.Context, l *slog.Logger, fpath string, fm fs.FileMode) { - var lvl slog.Level - var msg string - args := []any{"path", fpath} - - switch err := os.Chmod(fpath, fm); { - case err == nil: - lvl = slog.LevelInfo - msg = "changed permissions" - case errors.Is(err, os.ErrNotExist): - lvl = slog.LevelDebug - msg = "checking permissions" - args = append(args, slogutil.KeyError, err) - default: - lvl = slog.LevelError - msg = "cannot change permissions; this can leave your system vulnerable, see " + - "https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns" - args = append(args, "target_perm", fmt.Sprintf("%#o", fm), slogutil.KeyError, err) - } - - l.Log(ctx, lvl, msg, args...) -} diff --git a/internal/permcheck/migrate_windows.go b/internal/permcheck/migrate_windows.go index f04a2206..9eb6e4e9 100644 --- a/internal/permcheck/migrate_windows.go +++ b/internal/permcheck/migrate_windows.go @@ -57,7 +57,10 @@ func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok // migrate is the Windows-specific implementation of [Migrate]. // -// It +// It sets the owner to administrators and adds a full control access control +// entry for the account. It also removes all non-administrator access control +// entries, and keeps deny access control entries. For any created or modified +// entry it sets the propagation flags to be inherited by child objects. func migrate(ctx context.Context, logger *slog.Logger, workDir, _, _, _, _ string) { l := logger.With("type", typeDir, "path", workDir) @@ -80,7 +83,7 @@ func migrate(ctx context.Context, logger *slog.Logger, workDir, _, _, _, _ strin // TODO(e.burkov): Check for duplicates? var accessEntries []windows.EXPLICIT_ACCESS - var useACL bool + var setACL bool // Iterate over the access control entries in DACL to determine if its // migration is needed. err = rangeACEs(dacl, func( @@ -94,18 +97,18 @@ func migrate(ctx context.Context, logger *slog.Logger, workDir, _, _, _, _ strin // the access restrictions, which shouldn't be lost. l.InfoContext(ctx, "migrating deny access control entry", "sid", sid) accessEntries = append(accessEntries, newDenyExplicitAccess(sid, mask)) - useACL = true + setACL = true case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid): // Remove non-administrator ACEs, since such accounts should not // have any access rights. l.InfoContext(ctx, "removing access control entry", "sid", sid) - useACL = true + setACL = true default: // Administrators should have full control. Don't add a new entry // here since it will be added later in case there are other // required entries. l.InfoContext(ctx, "migrating access control entry", "sid", sid, "mask", mask) - useACL = useACL || mask&fullControlMask != fullControlMask + setACL = setACL || mask&fullControlMask != fullControlMask } return true @@ -116,7 +119,7 @@ func migrate(ctx context.Context, logger *slog.Logger, workDir, _, _, _, _ strin return } - if useACL { + if setACL { accessEntries = append(accessEntries, newFullExplicitAccess(owner)) } diff --git a/internal/permcheck/security_unix.go b/internal/permcheck/security_unix.go new file mode 100644 index 00000000..437feab9 --- /dev/null +++ b/internal/permcheck/security_unix.go @@ -0,0 +1,121 @@ +//go:build unix + +package permcheck + +import ( + "context" + "fmt" + "io/fs" + "log/slog" + "os" + "path/filepath" + + "github.com/AdguardTeam/golibs/container" + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/logutil/slogutil" +) + +// entity is a filesystem entity with a path and a flag indicating whether it is +// a directory. +type entity = container.KeyValue[string, bool] + +// entities returns a list of filesystem entities that need to be ranged over. +func entities(workDir, dataDir, statsDir, querylogDir, confFilePath string) (ents []entity) { + ents = container.KeyValues[string, bool]{{ + Key: workDir, + Value: true, + }, { + Key: confFilePath, + Value: false, + }, { + Key: dataDir, + Value: true, + }, { + Key: filepath.Join(dataDir, "filters"), + Value: true, + }, { + Key: filepath.Join(dataDir, "sessions.db"), + Value: false, + }, { + Key: filepath.Join(dataDir, "leases.json"), + Value: false, + }} + + if dataDir != querylogDir { + ents = append(ents, entity{ + Key: querylogDir, + Value: true, + }) + } + ents = append(ents, []entity{{ + Key: filepath.Join(querylogDir, "querylog.json"), + Value: false, + }, { + Key: filepath.Join(querylogDir, "querylog.json.1"), + Value: false, + }}...) + + if dataDir != statsDir { + ents = append(ents, entity{ + Key: statsDir, + Value: true, + }) + } + ents = append(ents, entity{ + Key: filepath.Join(statsDir, "stats.db"), + }) + + return ents +} + +// checkPath checks the permissions of a single filesystem entity. The results +// are logged at the appropriate level. +func checkPath(ctx context.Context, l *slog.Logger, entPath string, want fs.FileMode) { + l = l.With("path", entPath) + + s, err := os.Stat(entPath) + if err != nil { + lvl := slog.LevelError + if errors.Is(err, os.ErrNotExist) { + lvl = slog.LevelDebug + } + + l.Log(ctx, lvl, "checking permissions", slogutil.KeyError, err) + + return + } + + // TODO(a.garipov): Add a more fine-grained check and result reporting. + perm := s.Mode().Perm() + if perm == want { + return + } + + permOct, wantOct := fmt.Sprintf("%#o", perm), fmt.Sprintf("%#o", want) + l.WarnContext(ctx, "found unexpected permissions", "perm", permOct, "want", wantOct) +} + +// chmodPath changes the permissions of a single filesystem entity. The results +// are logged at the appropriate level. +func chmodPath(ctx context.Context, l *slog.Logger, entPath string, fm fs.FileMode) { + var lvl slog.Level + var msg string + args := []any{"path", entPath} + + switch err := os.Chmod(entPath, fm); { + case err == nil: + lvl = slog.LevelInfo + msg = "changed permissions" + case errors.Is(err, os.ErrNotExist): + lvl = slog.LevelDebug + msg = "checking permissions" + args = append(args, slogutil.KeyError, err) + default: + lvl = slog.LevelError + msg = "cannot change permissions; this can leave your system vulnerable, see " + + "https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns" + args = append(args, "target_perm", fmt.Sprintf("%#o", fm), slogutil.KeyError, err) + } + + l.Log(ctx, lvl, msg, args...) +}