all: imp code, docs

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].
 

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)

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:

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)
 	}

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) {
+	owner, err = adminsIfNot(owner)
-		var admins *windows.SID
+	switch {
-		admins, err = windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid)
+	case err != nil:
-		if err != nil {
+		l.ErrorContext(ctx, "creating administrators sid", slogutil.KeyError, err)
-			// This log message is not related to the directory.
+	case owner == nil:
-			l.ErrorContext(ctx, "creating administrators sid", slogutil.KeyError, err)
+		l.DebugContext(ctx, "owner is already an administrator")
-		} else {
+	default:
-			dirLogger.InfoContext(ctx, "migrating owner", "sid", admins)
+		l.InfoContext(ctx, "migrating owner", "sid", owner)
-			owner = admins
-		}
 	}
 
 	// 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.
+			// Add non-allowed access control entries as is, since they specify
-			dirLogger.InfoContext(ctx, "migrating deny control entry", "sid", sid)
+			// 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.
+			// Remove non-administrator ACEs, since such accounts should not
-			dirLogger.InfoContext(ctx, "removing access control entry", "sid", sid)
+			// 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)
+			// Administrators should have full control.  Don't add a new entry
-			accessEntries = append(accessEntries, newFullExplicitAccess(sid))
+			// 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)
+}

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.
+// desiredSecInfo defines the parts of a security descriptor to retrieve.
-const securityInfo windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMATION |
+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 {
+	var secInfo windows.SECURITY_INFORMATION
-		ents = []windows.EXPLICIT_ACCESS{
+
-			newFullExplicitAccess(owner),
+	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)
 	}