From 993e351712fbf004a6f96e06061ba2321c1c46e1 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Mon, 28 Oct 2024 16:24:56 +0300 Subject: [PATCH] all: fix more bugs --- internal/aghos/permission_windows.go | 262 +++++++++++------- ...go => permission_windows_internal_test.go} | 59 ++-- internal/home/home.go | 2 +- internal/permcheck/migrate.go | 5 +- internal/permcheck/permcheck.go | 2 +- internal/updater/updater.go | 13 +- 6 files changed, 209 insertions(+), 134 deletions(-) rename internal/aghos/{permission_windows_interal_test.go => permission_windows_internal_test.go} (57%) diff --git a/internal/aghos/permission_windows.go b/internal/aghos/permission_windows.go index 9fd6abd4..64bcd757 100644 --- a/internal/aghos/permission_windows.go +++ b/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 - - 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) + dacl, owner, group, err := retrieveDACL(absName) if err != nil { - return nil, fmt.Errorf("getting security descriptor: %w", 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) + // Don't wrap the error, since it's informative enough as is. + return nil, 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( - windows.DACL_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION, - ) + // secInfo defines the parts of a security descriptor to set. + 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 - groupWrite = 0b000_010_000 - worldWrite = 0b000_000_010 - - ownerRead = 0b100_000_000 - groupRead = 0b000_100_000 - worldRead = 0b000_000_100 - - ownerAll = 0b111_000_000 - groupAll = 0b000_111_000 - worldAll = 0b000_000_111 + permRead = 0b100 + permWrite = 0b010 + permExecute = 0b001 ) -// Constants reflecting the number of bits to shift the UNIX permission bits to -// convert them to the generic access rights used by Windows, see -// https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/access-mask. +// Windows access masks for appropriate UNIX file mode permission bits and +// file types. const ( - genericOwner = 23 - genericGroup = 26 - genericWorld = 29 -) + fileReadRights = windows.READ_CONTROL | + windows.FILE_READ_DATA | + 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 -// bits to convert them to the access rights used by Windows. -const ( - deleteOwner = 9 - deleteGroup = 12 - deleteWorld = 15 + fileWriteRights = windows.WRITE_DAC | + windows.WRITE_OWNER | + windows.FILE_WRITE_DATA | + windows.FILE_WRITE_ATTRIBUTES | + windows.FILE_WRITE_EA | + windows.DELETE | + windows.FILE_APPEND_DATA | + windows.SYNCHRONIZE | + windows.ACCESS_SYSTEM_SECURITY - listDirOwner = 7 - listDirGroup = 10 - listDirWorld = 13 + fileExecuteRights = windows.FILE_EXECUTE - traverseOwner = 2 - traverseGroup = 5 - traverseWorld = 8 + dirReadRights = windows.READ_CONTROL | + windows.FILE_LIST_DIRECTORY | + windows.FILE_READ_EA | + windows.FILE_READ_ATTRIBUTES<<1 | + windows.SYNCHRONIZE | + windows.ACCESS_SYSTEM_SECURITY - writeEAOwner = 2 - writeEAGroup = 1 - writeEAWorld = 4 + dirWriteRights = windows.WRITE_DAC | + windows.WRITE_OWNER | + 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 - deleteChildGroup = 4 - deleteChildWorld = 7 + dirExecuteRights = windows.FILE_TRAVERSE ) // 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) - group = ((mask & groupAll) << genericGroup) | ((mask & groupWrite) << deleteGroup) - world = ((mask & worldAll) << genericWorld) | ((mask & worldWrite) << deleteWorld) - - 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 - } + owner = permToMask(byte((mask>>6)&0b111), isDir) + group = permToMask(byte((mask>>3)&0b111), isDir) + world = permToMask(byte(mask&0b111), isDir) 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) { - perm |= os.FileMode(((u >> genericOwner) & ownerAll) | ((u >> deleteOwner) & ownerWrite)) - perm |= os.FileMode(((g >> genericGroup) & groupAll) | ((g >> deleteGroup) & groupWrite)) - perm |= os.FileMode(((o >> genericWorld) & worldAll) | ((o >> deleteWorld) & worldWrite)) +func masksToPerm(u, g, o windows.ACCESS_MASK, isDir bool) (perm fs.FileMode) { + perm |= fs.FileMode(maskToPerm(u, isDir)) << 6 + perm |= fs.FileMode(maskToPerm(g, isDir)) << 3 + 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 } diff --git a/internal/aghos/permission_windows_interal_test.go b/internal/aghos/permission_windows_internal_test.go similarity index 57% rename from internal/aghos/permission_windows_interal_test.go rename to internal/aghos/permission_windows_internal_test.go index 65a11ed9..ed3566a4 100644 --- a/internal/aghos/permission_windows_interal_test.go +++ b/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, - wantGroup: winAccessFull, - wantOther: winAccessFull, + wantUser: fileReadRights | fileWriteRights | fileExecuteRights, + wantGroup: fileReadRights | fileWriteRights | fileExecuteRights, + 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, - wantGroup: winAccessFull | winAccessDirRead, - wantOther: winAccessFull | winAccessDirRead, + wantUser: dirReadRights | dirWriteRights | dirExecuteRights, + wantGroup: dirReadRights | dirWriteRights | dirExecuteRights, + 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, - group: winAccessFull, - other: winAccessFull, + user: fileReadRights | fileWriteRights | fileExecuteRights, + group: fileReadRights | fileWriteRights | fileExecuteRights, + 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)) }) } } diff --git a/internal/home/home.go b/internal/home/home.go index ed96dee1..f79832e5 100644 --- a/internal/home/home.go +++ b/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) diff --git a/internal/permcheck/migrate.go b/internal/permcheck/migrate.go index 65d704c4..ce032f75 100644 --- a/internal/permcheck/migrate.go +++ b/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")) diff --git a/internal/permcheck/permcheck.go b/internal/permcheck/permcheck.go index aea4a743..ace62a5e 100644 --- a/internal/permcheck/permcheck.go +++ b/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) { diff --git a/internal/updater/updater.go b/internal/updater/updater.go index ca2abc44..e2a990df 100644 --- a/internal/updater/updater.go +++ b/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 - // rewritten and covered in more integration tests. It - // has weird assumptions and file mode issues. + // TODO(a.garipov): This whole package needs to be rewritten and + // covered in more integration tests. It has weird assumptions and + // 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 - // tarGzFileUnpack. + // TODO(a.garipov): See the similar todo in 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) }