all: fix more bugs

internal/aghos/permission_windows.go

@@ -9,7 +9,6 @@ import (
 	"path/filepath"
 	"unsafe"
 
-	"github.com/AdguardTeam/golibs/container"
 	"github.com/AdguardTeam/golibs/errors"
 	"golang.org/x/sys/windows"
 )
@@ -17,7 +16,7 @@ import (
 // fileInfo is a Windows implementation of [fs.FileInfo], that contains the
 // filemode converted from the security descriptor.
 type fileInfo struct {
-	// fs.FileInfo is embedded to provide the default implementations and info
+	// fs.FileInfo is embedded to provide the default implementations and data
 	// successfully retrieved by [os.Stat].
 	fs.FileInfo
 
@@ -33,38 +32,21 @@ func (fi *fileInfo) Mode() (mode fs.FileMode) { return fi.mode }
 
 // stat is a Windows implementation of [Stat].
 func stat(name string) (fi os.FileInfo, err error) {
-	fi, err = os.Stat(name)
+	absName, err := filepath.Abs(name)
 	if err != nil {
+		return nil, fmt.Errorf("computing absolute path: %w", err)
+	}
+
+	fi, err = os.Stat(absName)
+	if err != nil {
+		// Don't wrap the error, since it's informative enough as is.
 		return nil, err
 	}
 
-	const objectType windows.SE_OBJECT_TYPE = windows.SE_FILE_OBJECT
+	dacl, owner, group, err := retrieveDACL(absName)
-
-	secInfo := windows.SECURITY_INFORMATION(
-		windows.OWNER_SECURITY_INFORMATION |
-			windows.GROUP_SECURITY_INFORMATION |
-			windows.DACL_SECURITY_INFORMATION |
-			windows.PROTECTED_DACL_SECURITY_INFORMATION,
-	)
-
-	sd, err := windows.GetNamedSecurityInfo(fi.Name(), objectType, secInfo)
 	if err != nil {
-		return nil, fmt.Errorf("getting security descriptor: %w", err)
+		// Don't wrap the error, since it's informative enough as is.
-	}
+		return nil, err
-
-	dacl, _, err := sd.DACL()
-	if err != nil {
-		return nil, fmt.Errorf("getting discretionary access control list: %w", err)
-	}
-
-	owner, _, err := sd.Owner()
-	if err != nil {
-		return nil, fmt.Errorf("getting owner sid: %w", err)
-	}
-
-	group, _, err := sd.Group()
-	if err != nil {
-		return nil, fmt.Errorf("getting group sid: %w", err)
 	}
 
 	var ownerMask, groupMask, otherMask windows.ACCESS_MASK
@@ -86,18 +68,53 @@ func stat(name string) (fi os.FileInfo, err error) {
 		}
 	}
 
-	mode := masksToPerm(ownerMask, groupMask, otherMask) | (fi.Mode().Perm() & ^fs.ModePerm)
+	mode := fi.Mode()
+	perm := masksToPerm(ownerMask, groupMask, otherMask, mode.IsDir())
 
 	return &fileInfo{
 		FileInfo: fi,
-		mode:     mode,
+		// Use the file mode from the security descriptor, but use the
+		// calculated permission bits.
+		mode: perm | mode&^fs.FileMode(0o777),
 	}, nil
 }
 
+// retrieveDACL retrieves the discretionary access control list, owner, and
+// group from the security descriptor of the file with the specified absolute
+// name.
+func retrieveDACL(absName string) (dacl *windows.ACL, owner, group *windows.SID, err error) {
+	// desiredSecInfo defines the parts of a security descriptor to retrieve.
+	const desiredSecInfo windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMATION |
+		windows.GROUP_SECURITY_INFORMATION |
+		windows.DACL_SECURITY_INFORMATION |
+		windows.PROTECTED_DACL_SECURITY_INFORMATION |
+		windows.UNPROTECTED_DACL_SECURITY_INFORMATION
+
+	sd, err := windows.GetNamedSecurityInfo(absName, windows.SE_FILE_OBJECT, desiredSecInfo)
+	if err != nil {
+		return nil, nil, nil, fmt.Errorf("getting security descriptor: %w", err)
+	}
+
+	dacl, _, err = sd.DACL()
+	if err != nil {
+		return nil, nil, nil, fmt.Errorf("getting discretionary access control list: %w", err)
+	}
+
+	owner, _, err = sd.Owner()
+	if err != nil {
+		return nil, nil, nil, fmt.Errorf("getting owner sid: %w", err)
+	}
+
+	group, _, err = sd.Group()
+	if err != nil {
+		return nil, nil, nil, fmt.Errorf("getting group sid: %w", err)
+	}
+
+	return dacl, owner, group, nil
+}
+
 // chmod is a Windows implementation of [Chmod].
 func chmod(name string, perm fs.FileMode) (err error) {
-	const objectType windows.SE_OBJECT_TYPE = windows.SE_FILE_OBJECT
-
 	fi, err := os.Stat(name)
 	if err != nil {
 		return fmt.Errorf("getting file info: %w", err)
@@ -106,7 +123,10 @@ func chmod(name string, perm fs.FileMode) (err error) {
 	entries := make([]windows.EXPLICIT_ACCESS, 0, 3)
 	creatorMask, groupMask, worldMask := permToMasks(perm, fi.IsDir())
 
-	sidMasks := container.KeyValues[windows.WELL_KNOWN_SID_TYPE, windows.ACCESS_MASK]{{
+	sidMasks := []struct {
+		Key   windows.WELL_KNOWN_SID_TYPE
+		Value windows.ACCESS_MASK
+	}{{
 		Key:   windows.WinCreatorOwnerSid,
 		Value: creatorMask,
 	}, {
@@ -148,11 +168,11 @@ func chmod(name string, perm fs.FileMode) (err error) {
 		return fmt.Errorf("creating access control list: %w", err)
 	}
 
-	secInfo := windows.SECURITY_INFORMATION(
+	// secInfo defines the parts of a security descriptor to set.
-		windows.DACL_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION,
+	const secInfo windows.SECURITY_INFORMATION = windows.DACL_SECURITY_INFORMATION |
-	)
+		windows.PROTECTED_DACL_SECURITY_INFORMATION
 
-	err = windows.SetNamedSecurityInfo(name, objectType, secInfo, nil, nil, acl, nil)
+	err = windows.SetNamedSecurityInfo(name, windows.SE_FILE_OBJECT, secInfo, nil, nil, acl, nil)
 	if err != nil {
 		return fmt.Errorf("setting security descriptor: %w", err)
 	}
@@ -250,91 +270,131 @@ func newWellKnownTrustee(stype windows.WELL_KNOWN_SID_TYPE) (t *windows.TRUSTEE,
 	}, nil
 }
 
-// Constants reflecting the UNIX permission bits.
+// UNIX file mode permission bits.
 const (
-	ownerWrite = 0b010_000_000
+	permRead    = 0b100
-	groupWrite = 0b000_010_000
+	permWrite   = 0b010
-	worldWrite = 0b000_000_010
+	permExecute = 0b001
-
-	ownerRead = 0b100_000_000
-	groupRead = 0b000_100_000
-	worldRead = 0b000_000_100
-
-	ownerAll = 0b111_000_000
-	groupAll = 0b000_111_000
-	worldAll = 0b000_000_111
 )
 
-// Constants reflecting the number of bits to shift the UNIX permission bits to
+// Windows access masks for appropriate UNIX file mode permission bits and
-// convert them to the generic access rights used by Windows, see
+// file types.
-// https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/access-mask.
 const (
-	genericOwner = 23
+	fileReadRights = windows.READ_CONTROL |
-	genericGroup = 26
+		windows.FILE_READ_DATA |
-	genericWorld = 29
+		windows.FILE_READ_ATTRIBUTES |
-)
+		windows.FILE_READ_EA |
+		windows.SYNCHRONIZE |
+		windows.ACCESS_SYSTEM_SECURITY
 
-// Constants reflecting the number of bits to shift the UNIX write permission
+	fileWriteRights = windows.WRITE_DAC |
-// bits to convert them to the access rights used by Windows.
+		windows.WRITE_OWNER |
-const (
+		windows.FILE_WRITE_DATA |
-	deleteOwner = 9
+		windows.FILE_WRITE_ATTRIBUTES |
-	deleteGroup = 12
+		windows.FILE_WRITE_EA |
-	deleteWorld = 15
+		windows.DELETE |
+		windows.FILE_APPEND_DATA |
+		windows.SYNCHRONIZE |
+		windows.ACCESS_SYSTEM_SECURITY
 
-	listDirOwner = 7
+	fileExecuteRights = windows.FILE_EXECUTE
-	listDirGroup = 10
-	listDirWorld = 13
 
-	traverseOwner = 2
+	dirReadRights = windows.READ_CONTROL |
-	traverseGroup = 5
+		windows.FILE_LIST_DIRECTORY |
-	traverseWorld = 8
+		windows.FILE_READ_EA |
+		windows.FILE_READ_ATTRIBUTES<<1 |
+		windows.SYNCHRONIZE |
+		windows.ACCESS_SYSTEM_SECURITY
 
-	writeEAOwner = 2
+	dirWriteRights = windows.WRITE_DAC |
-	writeEAGroup = 1
+		windows.WRITE_OWNER |
-	writeEAWorld = 4
+		windows.DELETE |
+		windows.FILE_WRITE_DATA |
+		windows.FILE_APPEND_DATA |
+		windows.FILE_WRITE_EA |
+		windows.FILE_WRITE_ATTRIBUTES<<1 |
+		windows.SYNCHRONIZE |
+		windows.ACCESS_SYSTEM_SECURITY
 
-	deleteChildOwner = 1
+	dirExecuteRights = windows.FILE_TRAVERSE
-	deleteChildGroup = 4
-	deleteChildWorld = 7
 )
 
 // permToMasks converts a UNIX file mode permissions to the corresponding
 // Windows access masks.  The [isDir] argument is used to set specific access
 // bits for directories.
 func permToMasks(fm os.FileMode, isDir bool) (owner, group, world windows.ACCESS_MASK) {
-	mask := windows.ACCESS_MASK(fm.Perm())
+	mask := fm.Perm()
 
-	owner = ((mask & ownerAll) << genericOwner) | ((mask & ownerWrite) << deleteOwner)
+	owner = permToMask(byte((mask>>6)&0b111), isDir)
-	group = ((mask & groupAll) << genericGroup) | ((mask & groupWrite) << deleteGroup)
+	group = permToMask(byte((mask>>3)&0b111), isDir)
-	world = ((mask & worldAll) << genericWorld) | ((mask & worldWrite) << deleteWorld)
+	world = permToMask(byte(mask&0b111), isDir)
-
-	if isDir {
-		owner |= (mask & ownerRead) << listDirOwner
-		group |= (mask & groupRead) << listDirGroup
-		world |= (mask & worldRead) << listDirWorld
-
-		owner |= (mask & ownerRead) << traverseOwner
-		group |= (mask & groupRead) << traverseGroup
-		world |= (mask & worldRead) << traverseWorld
-
-		owner |= (mask & ownerWrite) << deleteChildOwner
-		group |= (mask & groupWrite) << deleteChildGroup
-		world |= (mask & worldWrite) << deleteChildWorld
-
-		owner |= (mask & ownerWrite) >> writeEAOwner
-		group |= (mask & groupWrite) << writeEAGroup
-		world |= (mask & worldWrite) << writeEAWorld
-	}
 
 	return owner, group, world
 }
 
+// permToMask converts a UNIX file mode permission bits within p byte to the
+// corresponding Windows access mask.  The [isDir] argument is used to set
+// specific access bits for directories.
+func permToMask(p byte, isDir bool) (mask windows.ACCESS_MASK) {
+	if p&permRead != 0 {
+		if isDir {
+			mask |= dirReadRights
+		} else {
+			mask |= fileReadRights
+		}
+	}
+	if p&permWrite != 0 {
+		if isDir {
+			mask |= dirWriteRights
+		} else {
+			mask |= fileWriteRights
+		}
+	}
+	if p&permExecute != 0 {
+		if isDir {
+			mask |= dirExecuteRights
+		} else {
+			mask |= fileExecuteRights
+		}
+	}
+
+	return mask
+}
+
 // masksToPerm converts Windows access masks to the corresponding UNIX file
 // mode permission bits.
-func masksToPerm(u, g, o windows.ACCESS_MASK) (perm os.FileMode) {
+func masksToPerm(u, g, o windows.ACCESS_MASK, isDir bool) (perm fs.FileMode) {
-	perm |= os.FileMode(((u >> genericOwner) & ownerAll) | ((u >> deleteOwner) & ownerWrite))
+	perm |= fs.FileMode(maskToPerm(u, isDir)) << 6
-	perm |= os.FileMode(((g >> genericGroup) & groupAll) | ((g >> deleteGroup) & groupWrite))
+	perm |= fs.FileMode(maskToPerm(g, isDir)) << 3
-	perm |= os.FileMode(((o >> genericWorld) & worldAll) | ((o >> deleteWorld) & worldWrite))
+	perm |= fs.FileMode(maskToPerm(o, isDir))
+
+	return perm
+}
+
+// maskToPerm converts a Windows access mask to the corresponding UNIX file
+// mode permission bits.
+func maskToPerm(mask windows.ACCESS_MASK, isDir bool) (perm byte) {
+	if isDir {
+		if mask&dirReadRights != 0 {
+			perm |= permRead
+		}
+		if mask&dirWriteRights != 0 {
+			perm |= permWrite
+		}
+		if mask&dirExecuteRights != 0 {
+			perm |= permExecute
+		}
+	} else {
+		if mask&fileReadRights != 0 {
+			perm |= permRead
+		}
+		if mask&fileWriteRights != 0 {
+			perm |= permWrite
+		}
+		if mask&fileExecuteRights != 0 {
+			perm |= permExecute
+		}
+	}
 
 	return perm
 }

internal/aghos/permission_windows_internal_test.go

@@ -10,14 +10,6 @@ import (
 	"golang.org/x/sys/windows"
 )
 
-// Common test constants for the Windows access masks.
-const (
-	winAccessWrite = windows.GENERIC_WRITE | windows.DELETE
-	winAccessFull  = windows.GENERIC_READ | windows.GENERIC_EXECUTE | winAccessWrite
-
-	winAccessDirRead = windows.GENERIC_READ | windows.FILE_LIST_DIRECTORY | windows.FILE_TRAVERSE
-)
-
 func TestPermToMasks(t *testing.T) {
 	t.Parallel()
 
@@ -31,14 +23,14 @@ func TestPermToMasks(t *testing.T) {
 	}{{
 		name:      "all",
 		perm:      0b111_111_111,
-		wantUser:  winAccessFull,
+		wantUser:  fileReadRights | fileWriteRights | fileExecuteRights,
-		wantGroup: winAccessFull,
+		wantGroup: fileReadRights | fileWriteRights | fileExecuteRights,
-		wantOther: winAccessFull,
+		wantOther: fileReadRights | fileWriteRights | fileExecuteRights,
 		isDir:     false,
 	}, {
 		name:      "user_write",
 		perm:      0o010_000_000,
-		wantUser:  winAccessWrite,
+		wantUser:  fileWriteRights,
 		wantGroup: 0,
 		wantOther: 0,
 		isDir:     false,
@@ -46,20 +38,20 @@ func TestPermToMasks(t *testing.T) {
 		name:      "group_read",
 		perm:      0o000_010_000,
 		wantUser:  0,
-		wantGroup: windows.GENERIC_READ,
+		wantGroup: fileReadRights,
 		wantOther: 0,
 		isDir:     false,
 	}, {
 		name:      "all_dir",
 		perm:      0b111_111_111,
-		wantUser:  winAccessFull | winAccessDirRead,
+		wantUser:  dirReadRights | dirWriteRights | dirExecuteRights,
-		wantGroup: winAccessFull | winAccessDirRead,
+		wantGroup: dirReadRights | dirWriteRights | dirExecuteRights,
-		wantOther: winAccessFull | winAccessDirRead,
+		wantOther: dirReadRights | dirWriteRights | dirExecuteRights,
 		isDir:     true,
 	}, {
 		name:      "user_write_dir",
 		perm:      0o010_000_000,
-		wantUser:  winAccessWrite,
+		wantUser:  dirWriteRights,
 		wantGroup: 0,
 		wantOther: 0,
 		isDir:     true,
@@ -86,37 +78,58 @@ func TestMasksToPerm(t *testing.T) {
 		group    windows.ACCESS_MASK
 		other    windows.ACCESS_MASK
 		wantPerm fs.FileMode
+		isDir    bool
 	}{{
 		name:     "all",
-		user:     winAccessFull,
+		user:     fileReadRights | fileWriteRights | fileExecuteRights,
-		group:    winAccessFull,
+		group:    fileReadRights | fileWriteRights | fileExecuteRights,
-		other:    winAccessFull,
+		other:    fileReadRights | fileWriteRights | fileExecuteRights,
 		wantPerm: 0b111_111_111,
+		isDir:    false,
 	}, {
 		name:     "user_write",
-		user:     winAccessWrite,
+		user:     fileWriteRights,
 		group:    0,
 		other:    0,
 		wantPerm: 0o010_000_000,
+		isDir:    false,
 	}, {
 		name:     "group_read",
 		user:     0,
-		group:    windows.GENERIC_READ,
+		group:    fileReadRights,
 		other:    0,
 		wantPerm: 0o000_010_000,
+		isDir:    false,
 	}, {
 		name:     "no_access",
 		user:     0,
 		group:    0,
 		other:    0,
 		wantPerm: 0,
+		isDir:    false,
+	}, {
+		name:     "all_dir",
+		user:     dirReadRights | dirWriteRights | dirExecuteRights,
+		group:    dirReadRights | dirWriteRights | dirExecuteRights,
+		other:    dirReadRights | dirWriteRights | dirExecuteRights,
+		wantPerm: 0b111_111_111,
+		isDir:    true,
+	}, {
+		name:     "user_write_dir",
+		user:     dirWriteRights,
+		group:    0,
+		other:    0,
+		wantPerm: 0o010_000_000,
+		isDir:    true,
 	}}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			assert.Equal(t, tc.wantPerm, masksToPerm(tc.user, tc.group, tc.other))
+			// Don't call [fs.FileMode.Perm] since the result is expected to
+			// contain only the permission bits.
+			assert.Equal(t, tc.wantPerm, masksToPerm(tc.user, tc.group, tc.other, tc.isDir))
 		})
 	}
 }

internal/home/home.go

@@ -687,7 +687,7 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) {
 	}
 
 	if permcheck.NeedsMigration(confPath) {
-		permcheck.Migrate(Context.workDir, dataDir, statsDir, querylogDir, confPath)
+		permcheck.Migrate(Context.workDir, dataDir, statsDir, querylogDir, confPath, execPath)
 	}
 
 	permcheck.Check(Context.workDir, dataDir, statsDir, querylogDir, confPath)

internal/permcheck/migrate.go

@@ -32,11 +32,14 @@ func NeedsMigration(confFilePath string) (ok bool) {
 
 // Migrate attempts to change the permissions of AdGuard Home's files.  It logs
 // the results at an appropriate level.
-func Migrate(workDir, dataDir, statsDir, querylogDir, confFilePath string) {
+func Migrate(workDir, dataDir, statsDir, querylogDir, confFilePath, execPath string) {
 	chmodDir(workDir)
 
 	chmodFile(confFilePath)
 
+	// Allow the binary to be executed.
+	chmodPath(execPath, typeFile, aghos.DefaultPermFile|0b001_000_000)
+
 	// TODO(a.garipov): Put all paths in one place and remove this duplication.
 	chmodDir(dataDir)
 	chmodDir(filepath.Join(dataDir, "filters"))

internal/permcheck/permcheck.go

@@ -60,7 +60,7 @@ func checkFile(filePath string) {
 // checkPath checks the permissions of a single filesystem entity.  The results
 // are logged at the appropriate level.
 func checkPath(entPath, fileType string, want fs.FileMode) {
-	s, err := os.Stat(entPath)
+	s, err := aghos.Stat(entPath)
 	if err != nil {
 		logFunc := log.Error
 		if errors.Is(err, os.ErrNotExist) {

internal/updater/updater.go

@@ -360,9 +360,9 @@ func tarGzFileUnpackOne(outDir string, tr *tar.Reader, hdr *tar.Header) (name st
 		if name == "AdGuardHome" {
 			// Top-level AdGuardHome/.  Skip it.
 			//
-			// TODO(a.garipov): This whole package needs to be
+			// TODO(a.garipov): This whole package needs to be rewritten and
-			// rewritten and covered in more integration tests.  It
+			// covered in more integration tests.  It has weird assumptions and
-			// has weird assumptions and file mode issues.
+			// file mode issues.
 			return "", nil
 		}
 
@@ -383,7 +383,7 @@ func tarGzFileUnpackOne(outDir string, tr *tar.Reader, hdr *tar.Header) (name st
 	}
 
 	var wc io.WriteCloser
-	wc, err = os.OpenFile(
+	wc, err = aghos.OpenFile(
 		outputName,
 		os.O_WRONLY|os.O_CREATE|os.O_TRUNC,
 		os.FileMode(hdr.Mode&0o755),
@@ -464,8 +464,7 @@ func zipFileUnpackOne(outDir string, zf *zip.File) (name string, err error) {
 		if name == "AdGuardHome" {
 			// Top-level AdGuardHome/.  Skip it.
 			//
-			// TODO(a.garipov): See the similar todo in
+			// TODO(a.garipov): See the similar todo in tarGzFileUnpack.
-			// tarGzFileUnpack.
 			return "", nil
 		}
 
@@ -480,7 +479,7 @@ func zipFileUnpackOne(outDir string, zf *zip.File) (name string, err error) {
 	}
 
 	var wc io.WriteCloser
-	wc, err = os.OpenFile(outputName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fi.Mode())
+	wc, err = aghos.OpenFile(outputName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fi.Mode())
 	if err != nil {
 		return "", fmt.Errorf("os.OpenFile(): %w", err)
 	}