From e77de2e67d174f882461b778a81a4b1c1f1b683e Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Tue, 29 Oct 2024 14:28:59 +0300 Subject: [PATCH] Pull request 2294: AGDNS-2455 Windows permissions Closes #7314. Squashed commit of the following: commit f8b6ffeec2f0f96c947cf896c75d05efaca77caf Author: Eugene Burkov Date: Tue Oct 29 14:14:41 2024 +0300 all: fix chlog commit 9417b7dc510296c096f234e2f340dad5a6faf627 Author: Eugene Burkov Date: Mon Oct 28 19:41:30 2024 +0300 aghos: imp doc commit b91f0e72a70a8e1392bd07b50714d8b83cc4e33e Author: Eugene Burkov Date: Mon Oct 28 19:26:15 2024 +0300 all: rm bin commit 9008ee93b181794c5082894bfa5ce4c76153f93d Author: Eugene Burkov Date: Mon Oct 28 18:23:54 2024 +0300 all: revert permcheck commit bcc85d50f5f39269713979c6509a9acd220570b8 Author: Eugene Burkov Date: Mon Oct 28 17:48:55 2024 +0300 all: use aghos more commit 993e351712fbf004a6f96e06061ba2321c1c46e1 Author: Eugene Burkov Date: Mon Oct 28 16:24:56 2024 +0300 all: fix more bugs commit a22b0d265eb0fa747e136363558b97de54e593b8 Author: Eugene Burkov Date: Fri Oct 25 18:30:52 2024 +0300 all: fix bugs commit a2309f812ad3fd83d26c373b67756ea3074f4854 Author: Eugene Burkov Date: Fri Oct 25 17:05:08 2024 +0300 all: fix chlog, imp api commit 42c3f8e91c49998068bc208166de20efe49c3dcb Author: Eugene Burkov Date: Fri Oct 25 16:04:47 2024 +0300 scripts: fix docs commit 9e781ff18db58ed9be35e259ecf3c669a4d41e02 Author: Eugene Burkov Date: Fri Oct 25 16:03:19 2024 +0300 scripts: imp docs commit 1dbc7849828cc4933bb5edc3257f158ac292d48e Author: Eugene Burkov Date: Fri Oct 25 15:55:16 2024 +0300 all: use new functions, add tests commit dcbabaf4e37149a73969c52c9bfac2b9d9127a67 Author: Eugene Burkov Date: Fri Oct 25 13:23:50 2024 +0300 aghos: add stat commit 72d7c0f881835725e65db63ac2dd1c5f7a409036 Author: Eugene Burkov Date: Thu Oct 24 17:10:30 2024 +0300 aghos: add windows functions --- CHANGELOG.md | 5 + internal/aghos/permission.go | 50 +++ internal/aghos/permission_unix.go | 42 ++ internal/aghos/permission_windows.go | 392 ++++++++++++++++++ .../aghos/permission_windows_internal_test.go | 135 ++++++ internal/aghrenameio/renameio_windows.go | 3 +- internal/filtering/filtering.go | 2 +- internal/filtering/http.go | 3 +- internal/home/auth.go | 6 +- internal/home/config.go | 2 +- internal/home/home.go | 2 +- internal/next/cmd/signal.go | 3 +- internal/next/configmgr/configmgr.go | 3 +- internal/permcheck/migrate.go | 4 +- internal/permcheck/permcheck.go | 2 +- internal/querylog/qlogfile.go | 1 + internal/querylog/querylogfile.go | 2 +- internal/stats/stats.go | 8 +- internal/updater/updater.go | 47 ++- scripts/make/go-lint.sh | 10 +- 20 files changed, 682 insertions(+), 40 deletions(-) create mode 100644 internal/aghos/permission.go create mode 100644 internal/aghos/permission_unix.go create mode 100644 internal/aghos/permission_windows.go create mode 100644 internal/aghos/permission_windows_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e2c11b75..de5aeb1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ See also the [v0.107.54 GitHub milestone][ms-v0.107.54]. NOTE: Add new changes BELOW THIS COMMENT. --> +### Security + +- Incorrect handling of sensitive files permissions on Windows ([#7314]). + ### Changed - Improved filtering performance ([#6818]). @@ -38,6 +42,7 @@ NOTE: Add new changes BELOW THIS COMMENT. [#6818]: https://github.com/AdguardTeam/AdGuardHome/issues/6818 [#7250]: https://github.com/AdguardTeam/AdGuardHome/issues/7250 +[#7314]: https://github.com/AdguardTeam/AdGuardHome/issues/7314 [#7315]: https://github.com/AdguardTeam/AdGuardHome/issues/7315 [#7338]: https://github.com/AdguardTeam/AdGuardHome/issues/7338 diff --git a/internal/aghos/permission.go b/internal/aghos/permission.go new file mode 100644 index 00000000..42a1de93 --- /dev/null +++ b/internal/aghos/permission.go @@ -0,0 +1,50 @@ +package aghos + +import ( + "io/fs" + "os" +) + +// TODO(e.burkov): Add platform-independent tests. + +// Chmod is an extension for [os.Chmod] that properly handles Windows access +// rights. +func Chmod(name string, perm fs.FileMode) (err error) { + return chmod(name, perm) +} + +// Mkdir is an extension for [os.Mkdir] that properly handles Windows access +// rights. +func Mkdir(name string, perm fs.FileMode) (err error) { + return mkdir(name, perm) +} + +// MkdirAll is an extension for [os.MkdirAll] that properly handles Windows +// access rights. +func MkdirAll(path string, perm fs.FileMode) (err error) { + return mkdirAll(path, perm) +} + +// WriteFile is an extension for [os.WriteFile] that properly handles Windows +// access rights. +func WriteFile(filename string, data []byte, perm fs.FileMode) (err error) { + return writeFile(filename, data, perm) +} + +// OpenFile is an extension for [os.OpenFile] that properly handles Windows +// access rights. +func OpenFile(name string, flag int, perm fs.FileMode) (file *os.File, err error) { + return openFile(name, flag, perm) +} + +// Stat is an extension for [os.Stat] that properly handles Windows access +// rights. +// +// Note that on Windows the "other" permission bits combines the access rights +// of any trustee that is neither the owner nor the owning group for the file. +// +// TODO(e.burkov): Inspect the behavior for the World (everyone) well-known +// SID and, perhaps, use it. +func Stat(name string) (fi fs.FileInfo, err error) { + return stat(name) +} diff --git a/internal/aghos/permission_unix.go b/internal/aghos/permission_unix.go new file mode 100644 index 00000000..8c2573ca --- /dev/null +++ b/internal/aghos/permission_unix.go @@ -0,0 +1,42 @@ +//go:build unix + +package aghos + +import ( + "io/fs" + "os" + + "github.com/google/renameio/v2/maybe" +) + +// chmod is a Unix implementation of [Chmod]. +func chmod(name string, perm fs.FileMode) (err error) { + return os.Chmod(name, perm) +} + +// mkdir is a Unix implementation of [Mkdir]. +func mkdir(name string, perm fs.FileMode) (err error) { + return os.Mkdir(name, perm) +} + +// mkdirAll is a Unix implementation of [MkdirAll]. +func mkdirAll(path string, perm fs.FileMode) (err error) { + return os.MkdirAll(path, perm) +} + +// writeFile is a Unix implementation of [WriteFile]. +func writeFile(filename string, data []byte, perm fs.FileMode) (err error) { + return maybe.WriteFile(filename, data, perm) +} + +// openFile is a Unix implementation of [OpenFile]. +func openFile(name string, flag int, perm fs.FileMode) (file *os.File, err error) { + // #nosec G304 -- This function simply wraps the [os.OpenFile] function, so + // the security concerns should be addressed to the [OpenFile] calls. + return os.OpenFile(name, flag, perm) +} + +// stat is a Unix implementation of [Stat]. +func stat(name string) (fi os.FileInfo, err error) { + return os.Stat(name) +} diff --git a/internal/aghos/permission_windows.go b/internal/aghos/permission_windows.go new file mode 100644 index 00000000..cc5a4aa8 --- /dev/null +++ b/internal/aghos/permission_windows.go @@ -0,0 +1,392 @@ +//go:build windows + +package aghos + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + "unsafe" + + "github.com/AdguardTeam/golibs/errors" + "golang.org/x/sys/windows" +) + +// 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 data + // successfully retrieved by [os.Stat]. + fs.FileInfo + + // mode is the file mode converted from the security descriptor. + mode fs.FileMode +} + +// type check +var _ fs.FileInfo = (*fileInfo)(nil) + +// Mode implements [fs.FileInfo.Mode] for [*fileInfo]. +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) { + 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 + } + + dacl, owner, group, err := retrieveDACL(absName) + if err != nil { + // Don't wrap the error, since it's informative enough as is. + return nil, err + } + + var ownerMask, groupMask, otherMask windows.ACCESS_MASK + for i := range uint32(dacl.AceCount) { + var ace *windows.ACCESS_ALLOWED_ACE + err = windows.GetAce(dacl, i, &ace) + if err != nil { + return nil, fmt.Errorf("getting access control entry at index %d: %w", i, err) + } + + entrySid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + switch { + case entrySid.Equals(owner): + ownerMask |= ace.Mask + case entrySid.Equals(group): + groupMask |= ace.Mask + default: + otherMask |= ace.Mask + } + } + + mode := fi.Mode() + perm := masksToPerm(ownerMask, groupMask, otherMask, mode.IsDir()) + + return &fileInfo{ + FileInfo: fi, + // 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) { + fi, err := os.Stat(name) + if err != nil { + return fmt.Errorf("getting file info: %w", err) + } + + entries := make([]windows.EXPLICIT_ACCESS, 0, 3) + creatorMask, groupMask, worldMask := permToMasks(perm, fi.IsDir()) + + sidMasks := []struct { + Key windows.WELL_KNOWN_SID_TYPE + Value windows.ACCESS_MASK + }{{ + Key: windows.WinCreatorOwnerSid, + Value: creatorMask, + }, { + Key: windows.WinCreatorGroupSid, + Value: groupMask, + }, { + Key: windows.WinWorldSid, + Value: worldMask, + }} + + var errs []error + for _, sidMask := range sidMasks { + if sidMask.Value == 0 { + continue + } + + var trustee windows.TRUSTEE + trustee, err = newWellKnownTrustee(sidMask.Key) + if err != nil { + errs = append(errs, err) + + continue + } + + entries = append(entries, windows.EXPLICIT_ACCESS{ + AccessPermissions: sidMask.Value, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: trustee, + }) + } + + if err = errors.Join(errs...); err != nil { + return fmt.Errorf("creating access control entries: %w", err) + } + + acl, err := windows.ACLFromEntries(entries, nil) + if err != nil { + return fmt.Errorf("creating access control list: %w", err) + } + + // 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, windows.SE_FILE_OBJECT, secInfo, nil, nil, acl, nil) + if err != nil { + return fmt.Errorf("setting security descriptor: %w", err) + } + + return nil +} + +// mkdir is a Windows implementation of [Mkdir]. +// +// TODO(e.burkov): Consider using [windows.CreateDirectory] instead of +// [os.Mkdir] to reduce the number of syscalls. +func mkdir(name string, perm os.FileMode) (err error) { + name, err = filepath.Abs(name) + if err != nil { + return fmt.Errorf("computing absolute path: %w", err) + } + + err = os.Mkdir(name, perm) + if err != nil { + return fmt.Errorf("creating directory: %w", err) + } + + defer func() { + if err != nil { + err = errors.WithDeferred(err, os.Remove(name)) + } + }() + + return chmod(name, perm) +} + +// mkdirAll is a Windows implementation of [MkdirAll]. +func mkdirAll(path string, perm os.FileMode) (err error) { + parent, _ := filepath.Split(path) + + if parent != "" { + err = os.MkdirAll(parent, perm) + if err != nil && !errors.Is(err, os.ErrExist) { + return fmt.Errorf("creating parent directories: %w", err) + } + } + + err = mkdir(path, perm) + if errors.Is(err, os.ErrExist) { + return nil + } + + return err +} + +// writeFile is a Windows implementation of [WriteFile]. +func writeFile(filename string, data []byte, perm os.FileMode) (err error) { + file, err := openFile(filename, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm) + if err != nil { + return fmt.Errorf("opening file: %w", err) + } + defer func() { err = errors.WithDeferred(err, file.Close()) }() + + _, err = file.Write(data) + if err != nil { + return fmt.Errorf("writing data: %w", err) + } + + return nil +} + +// openFile is a Windows implementation of [OpenFile]. +func openFile(name string, flag int, perm os.FileMode) (file *os.File, err error) { + // Only change permissions if the file not yet exists, but should be + // created. + if flag&os.O_CREATE == 0 { + return os.OpenFile(name, flag, perm) + } + + _, err = stat(name) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + defer func() { err = errors.WithDeferred(err, chmod(name, perm)) }() + } else { + return nil, fmt.Errorf("getting file info: %w", err) + } + } + + return os.OpenFile(name, flag, perm) +} + +// newWellKnownTrustee returns a trustee for a well-known SID. +func newWellKnownTrustee(stype windows.WELL_KNOWN_SID_TYPE) (t windows.TRUSTEE, err error) { + sid, err := windows.CreateWellKnownSid(stype) + if err != nil { + return windows.TRUSTEE{}, fmt.Errorf("creating sid for type %d: %w", stype, err) + } + + return windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, nil +} + +// UNIX file mode permission bits. +const ( + permRead = 0b100 + permWrite = 0b010 + permExecute = 0b001 +) + +// Windows access masks for appropriate UNIX file mode permission bits and +// file types. +const ( + fileReadRights windows.ACCESS_MASK = windows.READ_CONTROL | + windows.FILE_READ_DATA | + windows.FILE_READ_ATTRIBUTES | + windows.FILE_READ_EA | + windows.SYNCHRONIZE | + windows.ACCESS_SYSTEM_SECURITY + + fileWriteRights windows.ACCESS_MASK = 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 + + fileExecuteRights windows.ACCESS_MASK = windows.FILE_EXECUTE + + dirReadRights windows.ACCESS_MASK = windows.READ_CONTROL | + windows.FILE_LIST_DIRECTORY | + windows.FILE_READ_EA | + windows.FILE_READ_ATTRIBUTES<<1 | + windows.SYNCHRONIZE | + windows.ACCESS_SYSTEM_SECURITY + + dirWriteRights windows.ACCESS_MASK = 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 + + dirExecuteRights windows.ACCESS_MASK = 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 := fm.Perm() + + 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) { + readRights, writeRights, executeRights := fileReadRights, fileWriteRights, fileExecuteRights + if isDir { + readRights, writeRights, executeRights = dirReadRights, dirWriteRights, dirExecuteRights + } + + if p&permRead != 0 { + mask |= readRights + } + if p&permWrite != 0 { + mask |= writeRights + } + if p&permExecute != 0 { + mask |= executeRights + } + + return mask +} + +// masksToPerm converts Windows access masks to the corresponding UNIX file +// mode permission bits. +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) { + readMask, writeMask, executeMask := fileReadRights, fileWriteRights, fileExecuteRights + if isDir { + readMask, writeMask, executeMask = dirReadRights, dirWriteRights, dirExecuteRights + } + + // Remove common bits to avoid false positive detection of unset rights. + readMask ^= windows.SYNCHRONIZE | windows.ACCESS_SYSTEM_SECURITY + writeMask ^= windows.SYNCHRONIZE | windows.ACCESS_SYSTEM_SECURITY + + if mask&readMask != 0 { + perm |= permRead + } + if mask&writeMask != 0 { + perm |= permWrite + } + if mask&executeMask != 0 { + perm |= permExecute + } + + return perm +} diff --git a/internal/aghos/permission_windows_internal_test.go b/internal/aghos/permission_windows_internal_test.go new file mode 100644 index 00000000..3837fda1 --- /dev/null +++ b/internal/aghos/permission_windows_internal_test.go @@ -0,0 +1,135 @@ +//go:build windows + +package aghos + +import ( + "io/fs" + "testing" + + "github.com/stretchr/testify/assert" + "golang.org/x/sys/windows" +) + +func TestPermToMasks(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + perm fs.FileMode + wantUser windows.ACCESS_MASK + wantGroup windows.ACCESS_MASK + wantOther windows.ACCESS_MASK + isDir bool + }{{ + name: "all", + perm: 0b111_111_111, + wantUser: fileReadRights | fileWriteRights | fileExecuteRights, + wantGroup: fileReadRights | fileWriteRights | fileExecuteRights, + wantOther: fileReadRights | fileWriteRights | fileExecuteRights, + isDir: false, + }, { + name: "user_write", + perm: 0b010_000_000, + wantUser: fileWriteRights, + wantGroup: 0, + wantOther: 0, + isDir: false, + }, { + name: "group_read", + perm: 0b000_100_000, + wantUser: 0, + wantGroup: fileReadRights, + wantOther: 0, + isDir: false, + }, { + name: "all_dir", + perm: 0b111_111_111, + wantUser: dirReadRights | dirWriteRights | dirExecuteRights, + wantGroup: dirReadRights | dirWriteRights | dirExecuteRights, + wantOther: dirReadRights | dirWriteRights | dirExecuteRights, + isDir: true, + }, { + name: "user_write_dir", + perm: 0b010_000_000, + wantUser: dirWriteRights, + wantGroup: 0, + wantOther: 0, + isDir: true, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + user, group, other := permToMasks(tc.perm, tc.isDir) + assert.Equal(t, tc.wantUser, user) + assert.Equal(t, tc.wantGroup, group) + assert.Equal(t, tc.wantOther, other) + }) + } +} + +func TestMasksToPerm(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + user windows.ACCESS_MASK + group windows.ACCESS_MASK + other windows.ACCESS_MASK + wantPerm fs.FileMode + isDir bool + }{{ + name: "all", + user: fileReadRights | fileWriteRights | fileExecuteRights, + group: fileReadRights | fileWriteRights | fileExecuteRights, + other: fileReadRights | fileWriteRights | fileExecuteRights, + wantPerm: 0b111_111_111, + isDir: false, + }, { + name: "user_write", + user: fileWriteRights, + group: 0, + other: 0, + wantPerm: 0b010_000_000, + isDir: false, + }, { + name: "group_read", + user: 0, + group: fileReadRights, + other: 0, + wantPerm: 0b000_100_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: 0b010_000_000, + isDir: true, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // 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/aghrenameio/renameio_windows.go b/internal/aghrenameio/renameio_windows.go index d4f7cddd..b4d88f4f 100644 --- a/internal/aghrenameio/renameio_windows.go +++ b/internal/aghrenameio/renameio_windows.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" ) @@ -62,7 +63,7 @@ func newPendingFile(filePath string, mode fs.FileMode) (f PendingFile, err error return nil, fmt.Errorf("opening pending file: %w", err) } - err = file.Chmod(mode) + err = aghos.Chmod(file.Name(), mode) if err != nil { return nil, fmt.Errorf("preparing pending file: %w", err) } diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go index 8836515c..f4bd1f10 100644 --- a/internal/filtering/filtering.go +++ b/internal/filtering/filtering.go @@ -1057,7 +1057,7 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter, err error) { } } - err = os.MkdirAll(filepath.Join(d.conf.DataDir, filterDir), aghos.DefaultPermDir) + err = aghos.MkdirAll(filepath.Join(d.conf.DataDir, filterDir), aghos.DefaultPermDir) if err != nil { d.Close() diff --git a/internal/filtering/http.go b/internal/filtering/http.go index e459cb2f..08832b40 100644 --- a/internal/filtering/http.go +++ b/internal/filtering/http.go @@ -13,6 +13,7 @@ import ( "time" "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" @@ -25,7 +26,7 @@ func (d *DNSFilter) validateFilterURL(urlStr string) (err error) { if filepath.IsAbs(urlStr) { urlStr = filepath.Clean(urlStr) - _, err = os.Stat(urlStr) + _, err = aghos.Stat(urlStr) if err != nil { // Don't wrap the error since it's informative enough as is. return err diff --git a/internal/home/auth.go b/internal/home/auth.go index 5b6cf9b8..040c70fd 100644 --- a/internal/home/auth.go +++ b/internal/home/auth.go @@ -90,7 +90,11 @@ func InitAuth( trustedProxies: trustedProxies, } var err error - a.db, err = bbolt.Open(dbFilename, aghos.DefaultPermFile, nil) + + opts := *bbolt.DefaultOptions + opts.OpenFile = aghos.OpenFile + + a.db, err = bbolt.Open(dbFilename, aghos.DefaultPermFile, &opts) if err != nil { log.Error("auth: open DB: %s: %s", dbFilename, err) if err.Error() == "invalid argument" { diff --git a/internal/home/config.go b/internal/home/config.go index 20d2eb97..810ec24e 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -708,7 +708,7 @@ func (c *configuration) write() (err error) { return fmt.Errorf("generating config file: %w", err) } - err = maybe.WriteFile(confPath, buf.Bytes(), aghos.DefaultPermFile) + err = aghos.WriteFile(confPath, buf.Bytes(), aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing config file: %w", err) } diff --git a/internal/home/home.go b/internal/home/home.go index b986e972..ed96dee1 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -643,7 +643,7 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) { } dataDir := Context.getDataDir() - err = os.MkdirAll(dataDir, aghos.DefaultPermDir) + err = aghos.MkdirAll(dataDir, aghos.DefaultPermDir) fatalOnError(errors.Annotate(err, "creating DNS data dir at %s: %w", dataDir)) GLMode = opts.glinetMode diff --git a/internal/next/cmd/signal.go b/internal/next/cmd/signal.go index a9f8543f..2454e062 100644 --- a/internal/next/cmd/signal.go +++ b/internal/next/cmd/signal.go @@ -9,7 +9,6 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/next/configmgr" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/osutil" - "github.com/google/renameio/v2/maybe" ) // signalHandler processes incoming signals and shuts services down. @@ -142,7 +141,7 @@ func (h *signalHandler) writePID() { data = strconv.AppendInt(data, int64(os.Getpid()), 10) data = append(data, '\n') - err := maybe.WriteFile(h.pidFile, data, 0o644) + err := aghos.WriteFile(h.pidFile, data, 0o644) if err != nil { log.Error("sighdlr: writing pidfile: %s", err) diff --git a/internal/next/configmgr/configmgr.go b/internal/next/configmgr/configmgr.go index a75f1304..a22b5bbb 100644 --- a/internal/next/configmgr/configmgr.go +++ b/internal/next/configmgr/configmgr.go @@ -21,7 +21,6 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/timeutil" - "github.com/google/renameio/v2/maybe" "gopkg.in/yaml.v3" ) @@ -183,7 +182,7 @@ func (m *Manager) write() (err error) { return fmt.Errorf("encoding: %w", err) } - err = maybe.WriteFile(m.fileName, b, aghos.DefaultPermFile) + err = aghos.WriteFile(m.fileName, b, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing: %w", err) } diff --git a/internal/permcheck/migrate.go b/internal/permcheck/migrate.go index b052bffa..65d704c4 100644 --- a/internal/permcheck/migrate.go +++ b/internal/permcheck/migrate.go @@ -14,7 +14,7 @@ import ( // // TODO(a.garipov): Consider ways to detect this better. func NeedsMigration(confFilePath string) (ok bool) { - s, err := os.Stat(confFilePath) + s, err := aghos.Stat(confFilePath) if err != nil { if errors.Is(err, os.ErrNotExist) { // Likely a first run. Don't check. @@ -70,7 +70,7 @@ func chmodFile(filePath string) { // chmodPath changes the permissions of a single filesystem entity. The results // are logged at the appropriate level. func chmodPath(entPath, fileType string, fm fs.FileMode) { - err := os.Chmod(entPath, fm) + err := aghos.Chmod(entPath, fm) if err == nil { log.Info("permcheck: changed permissions for %s %q", fileType, entPath) 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/querylog/qlogfile.go b/internal/querylog/qlogfile.go index 175420ea..c145f520 100644 --- a/internal/querylog/qlogfile.go +++ b/internal/querylog/qlogfile.go @@ -57,6 +57,7 @@ type qLogFile struct { // newQLogFile initializes a new instance of the qLogFile. func newQLogFile(path string) (qf *qLogFile, err error) { + // Don't use [aghos.OpenFile] here, because the file is expected to exist. f, err := os.OpenFile(path, os.O_RDONLY, aghos.DefaultPermFile) if err != nil { return nil, err diff --git a/internal/querylog/querylogfile.go b/internal/querylog/querylogfile.go index 412a364b..6b4760c0 100644 --- a/internal/querylog/querylogfile.go +++ b/internal/querylog/querylogfile.go @@ -71,7 +71,7 @@ func (l *queryLog) flushToFile(b *bytes.Buffer) (err error) { filename := l.logFile - f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, aghos.DefaultPermFile) + f, err := aghos.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("creating file %q: %w", filename, err) } diff --git a/internal/stats/stats.go b/internal/stats/stats.go index 3b858ea4..38affdb9 100644 --- a/internal/stats/stats.go +++ b/internal/stats/stats.go @@ -384,7 +384,13 @@ func (s *StatsCtx) openDB() (err error) { s.logger.Debug("opening database") var db *bbolt.DB - db, err = bbolt.Open(s.filename, aghos.DefaultPermFile, nil) + + opts := *bbolt.DefaultOptions + // Use the custom OpenFile function to properly handle access rights on + // Windows. + opts.OpenFile = aghos.OpenFile + + db, err = bbolt.Open(s.filename, aghos.DefaultPermFile, &opts) if err != nil { if err.Error() == "invalid argument" { const lines = `AdGuard Home cannot be initialized due to an incompatible file system. diff --git a/internal/updater/updater.go b/internal/updater/updater.go index b3e85dee..e2a990df 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -264,7 +264,7 @@ func (u *Updater) check() (err error) { // ignores the configuration file if firstRun is true. func (u *Updater) backup(firstRun bool) (err error) { log.Debug("updater: backing up current configuration") - _ = os.Mkdir(u.backupDir, aghos.DefaultPermDir) + _ = aghos.Mkdir(u.backupDir, aghos.DefaultPermDir) if !firstRun { err = copyFile(u.confName, filepath.Join(u.backupDir, "AdGuardHome.yaml")) if err != nil { @@ -338,12 +338,12 @@ func (u *Updater) downloadPackageFile() (err error) { return fmt.Errorf("io.ReadAll() failed: %w", err) } - _ = os.Mkdir(u.updateDir, aghos.DefaultPermDir) + _ = aghos.Mkdir(u.updateDir, aghos.DefaultPermDir) log.Debug("updater: saving package to file") - err = os.WriteFile(u.packageName, body, aghos.DefaultPermFile) + err = aghos.WriteFile(u.packageName, body, aghos.DefaultPermFile) if err != nil { - return fmt.Errorf("os.WriteFile() failed: %w", err) + return fmt.Errorf("writing package file: %w", err) } return nil } @@ -360,15 +360,15 @@ 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 } - err = os.Mkdir(outputName, os.FileMode(hdr.Mode&0o755)) + err = aghos.Mkdir(outputName, os.FileMode(hdr.Mode&0o755)) if err != nil && !errors.Is(err, os.ErrExist) { - return "", fmt.Errorf("os.Mkdir(%q): %w", outputName, err) + return "", fmt.Errorf("creating directory %q: %w", outputName, err) } log.Debug("updater: created directory %q", outputName) @@ -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,14 +464,13 @@ 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 } - err = os.Mkdir(outputName, fi.Mode()) + err = aghos.Mkdir(outputName, fi.Mode()) if err != nil && !errors.Is(err, os.ErrExist) { - return "", fmt.Errorf("os.Mkdir(%q): %w", outputName, err) + return "", fmt.Errorf("creating directory %q: %w", outputName, err) } log.Debug("updater: created directory %q", outputName) @@ -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) } @@ -523,15 +522,19 @@ func zipFileUnpack(zipfile, outDir string) (files []string, err error) { } // Copy file on disk -func copyFile(src, dst string) error { - d, e := os.ReadFile(src) - if e != nil { - return e +func copyFile(src, dst string) (err error) { + d, err := os.ReadFile(src) + if err != nil { + // Don't wrap the error, since it's informative enough as is. + return err } - e = os.WriteFile(dst, d, aghos.DefaultPermFile) - if e != nil { - return e + + err = aghos.WriteFile(dst, d, aghos.DefaultPermFile) + if err != nil { + // Don't wrap the error, since it's informative enough as is. + return err } + return nil } diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index c6623b13..41f70d76 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -54,9 +54,12 @@ set -f -u # # * Package unsafe is… unsafe. # -# Currently, the only standard exception are files generated from protobuf -# schemas, which use package reflect. If your project needs more exceptions, -# add and document them. +# If your project needs more exceptions, add and document them. Currently, +# there are only two standard exceptions: +# +# * Files generated from protobuf schemas, which use package reflect. +# +# * Windows-specific code caused by golang.org/x/sys/windows API design. # # TODO(a.garipov): Add golibs/log. # @@ -75,6 +78,7 @@ blocklist_imports() { -n\ -- '*.go'\ ':!*.pb.go'\ + ':!./internal/aghos/permission_windows.go'\ | sed -e 's/^\([^[:space:]]\+\)\(.*\)$/\1 blocked import:\2/'\ || exit 0 }