From a2309f812ad3fd83d26c373b67756ea3074f4854 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Fri, 25 Oct 2024 17:05:08 +0300 Subject: [PATCH] all: fix chlog, imp api --- CHANGELOG.md | 5 ++++- internal/aghos/permission.go | 11 ++++++++++- internal/aghos/permission_unix.go | 7 +++++++ internal/aghos/permission_windows.go | 26 +++++++++++++++++++++++--- internal/querylog/qlogfile.go | 1 + internal/querylog/querylogfile.go | 2 +- internal/stats/stats.go | 8 +++++++- 7 files changed, 53 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e78e5abf..1a5961b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,13 +25,16 @@ 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]). ### Fixed -- Incorrect handling of sensitive files permissions on Windows ([#7314]). - Repetitive statistics log messages ([#7338]). - Custom client cache ([#7250]). - Missing runtime clients with information from the system hosts file on first diff --git a/internal/aghos/permission.go b/internal/aghos/permission.go index 219633e6..d98a1646 100644 --- a/internal/aghos/permission.go +++ b/internal/aghos/permission.go @@ -1,6 +1,9 @@ package aghos -import "io/fs" +import ( + "io/fs" + "os" +) // TODO(e.burkov): Add platform-independent tests. @@ -28,6 +31,12 @@ 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. func Stat(name string) (fi fs.FileInfo, err error) { diff --git a/internal/aghos/permission_unix.go b/internal/aghos/permission_unix.go index 378397d0..2afeb5e8 100644 --- a/internal/aghos/permission_unix.go +++ b/internal/aghos/permission_unix.go @@ -27,6 +27,13 @@ func writeFile(filename string, data []byte, perm fs.FileMode) (err error) { return os.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 index 1a234569..6154dfdb 100644 --- a/internal/aghos/permission_windows.go +++ b/internal/aghos/permission_windows.go @@ -180,7 +180,7 @@ func mkdirAll(path string, perm os.FileMode) (err error) { parent, _ := filepath.Split(path) err = os.MkdirAll(parent, perm) - if err != nil { + if err != nil && !errors.Is(err, os.ErrExist) { return fmt.Errorf("creating parent directories: %w", err) } @@ -197,6 +197,26 @@ func writeFile(filename string, data []byte, perm os.FileMode) (err error) { return chmod(filename, perm) } +// 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) @@ -213,8 +233,8 @@ func newWellKnownTrustee(stype windows.WELL_KNOWN_SID_TYPE) (t *windows.TRUSTEE, // Constants reflecting the UNIX permission bits. const ( ownerWrite = 0b010_000_000 - groupWrite = 0b000_100_000 - worldWrite = 0b000_000_100 + groupWrite = 0b000_010_000 + worldWrite = 0b000_000_010 ownerAll = 0b111_000_000 groupAll = 0b000_111_000 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.