From 4d470f94840c46d22f6dc1454a84a8fdc11eda6b Mon Sep 17 00:00:00 2001 From: Margaret Fero Date: Mon, 2 Dec 2024 00:45:18 -0800 Subject: [PATCH 1/3] Update wiki-start in README.md When I clicked the link aliased as wiki-start, the article had a banner at the top redirecting readers to a newer version on the wiki. This commit replaces the link to the outdated version with the link it redirects to, to save future visitors a click. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e8dd4c68662..8239bab74ed 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,7 @@ If you're running **Linux,** there's a secure and easy way to install AdGuard Ho [Docker Hub]: https://hub.docker.com/r/adguard/adguardhome [Snap Store]: https://snapcraft.io/adguard-home -[wiki-start]: https://github.com/AdguardTeam/AdGuardHome/wiki/Getting-Started +[wiki-start]: https://adguard-dns.io/kb/adguard-home/getting-started/ ### Guides From 3895cfb4f01c1bc2731447e883fa58e96e2bb589 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Tue, 3 Dec 2024 18:26:00 +0300 Subject: [PATCH 2/3] Pull request 2312: 7400 Windows permcheck Updates #7400. Squashed commit of the following: commit f50d7c200de545dc6c8ef70b39208f522033fb90 Merge: 47040a14c 37b16bcf7 Author: Eugene Burkov Date: Tue Dec 3 18:09:23 2024 +0300 Merge branch 'master' into 7400-chown-permcheck commit 47040a14cd50bf50429f44eba0acdcf736412b61 Author: Eugene Burkov Date: Tue Dec 3 14:26:43 2024 +0300 permcheck: fix nil entries commit e1d21c576d75a903b88db3b7beb82348cdcf60c9 Author: Eugene Burkov Date: Mon Dec 2 15:37:58 2024 +0300 permcheck: fix nil owner commit b1fc67c4d189293d0aee90c1905f7f387840643b Author: Eugene Burkov Date: Fri Nov 29 18:07:15 2024 +0300 permcheck: imp doc commit 0b6a71326e249f0923e389aa1f6f164b02802a24 Author: Eugene Burkov Date: Fri Nov 29 17:16:24 2024 +0300 permcheck: imp code commit 7dfbeda179d0ddb81db54fa4e0dcff189b400215 Author: Eugene Burkov Date: Fri Nov 29 14:28:17 2024 +0300 permcheck: imp code commit 3a5b6aced948a2d09fdae823fc986266c9984b3d Author: Eugene Burkov Date: Thu Nov 28 19:21:03 2024 +0300 all: imp code, docs commit c076c9366934303fa8c5909bd13770e367dca72e Author: Eugene Burkov Date: Thu Nov 28 15:14:06 2024 +0300 permcheck: imp code, docs commit 09e4ae1ba12e195454f1db11fa2f5c9e8e170f06 Author: Eugene Burkov Date: Wed Nov 27 19:19:11 2024 +0300 all: implement windows permcheck commit b75ed7d4d30e289b8a99e68e6a5e94ab74cf49cb Author: Eugene Burkov Date: Mon Nov 25 18:01:47 2024 +0300 all: revert permissions --- CHANGELOG.md | 3 + 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 | 5 +- internal/filtering/filtering.go | 2 +- internal/filtering/http.go | 3 +- internal/home/auth.go | 5 +- internal/home/config.go | 2 +- internal/home/home.go | 22 +- internal/next/cmd/signal.go | 4 +- internal/next/configmgr/configmgr.go | 3 +- internal/permcheck/check_unix.go | 43 ++ internal/permcheck/check_windows.go | 60 +++ internal/permcheck/migrate.go | 93 ----- internal/permcheck/migrate_unix.go | 66 +++ internal/permcheck/migrate_windows.go | 135 ++++++ internal/permcheck/permcheck.go | 93 ++--- internal/permcheck/security_unix.go | 123 ++++++ internal/permcheck/security_windows.go | 167 ++++++++ internal/querylog/qlogfile.go | 1 - internal/querylog/querylogfile.go | 2 +- internal/stats/stats.go | 7 +- internal/updater/updater.go | 30 +- scripts/make/go-lint.sh | 6 +- 26 files changed, 669 insertions(+), 825 deletions(-) delete mode 100644 internal/aghos/permission.go delete mode 100644 internal/aghos/permission_unix.go delete mode 100644 internal/aghos/permission_windows.go delete mode 100644 internal/aghos/permission_windows_internal_test.go create mode 100644 internal/permcheck/check_unix.go create mode 100644 internal/permcheck/check_windows.go delete mode 100644 internal/permcheck/migrate.go create mode 100644 internal/permcheck/migrate_unix.go create mode 100644 internal/permcheck/migrate_windows.go create mode 100644 internal/permcheck/security_unix.go create mode 100644 internal/permcheck/security_windows.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ff15938599c..ccc76f37015 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/aghos/permission.go b/internal/aghos/permission.go deleted file mode 100644 index 42a1de93ba3..00000000000 --- a/internal/aghos/permission.go +++ /dev/null @@ -1,50 +0,0 @@ -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 deleted file mode 100644 index 8c2573ca91c..00000000000 --- a/internal/aghos/permission_unix.go +++ /dev/null @@ -1,42 +0,0 @@ -//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 deleted file mode 100644 index cc5a4aa8f46..00000000000 --- a/internal/aghos/permission_windows.go +++ /dev/null @@ -1,392 +0,0 @@ -//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 deleted file mode 100644 index 3837fda1b04..00000000000 --- a/internal/aghos/permission_windows_internal_test.go +++ /dev/null @@ -1,135 +0,0 @@ -//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 b4d88f4f851..0a677035005 100644 --- a/internal/aghrenameio/renameio_windows.go +++ b/internal/aghrenameio/renameio_windows.go @@ -8,7 +8,6 @@ import ( "os" "path/filepath" - "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" ) @@ -63,7 +62,9 @@ func newPendingFile(filePath string, mode fs.FileMode) (f PendingFile, err error return nil, fmt.Errorf("opening pending file: %w", err) } - err = aghos.Chmod(file.Name(), mode) + // 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/filtering/filtering.go b/internal/filtering/filtering.go index f4bd1f100d7..8836515c6b5 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 = aghos.MkdirAll(filepath.Join(d.conf.DataDir, filterDir), aghos.DefaultPermDir) + err = os.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 60b0d1d59d4..94a601f53ac 100644 --- a/internal/filtering/http.go +++ b/internal/filtering/http.go @@ -13,7 +13,6 @@ 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" @@ -27,7 +26,7 @@ func (d *DNSFilter) validateFilterURL(urlStr string) (err error) { if filepath.IsAbs(urlStr) { urlStr = filepath.Clean(urlStr) - _, err = aghos.Stat(urlStr) + _, err = os.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 040c70fd8ec..969152af5ef 100644 --- a/internal/home/auth.go +++ b/internal/home/auth.go @@ -91,10 +91,7 @@ func InitAuth( } var err error - opts := *bbolt.DefaultOptions - opts.OpenFile = aghos.OpenFile - - a.db, err = bbolt.Open(dbFilename, aghos.DefaultPermFile, &opts) + a.db, err = bbolt.Open(dbFilename, aghos.DefaultPermFile, nil) 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 5242dbc6593..3c5c16f92fb 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -714,7 +714,7 @@ func (c *configuration) write() (err error) { return fmt.Errorf("generating config file: %w", err) } - err = aghos.WriteFile(confPath, buf.Bytes(), aghos.DefaultPermFile) + err = maybe.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 5f5738760b1..8ac6ae225f9 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -645,7 +645,7 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) { } dataDir := Context.getDataDir() - err = aghos.MkdirAll(dataDir, aghos.DefaultPermDir) + err = os.MkdirAll(dataDir, aghos.DefaultPermDir) fatalOnError(errors.Annotate(err, "creating DNS data dir at %s: %w", dataDir)) GLMode = opts.glinetMode @@ -689,7 +689,7 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) { } if !opts.noPermCheck { - checkPermissions(Context.workDir, confPath, dataDir, statsDir, querylogDir) + checkPermissions(ctx, slogLogger, Context.workDir, confPath, dataDir, statsDir, querylogDir) } Context.web.start() @@ -751,12 +751,22 @@ func newUpdater( // checkPermissions checks and migrates permissions of the files and directories // used by AdGuard Home, if needed. -func checkPermissions(workDir, confPath, dataDir, statsDir, querylogDir string) { - if permcheck.NeedsMigration(confPath) { - permcheck.Migrate(workDir, dataDir, statsDir, querylogDir, confPath) +func checkPermissions( + ctx context.Context, + baseLogger *slog.Logger, + workDir string, + confPath string, + dataDir string, + statsDir string, + querylogDir string, +) { + l := baseLogger.With(slogutil.KeyPrefix, "permcheck") + + if permcheck.NeedsMigration(ctx, l, workDir, confPath) { + permcheck.Migrate(ctx, l, workDir, dataDir, statsDir, querylogDir, confPath) } - permcheck.Check(workDir, dataDir, statsDir, querylogDir, confPath) + permcheck.Check(ctx, l, workDir, dataDir, statsDir, querylogDir, confPath) } // initUsers initializes context auth module. Clears config users field. diff --git a/internal/next/cmd/signal.go b/internal/next/cmd/signal.go index d6aa7dc54d3..cdff1b448fa 100644 --- a/internal/next/cmd/signal.go +++ b/internal/next/cmd/signal.go @@ -8,12 +8,12 @@ import ( "strconv" "time" - "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/next/configmgr" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/AdguardTeam/golibs/osutil" "github.com/AdguardTeam/golibs/service" + "github.com/google/renameio/v2/maybe" ) // signalHandler processes incoming signals and shuts services down. @@ -84,7 +84,7 @@ func (h *signalHandler) writePID(ctx context.Context) { data := strconv.AppendInt(nil, int64(pid), 10) data = append(data, '\n') - err := aghos.WriteFile(h.pidFile, data, 0o644) + err := maybe.WriteFile(h.pidFile, data, 0o644) if err != nil { h.logger.ErrorContext(ctx, "writing pidfile", slogutil.KeyError, err) diff --git a/internal/next/configmgr/configmgr.go b/internal/next/configmgr/configmgr.go index 1f9bc8de0e0..100680c8aeb 100644 --- a/internal/next/configmgr/configmgr.go +++ b/internal/next/configmgr/configmgr.go @@ -22,6 +22,7 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/AdguardTeam/golibs/timeutil" + "github.com/google/renameio/v2/maybe" "gopkg.in/yaml.v3" ) @@ -203,7 +204,7 @@ func (m *Manager) write(ctx context.Context) (err error) { return fmt.Errorf("encoding: %w", err) } - err = aghos.WriteFile(m.fileName, b, aghos.DefaultPermFile) + err = maybe.WriteFile(m.fileName, b, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing: %w", err) } diff --git a/internal/permcheck/check_unix.go b/internal/permcheck/check_unix.go new file mode 100644 index 00000000000..7e0b97165a4 --- /dev/null +++ b/internal/permcheck/check_unix.go @@ -0,0 +1,43 @@ +//go:build unix + +package permcheck + +import ( + "context" + "log/slog" + + "github.com/AdguardTeam/AdGuardHome/internal/aghos" +) + +// check is the Unix-specific implementation of [Check]. +func check( + ctx context.Context, + l *slog.Logger, + workDir string, + dataDir string, + statsDir string, + querylogDir string, + confFilePath string, +) { + dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile) + + for _, ent := range entities(workDir, dataDir, statsDir, querylogDir, confFilePath) { + if ent.Value { + checkDir(ctx, dirLoggger, ent.Key) + } else { + checkFile(ctx, fileLogger, ent.Key) + } + } +} + +// checkDir checks the permissions of a single directory. The results are +// logged at the appropriate level. +func checkDir(ctx context.Context, l *slog.Logger, dirPath string) { + checkPath(ctx, l, dirPath, aghos.DefaultPermDir) +} + +// checkFile checks the permissions of a single file. The results are logged at +// the appropriate level. +func checkFile(ctx context.Context, l *slog.Logger, filePath string) { + checkPath(ctx, l, filePath, aghos.DefaultPermFile) +} diff --git a/internal/permcheck/check_windows.go b/internal/permcheck/check_windows.go new file mode 100644 index 00000000000..11b10215585 --- /dev/null +++ b/internal/permcheck/check_windows.go @@ -0,0 +1,60 @@ +//go:build windows + +package permcheck + +import ( + "context" + "log/slog" + + "github.com/AdguardTeam/golibs/logutil/slogutil" + "golang.org/x/sys/windows" +) + +// 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) + + dacl, owner, err := getSecurityInfo(workDir) + if err != nil { + l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) + + return + } + + if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) { + l.WarnContext(ctx, "owner is not in administrators group") + } + + err = rangeACEs(dacl, func( + hdr windows.ACE_HEADER, + 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: + // Skip non-allowed ACEs. + case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid): + // Non-administrator ACEs should not have any access rights. + warn = mask > 0 + default: + // Administrators should full control access rights. + warn = mask&fullControlMask != fullControlMask + } + if warn { + l.WarnContext(ctx, "unexpected access control entry", "mask", mask, "sid", sid) + } + + return true + }) + if err != nil { + l.ErrorContext(ctx, "checking access control entries", slogutil.KeyError, err) + } +} diff --git a/internal/permcheck/migrate.go b/internal/permcheck/migrate.go deleted file mode 100644 index 65d704c496a..00000000000 --- a/internal/permcheck/migrate.go +++ /dev/null @@ -1,93 +0,0 @@ -package permcheck - -import ( - "io/fs" - "os" - "path/filepath" - - "github.com/AdguardTeam/AdGuardHome/internal/aghos" - "github.com/AdguardTeam/golibs/errors" - "github.com/AdguardTeam/golibs/log" -) - -// NeedsMigration returns true if AdGuard Home files need permission migration. -// -// TODO(a.garipov): Consider ways to detect this better. -func NeedsMigration(confFilePath string) (ok bool) { - s, err := aghos.Stat(confFilePath) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - // Likely a first run. Don't check. - return false - } - - log.Error("permcheck: checking if files need migration: %s", err) - - // Unexpected error. Try to migrate just in case. - return true - } - - return s.Mode().Perm() != aghos.DefaultPermFile -} - -// 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) { - chmodDir(workDir) - - chmodFile(confFilePath) - - // TODO(a.garipov): Put all paths in one place and remove this duplication. - chmodDir(dataDir) - chmodDir(filepath.Join(dataDir, "filters")) - chmodFile(filepath.Join(dataDir, "sessions.db")) - chmodFile(filepath.Join(dataDir, "leases.json")) - - if dataDir != querylogDir { - chmodDir(querylogDir) - } - chmodFile(filepath.Join(querylogDir, "querylog.json")) - chmodFile(filepath.Join(querylogDir, "querylog.json.1")) - - if dataDir != statsDir { - chmodDir(statsDir) - } - chmodFile(filepath.Join(statsDir, "stats.db")) -} - -// chmodDir changes the permissions of a single directory. The results are -// logged at the appropriate level. -func chmodDir(dirPath string) { - chmodPath(dirPath, typeDir, aghos.DefaultPermDir) -} - -// chmodFile changes the permissions of a single file. The results are logged -// at the appropriate level. -func chmodFile(filePath string) { - chmodPath(filePath, typeFile, aghos.DefaultPermFile) -} - -// 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 := aghos.Chmod(entPath, fm) - if err == nil { - log.Info("permcheck: changed permissions for %s %q", fileType, entPath) - - return - } else if errors.Is(err, os.ErrNotExist) { - log.Debug("permcheck: changing permissions for %s %q: %s", fileType, entPath, err) - - return - } - - log.Error( - "permcheck: SECURITY WARNING: cannot change permissions for %s %q to %#o: %s; "+ - "this can leave your system vulnerable, see "+ - "https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns", - fileType, - entPath, - fm, - err, - ) -} diff --git a/internal/permcheck/migrate_unix.go b/internal/permcheck/migrate_unix.go new file mode 100644 index 00000000000..95e12ee9207 --- /dev/null +++ b/internal/permcheck/migrate_unix.go @@ -0,0 +1,66 @@ +//go:build unix + +package permcheck + +import ( + "context" + "log/slog" + "os" + + "github.com/AdguardTeam/AdGuardHome/internal/aghos" + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/logutil/slogutil" +) + +// needsMigration is a Unix-specific implementation of [NeedsMigration]. +// +// TODO(a.garipov): Consider ways to detect this better. +func needsMigration(ctx context.Context, l *slog.Logger, _, confFilePath string) (ok bool) { + s, err := os.Stat(confFilePath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + // Likely a first run. Don't check. + return false + } + + l.ErrorContext(ctx, "checking a need for permission migration", slogutil.KeyError, err) + + // Unexpected error. Try to migrate just in case. + return true + } + + return s.Mode().Perm() != aghos.DefaultPermFile +} + +// migrate is a Unix-specific implementation of [Migrate]. +func migrate( + ctx context.Context, + l *slog.Logger, + workDir string, + dataDir string, + statsDir string, + querylogDir string, + confFilePath string, +) { + dirLoggger, fileLogger := l.With("type", typeDir), l.With("type", typeFile) + + for _, ent := range entities(workDir, dataDir, statsDir, querylogDir, confFilePath) { + if ent.Value { + chmodDir(ctx, dirLoggger, ent.Key) + } else { + chmodFile(ctx, fileLogger, ent.Key) + } + } +} + +// chmodDir changes the permissions of a single directory. The results are +// logged at the appropriate level. +func chmodDir(ctx context.Context, l *slog.Logger, dirPath string) { + chmodPath(ctx, l, dirPath, aghos.DefaultPermDir) +} + +// chmodFile changes the permissions of a single file. The results are logged +// at the appropriate level. +func chmodFile(ctx context.Context, l *slog.Logger, filePath string) { + chmodPath(ctx, l, filePath, aghos.DefaultPermFile) +} diff --git a/internal/permcheck/migrate_windows.go b/internal/permcheck/migrate_windows.go new file mode 100644 index 00000000000..d83d7b0b83d --- /dev/null +++ b/internal/permcheck/migrate_windows.go @@ -0,0 +1,135 @@ +//go:build windows + +package permcheck + +import ( + "context" + "log/slog" + + "github.com/AdguardTeam/golibs/logutil/slogutil" + "golang.org/x/sys/windows" +) + +// needsMigration is the Windows-specific implementation of [NeedsMigration]. +func needsMigration(ctx context.Context, l *slog.Logger, workDir, _ string) (ok bool) { + l = l.With("type", typeDir, "path", workDir) + + dacl, owner, err := getSecurityInfo(workDir) + if err != nil { + l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) + + return true + } + + if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) { + return true + } + + err = rangeACEs(dacl, func( + hdr windows.ACE_HEADER, + mask windows.ACCESS_MASK, + sid *windows.SID, + ) (cont bool) { + 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. + ok = mask > 0 + default: + // Administrators should have full control. + ok = mask&fullControlMask != fullControlMask + } + + // Stop ranging if the access control entry is unexpected. + return !ok + }) + if err != nil { + l.ErrorContext(ctx, "checking access control entries", slogutil.KeyError, err) + + return true + } + + return ok +} + +// migrate is the Windows-specific implementation of [Migrate]. +// +// It sets the owner to administrators and adds a full control access control +// entry for the account. It also removes all non-administrator access control +// entries, and keeps deny access control entries. For any created or modified +// entry it sets the propagation flags to be inherited by child objects. +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 { + l.ErrorContext(ctx, "getting security info", slogutil.KeyError, err) + + return + } + + admins, err := windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid) + if err != nil { + l.ErrorContext(ctx, "creating administrators sid", slogutil.KeyError, err) + + return + } + + // TODO(e.burkov): Check for duplicates? + var accessEntries []windows.EXPLICIT_ACCESS + var setACL 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, + sid *windows.SID, + ) (cont bool) { + switch { + case hdr.AceType != windows.ACCESS_ALLOWED_ACE_TYPE: + // 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)) + setACL = true + case !sid.IsWellKnown(windows.WinBuiltinAdministratorsSid): + // Remove non-administrator ACEs, since such accounts should not + // have any access rights. + l.InfoContext(ctx, "removing access control entry", "sid", sid) + setACL = true + default: + // 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) + setACL = setACL || mask&fullControlMask != fullControlMask + } + + return true + }) + if err != nil { + l.ErrorContext(ctx, "ranging through access control entries", slogutil.KeyError, err) + + return + } + + if setACL { + accessEntries = append(accessEntries, newFullExplicitAccess(admins)) + } + + if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) { + l.InfoContext(ctx, "migrating owner", "sid", owner) + owner = admins + } else { + l.DebugContext(ctx, "owner is already an administrator") + owner = nil + } + + err = setSecurityInfo(workDir, owner, accessEntries) + if err != nil { + l.ErrorContext(ctx, "setting security info", slogutil.KeyError, err) + } +} diff --git a/internal/permcheck/permcheck.go b/internal/permcheck/permcheck.go index ace62a5eb00..0f19a09a34c 100644 --- a/internal/permcheck/permcheck.go +++ b/internal/permcheck/permcheck.go @@ -1,17 +1,10 @@ // Package permcheck contains code for simplifying permissions checks on files // and directories. -// -// TODO(a.garipov): Improve the approach on Windows. package permcheck import ( - "io/fs" - "os" - "path/filepath" - - "github.com/AdguardTeam/AdGuardHome/internal/aghos" - "github.com/AdguardTeam/golibs/errors" - "github.com/AdguardTeam/golibs/log" + "context" + "log/slog" ) // File type constants for logging. @@ -22,65 +15,33 @@ const ( // Check checks the permissions on important files. It logs the results at // appropriate levels. -func Check(workDir, dataDir, statsDir, querylogDir, confFilePath string) { - checkDir(workDir) - - checkFile(confFilePath) - - // TODO(a.garipov): Put all paths in one place and remove this duplication. - checkDir(dataDir) - checkDir(filepath.Join(dataDir, "filters")) - checkFile(filepath.Join(dataDir, "sessions.db")) - checkFile(filepath.Join(dataDir, "leases.json")) - - if dataDir != querylogDir { - checkDir(querylogDir) - } - checkFile(filepath.Join(querylogDir, "querylog.json")) - checkFile(filepath.Join(querylogDir, "querylog.json.1")) - - if dataDir != statsDir { - checkDir(statsDir) - } - checkFile(filepath.Join(statsDir, "stats.db")) +func Check( + ctx context.Context, + l *slog.Logger, + workDir string, + dataDir string, + statsDir string, + querylogDir string, + confFilePath string, +) { + check(ctx, l, workDir, dataDir, statsDir, querylogDir, confFilePath) } -// checkDir checks the permissions of a single directory. The results are -// logged at the appropriate level. -func checkDir(dirPath string) { - checkPath(dirPath, typeDir, aghos.DefaultPermDir) +// NeedsMigration returns true if AdGuard Home files need permission migration. +func NeedsMigration(ctx context.Context, l *slog.Logger, workDir, confFilePath string) (ok bool) { + return needsMigration(ctx, l, workDir, confFilePath) } -// checkFile checks the permissions of a single file. The results are logged at -// the appropriate level. -func checkFile(filePath string) { - checkPath(filePath, typeFile, aghos.DefaultPermFile) -} - -// 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 := aghos.Stat(entPath) - if err != nil { - logFunc := log.Error - if errors.Is(err, os.ErrNotExist) { - logFunc = log.Debug - } - - logFunc("permcheck: checking %s %q: %s", fileType, entPath, err) - - return - } - - // TODO(a.garipov): Add a more fine-grained check and result reporting. - perm := s.Mode().Perm() - if perm != want { - log.Info( - "permcheck: SECURITY WARNING: %s %q has unexpected permissions %#o; want %#o", - fileType, - entPath, - perm, - want, - ) - } +// Migrate attempts to change the permissions of AdGuard Home's files. It logs +// the results at an appropriate level. +func Migrate( + ctx context.Context, + l *slog.Logger, + workDir string, + dataDir string, + statsDir string, + querylogDir string, + confFilePath string, +) { + migrate(ctx, l, workDir, dataDir, statsDir, querylogDir, confFilePath) } diff --git a/internal/permcheck/security_unix.go b/internal/permcheck/security_unix.go new file mode 100644 index 00000000000..76a36150356 --- /dev/null +++ b/internal/permcheck/security_unix.go @@ -0,0 +1,123 @@ +//go:build unix + +package permcheck + +import ( + "context" + "fmt" + "io/fs" + "log/slog" + "os" + "path/filepath" + + "github.com/AdguardTeam/golibs/container" + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/logutil/slogutil" +) + +// entity is a filesystem entity with a path and a flag indicating whether it is +// a directory. +type entity = container.KeyValue[string, bool] + +// entities returns a list of filesystem entities that need to be ranged over. +// +// TODO(a.garipov): Put all paths in one place and remove this duplication. +func entities(workDir, dataDir, statsDir, querylogDir, confFilePath string) (ents []entity) { + ents = []entity{{ + Key: workDir, + Value: true, + }, { + Key: confFilePath, + Value: false, + }, { + Key: dataDir, + Value: true, + }, { + Key: filepath.Join(dataDir, "filters"), + Value: true, + }, { + Key: filepath.Join(dataDir, "sessions.db"), + Value: false, + }, { + Key: filepath.Join(dataDir, "leases.json"), + Value: false, + }} + + if dataDir != querylogDir { + ents = append(ents, entity{ + Key: querylogDir, + Value: true, + }) + } + ents = append(ents, entity{ + Key: filepath.Join(querylogDir, "querylog.json"), + Value: false, + }, entity{ + Key: filepath.Join(querylogDir, "querylog.json.1"), + Value: false, + }) + + if dataDir != statsDir { + ents = append(ents, entity{ + Key: statsDir, + Value: true, + }) + } + ents = append(ents, entity{ + Key: filepath.Join(statsDir, "stats.db"), + }) + + return ents +} + +// checkPath checks the permissions of a single filesystem entity. The results +// are logged at the appropriate level. +func checkPath(ctx context.Context, l *slog.Logger, entPath string, want fs.FileMode) { + l = l.With("path", entPath) + + s, err := os.Stat(entPath) + if err != nil { + lvl := slog.LevelError + if errors.Is(err, os.ErrNotExist) { + lvl = slog.LevelDebug + } + + l.Log(ctx, lvl, "checking permissions", slogutil.KeyError, err) + + return + } + + // TODO(a.garipov): Add a more fine-grained check and result reporting. + perm := s.Mode().Perm() + if perm == want { + return + } + + permOct, wantOct := fmt.Sprintf("%#o", perm), fmt.Sprintf("%#o", want) + l.WarnContext(ctx, "found unexpected permissions", "perm", permOct, "want", wantOct) +} + +// chmodPath changes the permissions of a single filesystem entity. The results +// are logged at the appropriate level. +func chmodPath(ctx context.Context, l *slog.Logger, entPath string, fm fs.FileMode) { + var lvl slog.Level + var msg string + args := []any{"path", entPath} + + switch err := os.Chmod(entPath, fm); { + case err == nil: + lvl = slog.LevelInfo + msg = "changed permissions" + case errors.Is(err, os.ErrNotExist): + lvl = slog.LevelDebug + msg = "checking permissions" + args = append(args, slogutil.KeyError, err) + default: + lvl = slog.LevelError + 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) + } + + l.Log(ctx, lvl, msg, args...) +} diff --git a/internal/permcheck/security_windows.go b/internal/permcheck/security_windows.go new file mode 100644 index 00000000000..39ee2a9c9e2 --- /dev/null +++ b/internal/permcheck/security_windows.go @@ -0,0 +1,167 @@ +//go:build windows + +package permcheck + +import ( + "fmt" + "unsafe" + + "github.com/AdguardTeam/golibs/errors" + "golang.org/x/sys/windows" +) + +// objectType is the type of the object for directories in context of security +// API. +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. +// +// See https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/access-mask. +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 | + windows.FILE_WRITE_DATA | + windows.FILE_APPEND_DATA | + windows.FILE_READ_EA | + windows.FILE_WRITE_EA | + windows.FILE_TRAVERSE | + fileDeleteChildRight | + windows.FILE_READ_ATTRIBUTES | + windows.FILE_WRITE_ATTRIBUTES | + windows.DELETE | + windows.READ_CONTROL | + windows.WRITE_DAC | + windows.WRITE_OWNER | + windows.SYNCHRONIZE + +// aceFunc is a function that handles access control entries in the +// discretionary access control list. It should return true to continue +// iterating over the entries, or false to stop. +type aceFunc = func( + hdr windows.ACE_HEADER, + mask windows.ACCESS_MASK, + sid *windows.SID, +) (cont bool) + +// rangeACEs ranges over the access control entries in the discretionary access +// control list of the specified security descriptor and calls f for each one. +func rangeACEs(dacl *windows.ACL, f aceFunc) (err error) { + var errs []error + for i := range uint32(dacl.AceCount) { + var ace *windows.ACCESS_ALLOWED_ACE + err = windows.GetAce(dacl, i, &ace) + if err != nil { + errs = append(errs, fmt.Errorf("getting entry at index %d: %w", i, err)) + + continue + } + + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + if !f(ace.Header, ace.Mask, sid) { + break + } + } + + if err = errors.Join(errs...); err != nil { + return fmt.Errorf("checking access control entries: %w", err) + } + + return nil +} + +// setSecurityInfo sets the security information on the specified file, using +// ents to create a discretionary access control list. Either owner or ents can +// be nil, in which case the corresponding information is not set, but at least +// one of them should be specified. +func setSecurityInfo(fname string, owner *windows.SID, ents []windows.EXPLICIT_ACCESS) (err error) { + var secInfo windows.SECURITY_INFORMATION + + var acl *windows.ACL + 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 + + acl, err = windows.ACLFromEntries(ents, nil) + if err != nil { + return fmt.Errorf("creating access control list: %w", err) + } + } + + if owner != nil { + secInfo |= windows.OWNER_SECURITY_INFORMATION + } + + if secInfo == 0 { + return errors.Error("no security information to set") + } + + err = windows.SetNamedSecurityInfo(fname, objectType, secInfo, owner, nil, acl, nil) + if err != nil { + return fmt.Errorf("setting security info: %w", err) + } + + return nil +} + +// getSecurityInfo retrieves the security information for the specified file. +func getSecurityInfo(fname string) (dacl *windows.ACL, owner *windows.SID, err error) { + // 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 + + sd, err := windows.GetNamedSecurityInfo(fname, objectType, desiredSecInfo) + if err != nil { + return nil, nil, fmt.Errorf("getting security descriptor: %w", err) + } + + owner, _, err = sd.Owner() + if err != nil { + return nil, nil, fmt.Errorf("getting owner sid: %w", err) + } + + dacl, _, err = sd.DACL() + if err != nil { + return nil, nil, fmt.Errorf("getting discretionary access control list: %w", err) + } + + return dacl, owner, nil +} + +// newFullExplicitAccess creates a new explicit access entry with full control +// permissions. +func newFullExplicitAccess(sid *windows.SID) (accEnt windows.EXPLICIT_ACCESS) { + return windows.EXPLICIT_ACCESS{ + AccessPermissions: fullControlMask, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_UNKNOWN, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + } +} + +// newDenyExplicitAccess creates a new explicit access entry with specified deny +// permissions. +func newDenyExplicitAccess( + sid *windows.SID, + mask windows.ACCESS_MASK, +) (accEnt windows.EXPLICIT_ACCESS) { + return windows.EXPLICIT_ACCESS{ + AccessPermissions: mask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_UNKNOWN, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + } +} diff --git a/internal/querylog/qlogfile.go b/internal/querylog/qlogfile.go index 67b1eeb3279..9bc738c6388 100644 --- a/internal/querylog/qlogfile.go +++ b/internal/querylog/qlogfile.go @@ -59,7 +59,6 @@ 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 84da97cf379..bca46c8ca08 100644 --- a/internal/querylog/querylogfile.go +++ b/internal/querylog/querylogfile.go @@ -83,7 +83,7 @@ func (l *queryLog) flushToFile(ctx context.Context, b *bytes.Buffer) (err error) filename := l.logFile - f, err := aghos.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, aghos.DefaultPermFile) + f, err := os.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 38affdb9612..56bb8d46b13 100644 --- a/internal/stats/stats.go +++ b/internal/stats/stats.go @@ -385,12 +385,7 @@ func (s *StatsCtx) openDB() (err error) { var db *bbolt.DB - 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) + db, err = bbolt.Open(s.filename, aghos.DefaultPermFile, nil) 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 fe1e772719b..50869f35186 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -270,7 +270,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") - _ = aghos.Mkdir(u.backupDir, aghos.DefaultPermDir) + _ = os.Mkdir(u.backupDir, aghos.DefaultPermDir) if !firstRun { err = copyFile(u.confName, filepath.Join(u.backupDir, "AdGuardHome.yaml"), aghos.DefaultPermFile) if err != nil { @@ -344,10 +344,10 @@ func (u *Updater) downloadPackageFile() (err error) { return fmt.Errorf("io.ReadAll() failed: %w", err) } - _ = aghos.Mkdir(u.updateDir, aghos.DefaultPermDir) + _ = os.Mkdir(u.updateDir, aghos.DefaultPermDir) log.Debug("updater: saving package to file") - err = aghos.WriteFile(u.packageName, body, aghos.DefaultPermFile) + err = os.WriteFile(u.packageName, body, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing package file: %w", err) } @@ -360,7 +360,7 @@ func tarGzFileUnpackOne(outDir string, tr *tar.Reader, hdr *tar.Header) (name st return "", nil } - outputName := filepath.Join(outDir, name) + outName := filepath.Join(outDir, name) if hdr.Typeflag == tar.TypeDir { if name == "AdGuardHome" { @@ -372,12 +372,12 @@ func tarGzFileUnpackOne(outDir string, tr *tar.Reader, hdr *tar.Header) (name st return "", nil } - err = aghos.Mkdir(outputName, os.FileMode(hdr.Mode&0o755)) + err = os.Mkdir(outName, os.FileMode(hdr.Mode&0o755)) if err != nil && !errors.Is(err, os.ErrExist) { - return "", fmt.Errorf("creating directory %q: %w", outputName, err) + return "", fmt.Errorf("creating directory %q: %w", outName, err) } - log.Debug("updater: created directory %q", outputName) + log.Debug("updater: created directory %q", outName) return "", nil } @@ -389,13 +389,9 @@ func tarGzFileUnpackOne(outDir string, tr *tar.Reader, hdr *tar.Header) (name st } var wc io.WriteCloser - wc, err = aghos.OpenFile( - outputName, - os.O_WRONLY|os.O_CREATE|os.O_TRUNC, - os.FileMode(hdr.Mode&0o755), - ) + wc, err = os.OpenFile(outName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(hdr.Mode)&0o755) if err != nil { - return "", fmt.Errorf("os.OpenFile(%s): %w", outputName, err) + return "", fmt.Errorf("os.OpenFile(%s): %w", outName, err) } defer func() { err = errors.WithDeferred(err, wc.Close()) }() @@ -404,7 +400,7 @@ func tarGzFileUnpackOne(outDir string, tr *tar.Reader, hdr *tar.Header) (name st return "", fmt.Errorf("io.Copy(): %w", err) } - log.Debug("updater: created file %q", outputName) + log.Debug("updater: created file %q", outName) return name, nil } @@ -474,7 +470,7 @@ func zipFileUnpackOne(outDir string, zf *zip.File) (name string, err error) { return "", nil } - err = aghos.Mkdir(outputName, fi.Mode()) + err = os.Mkdir(outputName, fi.Mode()) if err != nil && !errors.Is(err, os.ErrExist) { return "", fmt.Errorf("creating directory %q: %w", outputName, err) } @@ -485,7 +481,7 @@ func zipFileUnpackOne(outDir string, zf *zip.File) (name string, err error) { } var wc io.WriteCloser - wc, err = aghos.OpenFile(outputName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fi.Mode()) + wc, err = os.OpenFile(outputName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fi.Mode()) if err != nil { return "", fmt.Errorf("os.OpenFile(): %w", err) } @@ -535,7 +531,7 @@ func copyFile(src, dst string, perm fs.FileMode) (err error) { return err } - err = aghos.WriteFile(dst, d, perm) + err = os.WriteFile(dst, d, perm) if err != nil { // Don't wrap the error, since it's informative enough as is. return err diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index 7124d9fb99b..2eb60a2d84d 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -62,8 +62,8 @@ set -f -u # NOTE: Flag -H for grep is non-POSIX but all of Busybox, GNU, macOS, and # OpenBSD support it. # -# NOTE: Exclude the permission_windows.go, because it requires unsafe for the -# OS APIs. +# NOTE: Exclude the security_windows.go, because it requires unsafe for the OS +# APIs. # # TODO(a.garipov): Add golibs/log. blocklist_imports() { @@ -72,7 +72,7 @@ blocklist_imports() { -name '*.go' \ '!' '(' \ -name '*.pb.go' \ - -o -path './internal/aghos/permission_windows.go' \ + -o -path './internal/permcheck/security_windows.go' \ ')' \ -exec \ 'grep' \ From c234e5dc319f8b35fb9d05a10e0c33bce74fe857 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 4 Dec 2024 17:57:37 +0300 Subject: [PATCH 3/3] Pull request 2317: Update all Squashed commit of the following: commit 832a5ac0c9d588428c7d030978769e510e011645 Author: Eugene Burkov Date: Wed Dec 4 17:38:20 2024 +0300 client: upd i18n commit 6fa7591109085b169b79bd771808922a1a53f875 Author: Eugene Burkov Date: Wed Dec 4 17:20:30 2024 +0300 client: upd trackers commit f4b692d37c1cc4d784b4a76f85198f4a1cfa7f83 Author: Eugene Burkov Date: Wed Dec 4 16:55:40 2024 +0300 all: upd tools, go --- .github/workflows/build.yml | 2 +- .github/workflows/lint.yml | 2 +- CHANGELOG.md | 6 +- Makefile | 2 +- bamboo-specs/release.yaml | 6 +- bamboo-specs/test.yaml | 4 +- client/src/__locales/fi.json | 2 +- client/src/__locales/si-lk.json | 21 ++-- client/src/__locales/sv.json | 3 +- client/src/helpers/trackers/trackers.json | 19 +++- go.mod | 20 ++-- go.sum | 32 +++--- internal/dhcpd/v4_unix.go | 4 +- internal/tools/go.mod | 60 +++++------ internal/tools/go.sum | 116 +++++++++++----------- 15 files changed, 161 insertions(+), 138 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fc901591f9a..ab3f5cce4a4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,7 +1,7 @@ 'name': 'build' 'env': - 'GO_VERSION': '1.23.3' + 'GO_VERSION': '1.23.4' 'NODE_VERSION': '16' 'on': diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 3d24f103ae6..d0fb2647187 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -1,7 +1,7 @@ 'name': 'lint' 'env': - 'GO_VERSION': '1.23.3' + 'GO_VERSION': '1.23.4' 'on': 'push': diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc76f37015..43d05809065 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,10 +29,8 @@ NOTE: Add new changes BELOW THIS COMMENT. - 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]. - + vulnerabilities fixed in [1.23.4][go-1.23.4]. - The release executables are now signed. ### Added @@ -51,7 +49,7 @@ NOTE: Add new changes BELOW THIS COMMENT. [#7357]: https://github.com/AdguardTeam/AdGuardHome/issues/7357 [#7400]: https://github.com/AdguardTeam/AdGuardHome/issues/7400 -[go-1.23.3]: https://groups.google.com/g/golang-announce/c/X5KodEJYuqI +[go-1.23.4]: https://groups.google.com/g/golang-announce/c/3DyiMkYx4Fo