From 9829136b8c689bc06f76f827765cff1d1e9687ef Mon Sep 17 00:00:00 2001 From: Vladimir Stoilov Date: Tue, 14 Jan 2025 18:13:31 +0200 Subject: [PATCH] [service] Fix file permission in the updates --- base/database/main.go | 15 +++------------ base/utils/fs.go | 15 ++++++++++----- base/utils/permissions.go | 14 ++------------ base/utils/permissions_windows.go | 23 ++++++----------------- base/utils/structure.go | 25 ++++++++++++++----------- service/netquery/database.go | 15 ++++----------- service/profile/module.go | 19 +++++++------------ service/ui/module.go | 2 +- service/updates/downloader.go | 9 +++++---- service/updates/index.go | 18 +++++++----------- service/updates/upgrade.go | 24 +++++++++--------------- 11 files changed, 68 insertions(+), 111 deletions(-) diff --git a/base/database/main.go b/base/database/main.go index 91cf02da..e11a73f8 100644 --- a/base/database/main.go +++ b/base/database/main.go @@ -3,7 +3,6 @@ package database import ( "errors" "fmt" - "os" "path/filepath" "github.com/safing/portmaster/base/utils" @@ -24,15 +23,11 @@ func Initialize(databasesRootDir string) error { if initialized.SetToIf(false, true) { rootDir = databasesRootDir - err := os.MkdirAll(rootDir, 0o0700) + // ensure root and databases dirs + err := utils.EnsureDirectory(rootDir, utils.AdminOnlyExecPermission) if err != nil { return fmt.Errorf("failed to create/check database dir %q: %w", rootDir, err) } - // ensure root and databases dirs - err = utils.EnsureDirectory(rootDir, utils.AdminOnlyPermission) - if err != nil { - return fmt.Errorf("could not set permissions to database directory (%s): %w", rootDir, err) - } return nil } @@ -64,13 +59,9 @@ func getLocation(name, storageType string) (string, error) { location := filepath.Join(rootDir, name, storageType) // Make sure location exists. - err := os.MkdirAll(location, 0o0700) + err := utils.EnsureDirectory(location, utils.AdminOnlyExecPermission) if err != nil { return "", fmt.Errorf("failed to create/check database dir %q: %w", location, err) } - err = utils.EnsureDirectory(location, utils.AdminOnlyPermission) - if err != nil { - return "", fmt.Errorf("could not set permissions to directory (%s): %w", location, err) - } return location, nil } diff --git a/base/utils/fs.go b/base/utils/fs.go index d32715af..edb42108 100644 --- a/base/utils/fs.go +++ b/base/utils/fs.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io/fs" + "log/slog" "os" "runtime" ) @@ -13,6 +14,10 @@ const isWindows = runtime.GOOS == "windows" // EnsureDirectory ensures that the given directory exists and that is has the given permissions set. // If path is a file, it is deleted and a directory created. func EnsureDirectory(path string, perm FSPermission) error { + if !perm.IsExecPermission() { + slog.Warn("utils: setting not executable permission for directory", "dir", path) + } + // open path f, err := os.Stat(path) if err == nil { @@ -21,10 +26,10 @@ func EnsureDirectory(path string, perm FSPermission) error { // directory exists, check permissions if isWindows { // Ignore windows permission error. For none admin users it will always fail. - _ = SetDirPermission(path, perm) + _ = SetFilePermission(path, perm) return nil - } else if f.Mode().Perm() != perm.AsUnixDirExecPermission() { - return SetDirPermission(path, perm) + } else if f.Mode().Perm() != perm.AsUnixPermission() { + return SetFilePermission(path, perm) } return nil } @@ -35,12 +40,12 @@ func EnsureDirectory(path string, perm FSPermission) error { } // file does not exist (or has been deleted) if err == nil || errors.Is(err, fs.ErrNotExist) { - err = os.MkdirAll(path, perm.AsUnixDirExecPermission()) + err = os.MkdirAll(path, perm.AsUnixPermission()) if err != nil { return fmt.Errorf("could not create dir %s: %w", path, err) } // Set permissions. - err = SetDirPermission(path, perm) + err = SetFilePermission(path, perm) // Ignore windows permission error. For none admin users it will always fail. if !isWindows { return err diff --git a/base/utils/permissions.go b/base/utils/permissions.go index d7690724..328ac884 100644 --- a/base/utils/permissions.go +++ b/base/utils/permissions.go @@ -4,17 +4,7 @@ package utils import "os" -// SetDirPermission sets the permission of a directory. -func SetDirPermission(path string, perm FSPermission) error { - return os.Chmod(path, perm.AsUnixDirExecPermission()) -} - -// SetExecPermission sets the permission of an executable file. -func SetExecPermission(path string, perm FSPermission) error { - return SetDirPermission(path, perm) -} - -// SetFilePermission sets the permission of a non executable file. +// SetFilePermission sets the permission of a file or directory. func SetFilePermission(path string, perm FSPermission) error { - return os.Chmod(path, perm.AsUnixFilePermission()) + return os.Chmod(path, perm.AsUnixPermission()) } diff --git a/base/utils/permissions_windows.go b/base/utils/permissions_windows.go index 13cbf583..8a65016d 100644 --- a/base/utils/permissions_windows.go +++ b/base/utils/permissions_windows.go @@ -31,22 +31,10 @@ func init() { } } -// SetDirPermission sets the permission of a directory. -func SetDirPermission(path string, perm FSPermission) error { - SetFilePermission(path, perm) - return nil -} - -// SetExecPermission sets the permission of an executable file. -func SetExecPermission(path string, perm FSPermission) error { - SetFilePermission(path, perm) - return nil -} - -// SetFilePermission sets the permission of a non executable file. -func SetFilePermission(path string, perm FSPermission) { +// SetFilePermission sets the permission of a file or directory. +func SetFilePermission(path string, perm FSPermission) error { switch perm { - case AdminOnlyPermission: + case AdminOnlyPermission, AdminOnlyExecPermission: // Set only admin rights, remove all others. acl.Apply( path, @@ -55,7 +43,7 @@ func SetFilePermission(path string, perm FSPermission) { acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, systemSID), acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, adminsSID), ) - case PublicReadPermission: + case PublicReadPermission, PublicReadExecPermission: // Set admin rights and read/execute rights for users, remove all others. acl.Apply( path, @@ -65,7 +53,7 @@ func SetFilePermission(path string, perm FSPermission) { acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, adminsSID), acl.GrantSid(windows.GENERIC_READ|windows.GENERIC_EXECUTE, usersSID), ) - case PublicWritePermission: + case PublicWritePermission, PublicWriteExecPermission: // Set full control to admin and regular users. Guest users will not have access. acl.Apply( path, @@ -76,4 +64,5 @@ func SetFilePermission(path string, perm FSPermission) { acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, usersSID), ) } + return nil } diff --git a/base/utils/structure.go b/base/utils/structure.go index 8e1f0786..3209b27b 100644 --- a/base/utils/structure.go +++ b/base/utils/structure.go @@ -12,36 +12,39 @@ type FSPermission uint8 const ( AdminOnlyPermission FSPermission = iota + AdminOnlyExecPermission PublicReadPermission + PublicReadExecPermission PublicWritePermission + PublicWriteExecPermission ) // AsUnixDirExecPermission return the corresponding unix permission for a directory or executable. -func (perm FSPermission) AsUnixDirExecPermission() fs.FileMode { +func (perm FSPermission) AsUnixPermission() fs.FileMode { switch perm { case AdminOnlyPermission: + return 0o600 + case AdminOnlyExecPermission: return 0o700 case PublicReadPermission: + return 0o644 + case PublicReadExecPermission: return 0o755 case PublicWritePermission: + return 0o666 + case PublicWriteExecPermission: return 0o777 } return 0 } -// AsUnixFilePermission return the corresponding unix permission for a regular file. -func (perm FSPermission) AsUnixFilePermission() fs.FileMode { +func (perm FSPermission) IsExecPermission() bool { switch perm { - case AdminOnlyPermission: - return 0o600 - case PublicReadPermission: - return 0o644 - case PublicWritePermission: - return 0o666 + case AdminOnlyExecPermission, PublicReadExecPermission, PublicWriteExecPermission: + return true } - - return 0 + return false } // DirStructure represents a directory structure with permissions that should be enforced. diff --git a/service/netquery/database.go b/service/netquery/database.go index 4bf28b0b..81eedf2d 100644 --- a/service/netquery/database.go +++ b/service/netquery/database.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io" - "os" "path/filepath" "sort" "strings" @@ -129,12 +128,9 @@ type ( // handed over to SQLite. func New(dbPath string) (*Database, error) { historyParentDir := filepath.Join(module.instance.DataDir(), "databases") - if err := os.MkdirAll(historyParentDir, 0o0700); err != nil { - return nil, fmt.Errorf("failed to ensure database directory exists: %w", err) - } - err := utils.EnsureDirectory(historyParentDir, utils.AdminOnlyPermission) + err := utils.EnsureDirectory(historyParentDir, utils.AdminOnlyExecPermission) if err != nil { - return nil, fmt.Errorf("failed to set permission to folder %s: %w", historyParentDir, err) + return nil, fmt.Errorf("failed to ensure database directory exists: %w", err) } // Get file location of history database. @@ -231,12 +227,9 @@ func (db *Database) Close() error { // VacuumHistory rewrites the history database in order to purge deleted records. func VacuumHistory(ctx context.Context) (err error) { historyParentDir := filepath.Join(module.instance.DataDir(), "databases") - if err := os.MkdirAll(historyParentDir, 0o0700); err != nil { - return fmt.Errorf("failed to ensure database directory exists: %w", err) - } - err = utils.EnsureDirectory(historyParentDir, utils.AdminOnlyPermission) + err = utils.EnsureDirectory(historyParentDir, utils.AdminOnlyExecPermission) if err != nil { - return fmt.Errorf("failed to set permission to folder %s: %w", historyParentDir, err) + return fmt.Errorf("failed to ensure database directory exists: %w", err) } // Get file location of history database. diff --git a/service/profile/module.go b/service/profile/module.go index 6124d090..06dffecd 100644 --- a/service/profile/module.go +++ b/service/profile/module.go @@ -3,7 +3,6 @@ package profile import ( "errors" "fmt" - "os" "path/filepath" "sync/atomic" @@ -68,19 +67,15 @@ func prep() error { // Setup icon storage location. databaseDir := filepath.Join(module.instance.DataDir(), "databases") + // Ensure folder existents and permission + err := utils.EnsureDirectory(databaseDir, utils.AdminOnlyExecPermission) + if err != nil { + return fmt.Errorf("failed to ensure directory existence %s: %w", databaseDir, err) + } iconsDir := filepath.Join(databaseDir, "icons") - if err := os.MkdirAll(iconsDir, 0o0700); err != nil { - return fmt.Errorf("failed to create/check icons directory: %w", err) - } - - // Ensure folder permissions - err := utils.EnsureDirectory(databaseDir, utils.AdminOnlyPermission) + err = utils.EnsureDirectory(iconsDir, utils.AdminOnlyExecPermission) if err != nil { - return fmt.Errorf("failed to set permission to folder %s: %w", databaseDir, err) - } - err = utils.EnsureDirectory(iconsDir, utils.AdminOnlyPermission) - if err != nil { - return fmt.Errorf("failed to set permission to folder %s: %w", iconsDir, err) + return fmt.Errorf("failed to ensure directory existence %s: %w", iconsDir, err) } binmeta.ProfileIconStoragePath = iconsDir diff --git a/service/ui/module.go b/service/ui/module.go index dbc6b47e..a058c508 100644 --- a/service/ui/module.go +++ b/service/ui/module.go @@ -37,7 +37,7 @@ func start() error { } // Ensure directory permission - err = utils.EnsureDirectory(execDir, utils.PublicWritePermission) + err = utils.EnsureDirectory(execDir, utils.PublicWriteExecPermission) if err != nil { log.Warningf("ui: failed to set permissions to directory %s: %s", execDir, err) } diff --git a/service/updates/downloader.go b/service/updates/downloader.go index 27836cac..c87d92d9 100644 --- a/service/updates/downloader.go +++ b/service/updates/downloader.go @@ -16,6 +16,7 @@ import ( "path/filepath" "github.com/safing/portmaster/base/log" + "github.com/safing/portmaster/base/utils" ) type Downloader struct { @@ -37,7 +38,7 @@ func NewDownloader(u *Updater, indexURLs []string) *Downloader { func (d *Downloader) updateIndex(ctx context.Context) error { // Make sure dir exists. - err := os.MkdirAll(d.u.cfg.DownloadDirectory, defaultDirMode) + err := os.MkdirAll(d.u.cfg.DownloadDirectory, utils.PublicReadExecPermission.AsUnixPermission()) if err != nil { return fmt.Errorf("create download directory: %s", d.u.cfg.DownloadDirectory) } @@ -65,7 +66,7 @@ func (d *Downloader) updateIndex(ctx context.Context) error { // Write the index into a file. indexFilepath := filepath.Join(d.u.cfg.DownloadDirectory, d.u.cfg.IndexFile) - err = os.WriteFile(indexFilepath, indexData, defaultFileMode) + err = os.WriteFile(indexFilepath, indexData, utils.PublicReadExecPermission.AsUnixPermission()) if err != nil { return fmt.Errorf("write index file: %w", err) } @@ -130,7 +131,7 @@ func (d *Downloader) gatherExistingFiles(dir string) error { func (d *Downloader) downloadArtifacts(ctx context.Context) error { // Make sure dir exists. - err := os.MkdirAll(d.u.cfg.DownloadDirectory, defaultDirMode) + err := os.MkdirAll(d.u.cfg.DownloadDirectory, utils.PublicReadExecPermission.AsUnixPermission()) if err != nil { return fmt.Errorf("create download directory: %s", d.u.cfg.DownloadDirectory) } @@ -176,7 +177,7 @@ artifacts: // Write artifact to temporary file. tmpFilename := dstFilePath + ".download" - err = os.WriteFile(tmpFilename, artifactData, artifact.GetFileMode()) + err = os.WriteFile(tmpFilename, artifactData, artifact.GetFileMode().AsUnixPermission()) if err != nil { return fmt.Errorf("write %s to temp file: %w", artifact.Filename, err) } diff --git a/service/updates/index.go b/service/updates/index.go index baa76329..cba07c14 100644 --- a/service/updates/index.go +++ b/service/updates/index.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "io" - "io/fs" "os" "path/filepath" "runtime" @@ -18,6 +17,7 @@ import ( "github.com/safing/jess" "github.com/safing/jess/filesig" + "github.com/safing/portmaster/base/utils" ) // MaxUnpackSize defines the maximum size that is allowed to be unpacked. @@ -41,17 +41,12 @@ type Artifact struct { } // GetFileMode returns the required filesystem permission for the artifact. -func (a *Artifact) GetFileMode() os.FileMode { - // Special case for portmaster ui. Should be able to be executed from the regular user - if a.Platform == currentPlatform && a.Filename == "portmaster" { - return executableUIFileMode - } - +func (a *Artifact) GetFileMode() utils.FSPermission { if a.Platform == currentPlatform { - return executableFileMode + return utils.PublicReadExecPermission } - return defaultFileMode + return utils.PublicReadPermission } // Path returns the absolute path to the local file. @@ -338,7 +333,7 @@ func checkSHA256Sum(fileData []byte, sha256sum string) error { // copyAndCheckSHA256Sum copies the file from src to dst and check the sha256 sum. // As a special case, if the sha256sum is not given, it is not checked. -func copyAndCheckSHA256Sum(src, dst, sha256sum string, fileMode fs.FileMode) error { +func copyAndCheckSHA256Sum(src, dst, sha256sum string, filePermission utils.FSPermission) error { // Check expected hash. var expectedDigest []byte if sha256sum != "" { @@ -367,7 +362,7 @@ func copyAndCheckSHA256Sum(src, dst, sha256sum string, fileMode fs.FileMode) err // Write to temporary file. tmpDst := dst + ".copy" - err = os.WriteFile(tmpDst, fileData, fileMode) + err = os.WriteFile(tmpDst, fileData, filePermission.AsUnixPermission()) if err != nil { return fmt.Errorf("write temp dst file: %w", err) } @@ -377,6 +372,7 @@ func copyAndCheckSHA256Sum(src, dst, sha256sum string, fileMode fs.FileMode) err if err != nil { return fmt.Errorf("rename dst file after write: %w", err) } + utils.SetFilePermission(dst, filePermission) return nil } diff --git a/service/updates/upgrade.go b/service/updates/upgrade.go index 1008dcdd..9d18de98 100644 --- a/service/updates/upgrade.go +++ b/service/updates/upgrade.go @@ -3,20 +3,13 @@ package updates import ( "errors" "fmt" - "io/fs" "os" "path/filepath" "slices" "strings" "github.com/safing/portmaster/base/log" -) - -const ( - defaultFileMode = os.FileMode(0o0644) - executableFileMode = os.FileMode(0o0744) - executableUIFileMode = os.FileMode(0o0755) - defaultDirMode = os.FileMode(0o0755) + "github.com/safing/portmaster/base/utils" ) func (u *Updater) upgrade(downloader *Downloader, ignoreVersion bool) error { @@ -55,7 +48,7 @@ func (u *Updater) upgradeMoveFiles(downloader *Downloader) error { // Reset purge directory, so that we can do a clean rollback later. _ = os.RemoveAll(u.cfg.PurgeDirectory) - err := os.MkdirAll(u.cfg.PurgeDirectory, defaultDirMode) + err := utils.EnsureDirectory(u.cfg.PurgeDirectory, utils.PublicReadExecPermission) if err != nil { return fmt.Errorf("failed to create purge directory: %w", err) } @@ -69,7 +62,7 @@ func (u *Updater) upgradeMoveFiles(downloader *Downloader) error { if !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("read current directory: %w", err) } - err = os.MkdirAll(u.cfg.Directory, defaultDirMode) + err := utils.EnsureDirectory(u.cfg.PurgeDirectory, utils.PublicReadExecPermission) if err != nil { return fmt.Errorf("create current directory: %w", err) } @@ -84,7 +77,7 @@ func (u *Updater) upgradeMoveFiles(downloader *Downloader) error { // Otherwise, move file to purge dir. src := filepath.Join(u.cfg.Directory, file.Name()) dst := filepath.Join(u.cfg.PurgeDirectory, file.Name()) - err := u.moveFile(src, dst, "", file.Type().Perm()) + err := u.moveFile(src, dst, "", utils.PublicReadPermission) if err != nil { return fmt.Errorf("failed to move current file %s to purge dir: %w", file.Name(), err) } @@ -95,7 +88,7 @@ func (u *Updater) upgradeMoveFiles(downloader *Downloader) error { log.Debugf("updates/%s: installing the new version (v%s from %s)", u.cfg.Name, downloader.index.Version, downloader.index.Published) src := filepath.Join(u.cfg.DownloadDirectory, u.cfg.IndexFile) dst := filepath.Join(u.cfg.Directory, u.cfg.IndexFile) - err = u.moveFile(src, dst, "", defaultFileMode) + err = u.moveFile(src, dst, "", utils.PublicReadPermission) if err != nil { return fmt.Errorf("failed to move index file to %s: %w", dst, err) } @@ -120,17 +113,18 @@ func (u *Updater) upgradeMoveFiles(downloader *Downloader) error { } // moveFile moves a file and falls back to copying if it fails. -func (u *Updater) moveFile(currentPath, newPath string, sha256sum string, fileMode fs.FileMode) error { +func (u *Updater) moveFile(currentPath, newPath string, sha256sum string, filePermission utils.FSPermission) error { // Try to simply move file. err := os.Rename(currentPath, newPath) if err == nil { // Moving was successful, return. + utils.SetFilePermission(newPath, filePermission) return nil } log.Tracef("updates/%s: failed to move to %q, falling back to copy+delete: %s", u.cfg.Name, newPath, err) // Copy and check the checksum while we are at it. - err = copyAndCheckSHA256Sum(currentPath, newPath, sha256sum, fileMode) + err = copyAndCheckSHA256Sum(currentPath, newPath, sha256sum, filePermission) if err != nil { return fmt.Errorf("move failed, copy+delete fallback failed: %w", err) } @@ -150,7 +144,7 @@ func (u *Updater) recoverFromFailedUpgrade() error { for _, file := range files { purgedFile := filepath.Join(u.cfg.PurgeDirectory, file.Name()) activeFile := filepath.Join(u.cfg.Directory, file.Name()) - err := u.moveFile(purgedFile, activeFile, "", file.Type().Perm()) + err := u.moveFile(purgedFile, activeFile, "", utils.PublicReadPermission) if err != nil { // Only warn and continue to recover as many files as possible. log.Warningf("updates/%s: failed to roll back file %s: %s", u.cfg.Name, file.Name(), err)