From c076c9366934303fa8c5909bd13770e367dca72e Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Thu, 28 Nov 2024 15:14:06 +0300 Subject: [PATCH] permcheck: imp code, docs --- internal/permcheck/check_unix.go | 59 +++++++++++------------- internal/permcheck/check_windows.go | 12 +++-- internal/permcheck/migrate_unix.go | 64 +++++++++++++------------- internal/permcheck/migrate_windows.go | 19 +++++--- internal/permcheck/permcheck.go | 2 - internal/permcheck/security_windows.go | 21 +++++---- 6 files changed, 90 insertions(+), 87 deletions(-) diff --git a/internal/permcheck/check_unix.go b/internal/permcheck/check_unix.go index 3f0b82b8..0c467e50 100644 --- a/internal/permcheck/check_unix.go +++ b/internal/permcheck/check_unix.go @@ -25,71 +25,64 @@ func check( querylogDir string, confFilePath string, ) { - checkDir(ctx, l, workDir) + dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile) - checkFile(ctx, l, confFilePath) + checkDir(ctx, dirLoggger, workDir) + + checkFile(ctx, fileLogger, confFilePath) // TODO(a.garipov): Put all paths in one place and remove this duplication. - checkDir(ctx, l, dataDir) - checkDir(ctx, l, filepath.Join(dataDir, "filters")) - checkFile(ctx, l, filepath.Join(dataDir, "sessions.db")) - checkFile(ctx, l, filepath.Join(dataDir, "leases.json")) + 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, l, querylogDir) + checkDir(ctx, dirLoggger, querylogDir) } - checkFile(ctx, l, filepath.Join(querylogDir, "querylog.json")) - checkFile(ctx, l, filepath.Join(querylogDir, "querylog.json.1")) + checkFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json")) + checkFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json.1")) if dataDir != statsDir { - checkDir(ctx, l, statsDir) + checkDir(ctx, dirLoggger, statsDir) } - checkFile(ctx, l, filepath.Join(statsDir, "stats.db")) + checkFile(ctx, fileLogger, filepath.Join(statsDir, "stats.db")) } // checkDir checks the permissions of a single directory. The results are // logged at the appropriate level. func checkDir(ctx context.Context, l *slog.Logger, dirPath string) { - checkPath(ctx, l, dirPath, typeDir, aghos.DefaultPermDir) + checkPath(ctx, l, dirPath, aghos.DefaultPermDir) } // checkFile checks the permissions of a single file. The results are logged at // the appropriate level. func checkFile(ctx context.Context, l *slog.Logger, filePath string) { - checkPath(ctx, l, filePath, typeFile, aghos.DefaultPermFile) + 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, entPath, fileType string, want fs.FileMode) { - s, err := os.Stat(entPath) +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 { - logFunc := l.ErrorContext + lvl := slog.LevelError if errors.Is(err, os.ErrNotExist) { - logFunc = l.DebugContext + lvl = slog.LevelDebug } - logFunc( - ctx, - "checking permissions", - "type", fileType, - "path", entPath, - slogutil.KeyError, err, - ) + 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 { - l.WarnContext( - ctx, - "found unexpected permissions", - "type", fileType, - "path", entPath, - "got", fmt.Sprintf("%#o", perm), - "want", fmt.Sprintf("%#o", want), - ) + 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/check_windows.go b/internal/permcheck/check_windows.go index 262b1663..5e0c1ed4 100644 --- a/internal/permcheck/check_windows.go +++ b/internal/permcheck/check_windows.go @@ -12,6 +12,8 @@ import ( // check is the Windows-specific implementation of [Check]. func check(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { + l = l.With("type", typeDir, "path", workDir) + dacl, owner, err := getSecurityInfo(workDir) if err != nil { l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) @@ -20,12 +22,14 @@ func check(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { } if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) { - l.WarnContext(ctx, "working directory owner is not in administrators group") + l.WarnContext(ctx, "owner is not in administrators group") } - err = rangeACEs(dacl, func(hdr windows.ACE_HEADER, mask windows.ACCESS_MASK, sid *windows.SID) (cont bool) { - l.DebugContext(ctx, "checking entry", "sid", sid, "mask", mask) - + err = rangeACEs(dacl, func( + hdr windows.ACE_HEADER, + mask windows.ACCESS_MASK, + sid *windows.SID, + ) (cont bool) { warn := false switch { case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE: diff --git a/internal/permcheck/migrate_unix.go b/internal/permcheck/migrate_unix.go index 8b1b6993..c576aa16 100644 --- a/internal/permcheck/migrate_unix.go +++ b/internal/permcheck/migrate_unix.go @@ -45,63 +45,63 @@ func migrate( querylogDir string, confFilePath string, ) { - chmodDir(ctx, l, workDir) + dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile) - chmodFile(ctx, l, confFilePath) + chmodDir(ctx, dirLoggger, workDir) + + chmodFile(ctx, fileLogger, confFilePath) // TODO(a.garipov): Put all paths in one place and remove this duplication. - chmodDir(ctx, l, dataDir) - chmodDir(ctx, l, filepath.Join(dataDir, "filters")) - chmodFile(ctx, l, filepath.Join(dataDir, "sessions.db")) - chmodFile(ctx, l, filepath.Join(dataDir, "leases.json")) + 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, l, querylogDir) + chmodDir(ctx, dirLoggger, querylogDir) } - chmodFile(ctx, l, filepath.Join(querylogDir, "querylog.json")) - chmodFile(ctx, l, filepath.Join(querylogDir, "querylog.json.1")) + chmodFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json")) + chmodFile(ctx, fileLogger, filepath.Join(querylogDir, "querylog.json.1")) if dataDir != statsDir { - chmodDir(ctx, l, statsDir) + chmodDir(ctx, dirLoggger, statsDir) } - chmodFile(ctx, l, filepath.Join(statsDir, "stats.db")) + chmodFile(ctx, fileLogger, filepath.Join(statsDir, "stats.db")) } // chmodDir changes the permissions of a single directory. The results are // logged at the appropriate level. func chmodDir(ctx context.Context, l *slog.Logger, dirPath string) { - chmodPath(ctx, l, dirPath, typeDir, aghos.DefaultPermDir) + chmodPath(ctx, l, dirPath, aghos.DefaultPermDir) } // chmodFile changes the permissions of a single file. The results are logged // at the appropriate level. func chmodFile(ctx context.Context, l *slog.Logger, filePath string) { - chmodPath(ctx, l, filePath, typeFile, aghos.DefaultPermFile) + 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, entPath, fileType string, fm fs.FileMode) { - switch err := os.Chmod(entPath, fm); { +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: - l.InfoContext(ctx, "changed permissions", "type", fileType, "path", entPath) + lvl = slog.LevelInfo + msg = "changed permissions" case errors.Is(err, os.ErrNotExist): - l.DebugContext( - ctx, - "changing permissions", - "type", fileType, - "path", entPath, - slogutil.KeyError, err, - ) + lvl = slog.LevelDebug + msg = "checking permissions" + args = append(args, slogutil.KeyError, err) default: - l.ErrorContext( - ctx, - "can not change permissions; this can leave your system vulnerable, see "+ - "https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns", - "type", fileType, - "path", entPath, - "target_perm", fmt.Sprintf("%#o", fm), - slogutil.KeyError, err, - ) + lvl = slog.LevelError + msg = "can not 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 5d8a28b0..038ad617 100644 --- a/internal/permcheck/migrate_windows.go +++ b/internal/permcheck/migrate_windows.go @@ -12,6 +12,8 @@ import ( // needsMigration is the Windows-specific implementation of [NeedsMigration]. func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok bool) { + l = l.With("type", typeDir, "path", workDir) + dacl, owner, err := getSecurityInfo(workDir) if err != nil { l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) @@ -54,9 +56,11 @@ func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok // migrate is the Windows-specific implementation of [Migrate]. func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { + dirLogger := l.With("type", typeDir, "path", workDir) + dacl, owner, err := getSecurityInfo(workDir) if err != nil { - l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) + dirLogger.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) return } @@ -65,9 +69,10 @@ func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { var admins *windows.SID admins, err = windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid) if err != nil { + // This log message is not related to the directory. l.ErrorContext(ctx, "creating administrators sid", slogutil.KeyError, err) } else { - l.InfoContext(ctx, "migrating working directory owner", "sid", admins) + dirLogger.InfoContext(ctx, "migrating owner", "sid", admins) owner = admins } } @@ -82,26 +87,26 @@ func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { switch { case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE: // Add non-allowed access control entries as is. - l.InfoContext(ctx, "migrating deny control entry", "sid", sid) + dirLogger.InfoContext(ctx, "migrating deny control entry", "sid", sid) accessEntries = append(accessEntries, newDenyExplicitAccess(sid, mask)) case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid): // Skip non-administrator ACEs. - l.InfoContext(ctx, "removing access control entry", "sid", sid) + dirLogger.InfoContext(ctx, "removing access control entry", "sid", sid) default: - l.InfoContext(ctx, "migrating access control entry", "sid", sid, "mask", mask) + dirLogger.InfoContext(ctx, "migrating access control entry", "sid", sid, "mask", mask) accessEntries = append(accessEntries, newFullExplicitAccess(sid)) } return true }) if err != nil { - l.ErrorContext(ctx, "ranging trough access control entries", slogutil.KeyError, err) + dirLogger.ErrorContext(ctx, "filtering access control entries", slogutil.KeyError, err) return } err = setSecurityInfo(workDir, owner, accessEntries) if err != nil { - l.ErrorContext(ctx, "setting security info", slogutil.KeyError, err) + dirLogger.ErrorContext(ctx, "setting security info", slogutil.KeyError, err) } } diff --git a/internal/permcheck/permcheck.go b/internal/permcheck/permcheck.go index ef8850a8..0f19a09a 100644 --- a/internal/permcheck/permcheck.go +++ b/internal/permcheck/permcheck.go @@ -8,8 +8,6 @@ import ( ) // File type constants for logging. -// -// TODO(e.burkov): !! Use in Windows logging. const ( typeDir = "directory" typeFile = "file" diff --git a/internal/permcheck/security_windows.go b/internal/permcheck/security_windows.go index a781f9fe..713c7418 100644 --- a/internal/permcheck/security_windows.go +++ b/internal/permcheck/security_windows.go @@ -16,6 +16,8 @@ const securityInfo windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMA windows.PROTECTED_DACL_SECURITY_INFORMATION | windows.UNPROTECTED_DACL_SECURITY_INFORMATION +// objectType is the type of the object for directories in context of security +// API. const objectType = windows.SE_FILE_OBJECT // fileDeleteChildRight is the mask bit for the right to delete a child object. @@ -41,8 +43,8 @@ const fullControlMask windows.ACCESS_MASK = windows.FILE_LIST_DIRECTORY | windows.SYNCHRONIZE // aceFunc is a function that handles access control entries in the -// discretionary access control list. It should return true to continue ranging -// or false to stop. +// discretionary access control list. It should return true to continue +// iterating over the entries, or false to stop. type aceFunc = func( hdr windows.ACE_HEADER, mask windows.ACCESS_MASK, @@ -50,7 +52,7 @@ type aceFunc = func( ) (cont bool) // rangeACEs ranges over the access control entries in the discretionary access -// control list of the specified security descriptor. +// control list of the specified security descriptor and calls f for each one. func rangeACEs(dacl *windows.ACL, f aceFunc) (err error) { var errs []error for i := range uint32(dacl.AceCount) { @@ -119,15 +121,14 @@ func getSecurityInfo(fname string) (dacl *windows.ACL, owner *windows.SID, err e // newFullExplicitAccess creates a new explicit access entry with full control // permissions. -func newFullExplicitAccess(sid *windows.SID) (expAcc windows.EXPLICIT_ACCESS) { - // TODO(e.burkov): !! lookup account type +func newFullExplicitAccess(sid *windows.SID) (accEnt windows.EXPLICIT_ACCESS) { return windows.EXPLICIT_ACCESS{ AccessPermissions: fullControlMask, AccessMode: windows.GRANT_ACCESS, Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, Trustee: windows.TRUSTEE{ TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeType: windows.TRUSTEE_IS_UNKNOWN, TrusteeValue: windows.TrusteeValueFromSID(sid), }, } @@ -135,15 +136,17 @@ func newFullExplicitAccess(sid *windows.SID) (expAcc windows.EXPLICIT_ACCESS) { // newDenyExplicitAccess creates a new explicit access entry with specified deny // permissions. -func newDenyExplicitAccess(sid *windows.SID, mask windows.ACCESS_MASK) (expAcc windows.EXPLICIT_ACCESS) { - // TODO(e.burkov): !! lookup account type +func newDenyExplicitAccess( + sid *windows.SID, + mask windows.ACCESS_MASK, +) (accEnt windows.EXPLICIT_ACCESS) { return windows.EXPLICIT_ACCESS{ AccessPermissions: mask, AccessMode: windows.DENY_ACCESS, Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, Trustee: windows.TRUSTEE{ TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeType: windows.TRUSTEE_IS_UNKNOWN, TrusteeValue: windows.TrusteeValueFromSID(sid), }, }