From 3a5b6aced948a2d09fdae823fc986266c9984b3d Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Thu, 28 Nov 2024 19:21:03 +0300 Subject: [PATCH] all: imp code, docs --- CHANGELOG.md | 3 ++ internal/aghrenameio/renameio_windows.go | 2 + internal/permcheck/check_windows.go | 7 +++ internal/permcheck/migrate_unix.go | 2 +- internal/permcheck/migrate_windows.go | 67 ++++++++++++++++-------- internal/permcheck/security_windows.go | 36 ++++++++----- 6 files changed, 83 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52053e9f..ee8aaebb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,9 @@ NOTE: Add new changes BELOW THIS COMMENT. ### Security +- The permission check and migration on Windows has been fixed to use the + Windows security model more accurately ([#7400]). + - Go version has been updated to prevent the possibility of exploiting the Go vulnerabilities fixed in [1.23.3][go-1.23.3]. diff --git a/internal/aghrenameio/renameio_windows.go b/internal/aghrenameio/renameio_windows.go index 8097caf4..0a677035 100644 --- a/internal/aghrenameio/renameio_windows.go +++ b/internal/aghrenameio/renameio_windows.go @@ -62,6 +62,8 @@ func newPendingFile(filePath string, mode fs.FileMode) (f PendingFile, err error return nil, fmt.Errorf("opening pending file: %w", err) } + // TODO(e.burkov): The [os.Chmod] implementation is useless on Windows, + // investigate if it can be removed. err = os.Chmod(file.Name(), mode) if err != nil { return nil, fmt.Errorf("preparing pending file: %w", err) diff --git a/internal/permcheck/check_windows.go b/internal/permcheck/check_windows.go index 5e0c1ed4..11b10215 100644 --- a/internal/permcheck/check_windows.go +++ b/internal/permcheck/check_windows.go @@ -11,6 +11,11 @@ import ( ) // check is the Windows-specific implementation of [Check]. +// +// Note, that it only checks the owner and the ACEs of the working directory. +// This is due to the assumption that the working directory ACEs are inherited +// by the underlying files and directories, since at least [migrate] sets this +// inheritance mode. func check(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { l = l.With("type", typeDir, "path", workDir) @@ -30,6 +35,8 @@ func check(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { mask windows.ACCESS_MASK, sid *windows.SID, ) (cont bool) { + l.DebugContext(ctx, "checking access control entry", "mask", mask, "sid", sid) + 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 c576aa16..2c60ce3e 100644 --- a/internal/permcheck/migrate_unix.go +++ b/internal/permcheck/migrate_unix.go @@ -98,7 +98,7 @@ func chmodPath(ctx context.Context, l *slog.Logger, fpath string, fm fs.FileMode args = append(args, slogutil.KeyError, err) default: lvl = slog.LevelError - msg = "can not change permissions; this can leave your system vulnerable, see " + + 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) } diff --git a/internal/permcheck/migrate_windows.go b/internal/permcheck/migrate_windows.go index 038ad617..f04a2206 100644 --- a/internal/permcheck/migrate_windows.go +++ b/internal/permcheck/migrate_windows.go @@ -33,6 +33,7 @@ func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok switch { case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE: // Skip non-allowed access control entries. + l.DebugContext(ctx, "skipping deny access control entry", "sid", sid) case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid): // Non-administrator access control entries should not have any // access rights. @@ -55,30 +56,33 @@ 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) +// +// It +func migrate(ctx context.Context, logger *slog.Logger, workDir, _, _, _, _ string) { + l := logger.With("type", typeDir, "path", workDir) dacl, owner, err := getSecurityInfo(workDir) if err != nil { - dirLogger.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) + l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) return } - if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) { - 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 { - dirLogger.InfoContext(ctx, "migrating owner", "sid", admins) - owner = admins - } + owner, err = adminsIfNot(owner) + switch { + case err != nil: + l.ErrorContext(ctx, "creating administrators sid", slogutil.KeyError, err) + case owner == nil: + l.DebugContext(ctx, "owner is already an administrator") + default: + l.InfoContext(ctx, "migrating owner", "sid", owner) } // TODO(e.burkov): Check for duplicates? var accessEntries []windows.EXPLICIT_ACCESS + var useACL bool + // Iterate over the access control entries in DACL to determine if its + // migration is needed. err = rangeACEs(dacl, func( hdr windows.ACE_HEADER, mask windows.ACCESS_MASK, @@ -86,27 +90,48 @@ func migrate(ctx context.Context, l *slog.Logger, workDir, _, _, _, _ string) { ) (cont bool) { switch { case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE: - // Add non-allowed access control entries as is. - dirLogger.InfoContext(ctx, "migrating deny control entry", "sid", sid) + // Add non-allowed access control entries as is, since they specify + // 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 case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid): - // Skip non-administrator ACEs. - dirLogger.InfoContext(ctx, "removing access control entry", "sid", sid) + // Remove non-administrator ACEs, since such accounts should not + // have any access rights. + l.InfoContext(ctx, "removing access control entry", "sid", sid) + useACL = true default: - dirLogger.InfoContext(ctx, "migrating access control entry", "sid", sid, "mask", mask) - accessEntries = append(accessEntries, newFullExplicitAccess(sid)) + // 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 } return true }) if err != nil { - dirLogger.ErrorContext(ctx, "filtering access control entries", slogutil.KeyError, err) + l.ErrorContext(ctx, "ranging through access control entries", slogutil.KeyError, err) return } + if useACL { + accessEntries = append(accessEntries, newFullExplicitAccess(owner)) + } + err = setSecurityInfo(workDir, owner, accessEntries) if err != nil { - dirLogger.ErrorContext(ctx, "setting security info", slogutil.KeyError, err) + l.ErrorContext(ctx, "setting security info", slogutil.KeyError, err) } } + +// adminsIfNot returns the administrators SID if sid is not a +// [windows.WinBuiltinAdministratorsSid] yet, or nil if it is. +func adminsIfNot(sid *windows.SID) (admins *windows.SID, err error) { + if sid.IsWellKnown(windows.WinBuiltinAdministratorsSid) { + return nil, nil + } + + return windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid) +} diff --git a/internal/permcheck/security_windows.go b/internal/permcheck/security_windows.go index 713c7418..25334279 100644 --- a/internal/permcheck/security_windows.go +++ b/internal/permcheck/security_windows.go @@ -10,21 +10,21 @@ import ( "golang.org/x/sys/windows" ) -// securityInfo defines the parts of a security descriptor to retrieve and set. -const securityInfo windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMATION | +// desiredSecInfo defines the parts of a security descriptor to retrieve. +const desiredSecInfo windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMATION | windows.DACL_SECURITY_INFORMATION | 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 +const objectType windows.SE_OBJECT_TYPE = windows.SE_FILE_OBJECT // fileDeleteChildRight is the mask bit for the right to delete a child object. -// It seems to be missing from the windows package. +// It seems to be missing from the [windows] package. // // See https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/access-mask. -const fileDeleteChildRight = 0b1000000 +const fileDeleteChildRight windows.ACCESS_MASK = 0b0100_0000 // fullControlMask is the mask for full control access rights. const fullControlMask windows.ACCESS_MASK = windows.FILE_LIST_DIRECTORY | @@ -78,12 +78,24 @@ func rangeACEs(dacl *windows.ACL, f aceFunc) (err error) { } // setSecurityInfo sets the security information on the specified file, using -// ents to create a discretionary access control list. +// ents to create a discretionary access control list. Both owner and ents can +// be nil, in which case the corresponding information is not set. func setSecurityInfo(fname string, owner *windows.SID, ents []windows.EXPLICIT_ACCESS) (err error) { - if len(ents) == 0 { - ents = []windows.EXPLICIT_ACCESS{ - newFullExplicitAccess(owner), - } + var secInfo windows.SECURITY_INFORMATION + + if len(ents) > 0 { + // TODO(e.burkov): Investigate if this whole set is necessary. + secInfo |= windows.DACL_SECURITY_INFORMATION | + windows.PROTECTED_DACL_SECURITY_INFORMATION | + windows.UNPROTECTED_DACL_SECURITY_INFORMATION + } + + if owner != nil { + secInfo |= windows.OWNER_SECURITY_INFORMATION + } + + if secInfo == 0 { + return errors.Error("no security information to set") } acl, err := windows.ACLFromEntries(ents, nil) @@ -91,7 +103,7 @@ func setSecurityInfo(fname string, owner *windows.SID, ents []windows.EXPLICIT_A return fmt.Errorf("creating access control list: %w", err) } - err = windows.SetNamedSecurityInfo(fname, objectType, securityInfo, owner, nil, acl, nil) + err = windows.SetNamedSecurityInfo(fname, objectType, desiredSecInfo, owner, nil, acl, nil) if err != nil { return fmt.Errorf("setting security info: %w", err) } @@ -101,7 +113,7 @@ func setSecurityInfo(fname string, owner *windows.SID, ents []windows.EXPLICIT_A // getSecurityInfo retrieves the security information for the specified file. func getSecurityInfo(fname string) (dacl *windows.ACL, owner *windows.SID, err error) { - sd, err := windows.GetNamedSecurityInfo(fname, objectType, securityInfo) + sd, err := windows.GetNamedSecurityInfo(fname, objectType, desiredSecInfo) if err != nil { return nil, nil, fmt.Errorf("getting security descriptor: %w", err) }