Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix windows permissions #1766

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/config/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,9 @@ func InitializeUnitTestDataroot(testName string) (string, error) {
return "", fmt.Errorf("failed to make tmp dir: %w", err)
}

ds := utils.NewDirStructure(basePath, 0o0755)
ds := utils.NewDirStructure(basePath, utils.PublicReadPermission)
SetDataRoot(ds)
err = dataroot.Initialize(basePath, 0o0755)
err = dataroot.Initialize(basePath, utils.PublicReadPermission)
if err != nil {
return "", fmt.Errorf("failed to initialize dataroot: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions base/database/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ var (

// InitializeWithPath initializes the database at the specified location using a path.
func InitializeWithPath(dirPath string) error {
return Initialize(utils.NewDirStructure(dirPath, 0o0755))
return Initialize(utils.NewDirStructure(dirPath, utils.PublicReadPermission))
}

// Initialize initializes the database at the specified location using a dir structure.
Expand All @@ -34,7 +34,7 @@ func Initialize(dirStructureRoot *utils.DirStructure) error {
rootStructure = dirStructureRoot

// ensure root and databases dirs
databasesStructure = rootStructure.ChildDir(databasesSubDir, 0o0700)
databasesStructure = rootStructure.ChildDir(databasesSubDir, utils.AdminOnlyPermission)
err := databasesStructure.Ensure()
if err != nil {
return fmt.Errorf("could not create/open database directory (%s): %w", rootStructure.Path, err)
Expand Down Expand Up @@ -67,7 +67,7 @@ func Shutdown() (err error) {

// getLocation returns the storage location for the given name and type.
func getLocation(name, storageType string) (string, error) {
location := databasesStructure.ChildDir(name, 0o0700).ChildDir(storageType, 0o0700)
location := databasesStructure.ChildDir(name, utils.AdminOnlyPermission).ChildDir(storageType, utils.AdminOnlyPermission)
// check location
err := location.Ensure()
if err != nil {
Expand Down
7 changes: 2 additions & 5 deletions base/database/storage/fstree/fstree.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check failure on line 1 in base/database/storage/fstree/fstree.go

View workflow job for this annotation

GitHub Actions / Linter

: # github.com/safing/portmaster/base/database/storage/fstree [github.com/safing/portmaster/base/database/storage/fstree.test]
Package fstree provides a dead simple file-based database storage backend.
It is primarily meant for easy testing or storing big files that can easily be accesses directly, without datastore.
*/
Expand All @@ -15,7 +15,7 @@
"strings"
"time"

"github.com/hectane/go-acl"

Check failure on line 18 in base/database/storage/fstree/fstree.go

View workflow job for this annotation

GitHub Actions / Linter

"github.com/hectane/go-acl" imported as acl and not used) (typecheck)

Check failure on line 18 in base/database/storage/fstree/fstree.go

View workflow job for this annotation

GitHub Actions / Linter

"github.com/hectane/go-acl" imported as acl and not used (typecheck)
"github.com/safing/portmaster/base/database/iterator"
"github.com/safing/portmaster/base/database/query"
"github.com/safing/portmaster/base/database/record"
Expand Down Expand Up @@ -289,11 +289,8 @@
defer t.Cleanup() //nolint:errcheck

// Set permissions before writing data, in case the data is sensitive.
if onWindows {
err = acl.Chmod(filename, perm)
} else {
err = t.Chmod(perm)
}
// TODO(vladimir): to set permissions on windows we need the full path of the file.
err = t.Chmod(perm)
Comment on lines +291 to +292
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider implementing Windows-specific permission handling.

The TODO comment correctly identifies that Windows requires the full path for setting permissions. Consider implementing this using the Windows-specific APIs from the go-acl package that's already imported.

Here's a suggested implementation:

-// TODO(vladimir): to set permissions on windows we need the full path of the file.
-err = t.Chmod(perm)
+if onWindows {
+    fullPath := t.Name()
+    err = acl.Chmod(fullPath, perm)
+} else {
+    err = t.Chmod(perm)
+}

Committable suggestion skipped: line range outside the PR's diff.

if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions base/dataroot/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@ package dataroot

import (
"errors"
"os"

"github.com/safing/portmaster/base/utils"
)

var root *utils.DirStructure

// Initialize initializes the data root directory.
func Initialize(rootDir string, perm os.FileMode) error {
func Initialize(rootDir string, perm utils.FSPermission) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Update permission value in notifier/main.go to use utils.FSPermission

The verification shows that most callers have been updated to use utils.FSPermission, but there's one remaining instance in cmds/notifier/main.go that still uses a raw octal permission value (0o0755). This needs to be updated for consistency.

  • cmds/notifier/main.go: Replace 0o0755 with appropriate utils.FSPermission constant
🔗 Analysis chain

Verify all callers of Initialize function

The parameter type change from os.FileMode to utils.FSPermission is good for cross-platform compatibility. However, we should verify that all callers have been updated accordingly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining direct calls using os.FileMode
rg -A 2 "Initialize\([^,]+,\s*os\." 

# Search for all calls to Initialize to verify they've been updated
rg -A 2 "dataroot\.Initialize\("

Length of output: 897

if root != nil {
return errors.New("already initialized")
}
Expand Down
17 changes: 5 additions & 12 deletions base/updater/fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import (
"path/filepath"
"time"

"github.com/hectane/go-acl"
"github.com/safing/jess/filesig"
"github.com/safing/jess/lhash"
"github.com/safing/portmaster/base/log"
"github.com/safing/portmaster/base/utils"
"github.com/safing/portmaster/base/utils/renameio"
)

Expand Down Expand Up @@ -137,17 +137,10 @@ func (reg *ResourceRegistry) fetchFile(ctx context.Context, client *http.Client,
return fmt.Errorf("%s: failed to finalize file %s: %w", reg.Name, rv.storagePath(), err)
}
// set permissions
if onWindows {
err = acl.Chmod(rv.storagePath(), 0o0755)
if err != nil {
log.Warningf("%s: failed to set permissions on downloaded file %s: %s", reg.Name, rv.storagePath(), err)
}
} else {
// TODO: only set executable files to 0755, set other to 0644
err = os.Chmod(rv.storagePath(), 0o0755) //nolint:gosec // See TODO above.
if err != nil {
log.Warningf("%s: failed to set permissions on downloaded file %s: %s", reg.Name, rv.storagePath(), err)
}
// TODO: distinguish between executable and non executable files.
err = utils.SetExecPermission(rv.storagePath(), utils.PublicReadPermission)
Copy link
Preview

Copilot AI Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function SetExecPermission is used for all files without distinguishing between executable and non-executable files. This could lead to incorrect permissions being set. Consider using different functions for setting permissions based on the file type.

Suggested change
err = utils.SetExecPermission(rv.storagePath(), utils.PublicReadPermission)
if isExecutable(rv.storagePath()) { err = utils.SetExecPermission(rv.storagePath(), utils.PublicReadPermission) } else { err = os.Chmod(rv.storagePath(), 0o0644) }

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
if err != nil {
log.Warningf("%s: failed to set permissions on downloaded file %s: %s", reg.Name, rv.storagePath(), err)
Comment on lines +140 to +143
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Let me analyze the current findings and generate a final response based on the code search results.

Executable detection should be based on file extension and platform

The codebase already has platform-specific handling for executables, particularly for Windows (.exe). The implementation should leverage this existing pattern:

  • base/utils/permissions.go already has separate functions for executable and non-executable files (SetExecPermission vs SetFilePermission)
  • cmds/portmaster-start/run.go defines platform-specific executable suffix
  • base/profile/binmeta/name.go identifies .exe as Windows Executable

Suggested improvements:

  • Use the existing exeSuffix constant from portmaster-start/run.go or create a shared constant
  • Check file extension against known executable extensions per platform
  • For Windows: check .exe extension
  • For Unix: check file header (shebang) or executable bit of source file
🔗 Analysis chain

Consider implementing TODO for executable detection.

The current implementation sets execution permissions on all files, but the TODO suggests a need for differentiation. This could be important for security and functionality.

Consider implementing the following improvements:

  1. Add a flag in ResourceVersion to indicate if a file should be executable
  2. Use file extension or resource type to automatically determine executability
  3. Add platform-specific logic in utils.SetExecPermission to handle this differentiation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check current file types and their permissions
# Test: Look for patterns that could help identify executable files

# Check file extensions of resources
rg "NewResource\(" -A 3

# Check for existing executable detection logic
rg -g "*.go" "executable|\.exe" 

Length of output: 37839

}

log.Debugf("%s: fetched %s and stored to %s", reg.Name, downloadURL, rv.storagePath())
Expand Down
2 changes: 1 addition & 1 deletion base/updater/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func (reg *ResourceRegistry) Initialize(storageDir *utils.DirStructure) error {

// initialize private attributes
reg.storageDir = storageDir
reg.tmpDir = storageDir.ChildDir("tmp", 0o0700)
reg.tmpDir = storageDir.ChildDir("tmp", utils.AdminOnlyPermission)
reg.resources = make(map[string]*Resource)
if reg.state == nil {
reg.state = &RegistryState{}
Expand Down
19 changes: 8 additions & 11 deletions base/utils/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,13 @@ import (
"io/fs"
"os"
"runtime"

"github.com/hectane/go-acl"
)

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 os.FileMode) error {
func EnsureDirectory(path string, perm FSPermission) error {
// open path
f, err := os.Stat(path)
if err == nil {
Expand All @@ -23,10 +21,10 @@ func EnsureDirectory(path string, perm os.FileMode) error {
// directory exists, check permissions
if isWindows {
// Ignore windows permission error. For none admin users it will always fail.
acl.Chmod(path, perm)
SetDirPermission(path, perm)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add logging for Windows permission failures

Instead of silently ignoring Windows permission errors, log them to help diagnose permission issues during deployment and maintenance.

 if isWindows {
     // Ignore windows permission error. For none admin users it will always fail.
-    SetDirPermission(path, perm)
+    if err := SetDirPermission(path, perm); err != nil {
+        log.Warningf("Failed to set directory permissions on Windows: %v", err)
+    }
     return nil
 }

Committable suggestion skipped: line range outside the PR's diff.

} else if f.Mode().Perm() != perm {
return os.Chmod(path, perm)
} else if f.Mode().Perm() != perm.AsUnixDirExecPermission() {
return SetDirPermission(path, perm)
}
return nil
}
Expand All @@ -37,17 +35,16 @@ func EnsureDirectory(path string, perm os.FileMode) error {
}
// file does not exist (or has been deleted)
if err == nil || errors.Is(err, fs.ErrNotExist) {
err = os.Mkdir(path, perm)
err = os.Mkdir(path, perm.AsUnixDirExecPermission())
if err != nil {
return fmt.Errorf("could not create dir %s: %w", path, err)
}
// Set windows permissions. Linux permission where already set with creation.
if isWindows {
// Ignore windows permission error. For none admin users it will always fail.
acl.Chmod(path, perm)
return nil
} else {
return os.Chmod(path, perm)
SetDirPermission(path, perm)
}
return nil
}
// other error opening path
return fmt.Errorf("failed to access %s: %w", path, err)
Expand Down
20 changes: 20 additions & 0 deletions base/utils/permissions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//go:build !windows

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.
func SetFilePermission(path string, perm FSPermission) error {
return os.Chmod(path, perm.AsUnixFilePermission())
}
35 changes: 35 additions & 0 deletions base/utils/permissions_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//go:build windows

package utils

import (
"github.com/hectane/go-acl"
"golang.org/x/sys/windows"
)

func SetDirPermission(path string, perm FSPermission) error {
setWindowsFilePermissions(path, perm)
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Add error handling for ACL operations

The function ignores all errors from setWindowsFilePermissions. This could silently fail to set permissions, leading to security vulnerabilities.

 func SetDirPermission(path string, perm FSPermission) error {
-    setWindowsFilePermissions(path, perm)
-    return nil
+    return setWindowsFilePermissions(path, perm)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func SetDirPermission(path string, perm FSPermission) error {
setWindowsFilePermissions(path, perm)
return nil
}
func SetDirPermission(path string, perm FSPermission) error {
return setWindowsFilePermissions(path, perm)
}


// SetExecPermission sets the permission of an executable file.
func SetExecPermission(path string, perm FSPermission) error {
return SetDirPermission(path, perm)
}

func setWindowsFilePermissions(path string, perm FSPermission) {
switch perm {
case AdminOnlyPermission:
// Set only admin rights, remove all others.
acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
case PublicReadPermission:
// Set admin rights and read/execute rights for users, remove all others.
acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_EXECUTE, "Users"))
acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_READ, "Users"))
case PublicWritePermission:
// Set full control to admin and regular users. Guest users will not have access.
acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Users"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Based on the context gathered, I can now provide a final response:

Security concern confirmed: PublicWritePermission requires restrictive implementation

The PublicWritePermission is used for temporary execution directories ("exec") and log directories that specifically require write access. However, granting GENERIC_ALL|STANDARD_RIGHTS_ALL is still overly permissive as:

  • The code comments acknowledge the permissions "may seem dangerous"
  • It's used only for temporary execution directories that get cleaned up (cleanAndEnsureExecDir())
  • The Unix equivalent is 0o777/0o666, suggesting only read/write/execute is needed

Recommendations:

  • Replace GENERIC_ALL|STANDARD_RIGHTS_ALL with the minimum required permissions: GENERIC_READ|GENERIC_WRITE|GENERIC_EXECUTE
  • Consider implementing separate permission levels for logs (which may need only append rights) versus exec directories
🔗 Analysis chain

Security concern: Review PublicWritePermission implementation

Granting GENERIC_ALL|STANDARD_RIGHTS_ALL to the Users group gives excessive permissions. Consider restricting to specific required permissions instead of full control.


Let me gather more context about how this permission is used in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of PublicWritePermission to assess impact
rg "PublicWritePermission" --type go

Length of output: 546


Script:

#!/bin/bash
# Check the implementation and usage context of PublicWritePermission
ast-grep --pattern 'const PublicWritePermission = $_'

# Check the content of files using this permission
rg -B 5 -A 5 "PublicWritePermission" --type go

# Check what directories are created with these permissions
fd exec logs -t d -x ls -la {}

Length of output: 4702

}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Improve error handling in setWindowsFilePermissions

  1. The function doesn't handle errors from acl.Apply
  2. Multiple acl.Apply calls could partially succeed, leaving inconsistent permissions
  3. No path validation against directory traversal attacks
-func setWindowsFilePermissions(path string, perm FSPermission) {
+func setWindowsFilePermissions(path string, perm FSPermission) error {
+    // Validate path to prevent directory traversal
+    cleanPath, err := filepath.Abs(filepath.Clean(path))
+    if err != nil {
+        return fmt.Errorf("invalid path: %w", err)
+    }
+
     switch perm {
     case AdminOnlyPermission:
-        acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
+        if err := acl.Apply(cleanPath, true, false,
+            acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators")); err != nil {
+            return fmt.Errorf("failed to set admin-only permissions: %w", err)
+        }
     case PublicReadPermission:
-        acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
-        acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_EXECUTE, "Users"))
-        acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_READ, "Users"))
+        if err := acl.Apply(cleanPath, true, false,
+            acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators")); err != nil {
+            return fmt.Errorf("failed to set admin permissions: %w", err)
+        }
+        if err := acl.Apply(cleanPath, false, false,
+            acl.GrantName(windows.GENERIC_EXECUTE|windows.GENERIC_READ, "Users")); err != nil {
+            return fmt.Errorf("failed to set user permissions: %w", err)
+        }
     case PublicWritePermission:
-        acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
-        acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Users"))
+        if err := acl.Apply(cleanPath, true, false,
+            acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators")); err != nil {
+            return fmt.Errorf("failed to set admin permissions: %w", err)
+        }
+        if err := acl.Apply(cleanPath, false, false,
+            acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Users")); err != nil {
+            return fmt.Errorf("failed to set user permissions: %w", err)
+        }
     }
+    return nil
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func setWindowsFilePermissions(path string, perm FSPermission) {
switch perm {
case AdminOnlyPermission:
// Set only admin rights, remove all others.
acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
case PublicReadPermission:
// Set admin rights and read/execute rights for users, remove all others.
acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_EXECUTE, "Users"))
acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_READ, "Users"))
case PublicWritePermission:
// Set full control to admin and regular users. Guest users will not have access.
acl.Apply(path, true, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators"))
acl.Apply(path, false, false, acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Users"))
}
}
func setWindowsFilePermissions(path string, perm FSPermission) error {
// Validate path to prevent directory traversal
cleanPath, err := filepath.Abs(filepath.Clean(path))
if err != nil {
return fmt.Errorf("invalid path: %w", err)
}
switch perm {
case AdminOnlyPermission:
if err := acl.Apply(cleanPath, true, false,
acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators")); err != nil {
return fmt.Errorf("failed to set admin-only permissions: %w", err)
}
case PublicReadPermission:
if err := acl.Apply(cleanPath, true, false,
acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators")); err != nil {
return fmt.Errorf("failed to set admin permissions: %w", err)
}
if err := acl.Apply(cleanPath, false, false,
acl.GrantName(windows.GENERIC_EXECUTE|windows.GENERIC_READ, "Users")); err != nil {
return fmt.Errorf("failed to set user permissions: %w", err)
}
case PublicWritePermission:
if err := acl.Apply(cleanPath, true, false,
acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Administrators")); err != nil {
return fmt.Errorf("failed to set admin permissions: %w", err)
}
if err := acl.Apply(cleanPath, false, false,
acl.GrantName(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, "Users")); err != nil {
return fmt.Errorf("failed to set user permissions: %w", err)
}
}
return nil
}

44 changes: 40 additions & 4 deletions base/utils/structure.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,61 @@ package utils

import (
"fmt"
"os"
"io/fs"
"path/filepath"
"strings"
"sync"
)

type FSPermission uint8

const (
AdminOnlyPermission FSPermission = iota
PublicReadPermission
PublicWritePermission
)

// AsUnixDirPermission return the corresponding unix permission for a directory or executable.
func (perm FSPermission) AsUnixDirExecPermission() fs.FileMode {
switch perm {
case AdminOnlyPermission:
return 0o700
case PublicReadPermission:
return 0o755
case PublicWritePermission:
return 0o777
}

return 0
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for zero FSPermission value

Both permission conversion methods return 0 for invalid/zero FSPermission values without any error indication. This could lead to security issues if the permission value is not properly initialized.

Consider adding validation:

func (perm FSPermission) AsUnixDirExecPermission() fs.FileMode {
+ if perm > PublicWritePermission {
+   panic("invalid FSPermission value")
+ }
  switch perm {
  // ...
  }
- return 0
}

func (perm FSPermission) AsUnixFilePermission() fs.FileMode {
+ if perm > PublicWritePermission {
+   panic("invalid FSPermission value")
+ }
  switch perm {
  // ...
  }
- return 0
}

Also applies to: 33-45


// AsUnixDirPermission return the corresponding unix permission for a regular file.
func (perm FSPermission) AsUnixFilePermission() fs.FileMode {
switch perm {
case AdminOnlyPermission:
return 0o600
case PublicReadPermission:
return 0o655
case PublicWritePermission:
return 0o666
}

return 0
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect file permission for PublicReadPermission

The file permission for PublicReadPermission is set to 0o655, which is unusual. The standard file permission for public-readable files is 0o644 (rw-r--r--).

Apply this fix:

  case PublicReadPermission:
-   return 0o655
+   return 0o644
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// AsUnixDirPermission return the corresponding unix permission for a regular file.
func (perm FSPermission) AsUnixFilePermission() fs.FileMode {
switch perm {
case AdminOnlyPermission:
return 0o600
case PublicReadPermission:
return 0o655
case PublicWritePermission:
return 0o666
}
return 0
}
// AsUnixDirPermission return the corresponding unix permission for a regular file.
func (perm FSPermission) AsUnixFilePermission() fs.FileMode {
switch perm {
case AdminOnlyPermission:
return 0o600
case PublicReadPermission:
return 0o644
case PublicWritePermission:
return 0o666
}
return 0
}


// DirStructure represents a directory structure with permissions that should be enforced.
type DirStructure struct {
sync.Mutex

Path string
Dir string
Perm os.FileMode
Perm FSPermission
Parent *DirStructure
Children map[string]*DirStructure
}

// NewDirStructure returns a new DirStructure.
func NewDirStructure(path string, perm os.FileMode) *DirStructure {
func NewDirStructure(path string, perm FSPermission) *DirStructure {
return &DirStructure{
Path: path,
Perm: perm,
Expand All @@ -29,7 +65,7 @@ func NewDirStructure(path string, perm os.FileMode) *DirStructure {
}

// ChildDir adds a new child DirStructure and returns it. Should the child already exist, the existing child is returned and the permissions are updated.
func (ds *DirStructure) ChildDir(dirName string, perm os.FileMode) (child *DirStructure) {
func (ds *DirStructure) ChildDir(dirName string, perm FSPermission) (child *DirStructure) {
ds.Lock()
defer ds.Unlock()

Expand Down
3 changes: 2 additions & 1 deletion cmds/portmaster-start/dirs.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"log"
"os"

"github.com/safing/portmaster/base/utils"
"github.com/spf13/cobra"
)

Expand All @@ -24,7 +25,7 @@ var cleanStructureCmd = &cobra.Command{
}

func cleanAndEnsureExecDir() error {
execDir := dataRoot.ChildDir("exec", 0o777)
execDir := dataRoot.ChildDir("exec", utils.PublicWritePermission)

// Clean up and remove exec dir.
err := os.RemoveAll(execDir.Path)
Expand Down
6 changes: 3 additions & 3 deletions cmds/portmaster-start/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,14 @@ func configureRegistry(mustLoadIndex bool) error {
// Remove left over quotes.
dataDir = strings.Trim(dataDir, `\"`)
// Initialize data root.
err := dataroot.Initialize(dataDir, 0o0755)
err := dataroot.Initialize(dataDir, utils.PublicReadPermission)
if err != nil {
return fmt.Errorf("failed to initialize data root: %w", err)
}
dataRoot = dataroot.Root()

// Initialize registry.
err = registry.Initialize(dataRoot.ChildDir("updates", 0o0755))
err = registry.Initialize(dataRoot.ChildDir("updates", utils.PublicReadPermission))
if err != nil {
return err
}
Expand All @@ -196,7 +196,7 @@ func configureRegistry(mustLoadIndex bool) error {

func ensureLoggingDir() error {
// set up logs root
logsRoot = dataRoot.ChildDir("logs", 0o0777)
logsRoot = dataRoot.ChildDir("logs", utils.PublicWritePermission)
err := logsRoot.Ensure()
if err != nil {
return fmt.Errorf("failed to initialize logs root (%q): %w", logsRoot.Path, err)
Expand Down
3 changes: 2 additions & 1 deletion service/core/base/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/safing/portmaster/base/api"
"github.com/safing/portmaster/base/dataroot"
"github.com/safing/portmaster/base/info"
"github.com/safing/portmaster/base/utils"
"github.com/safing/portmaster/service/mgr"
)

Expand Down Expand Up @@ -54,7 +55,7 @@ func prep(instance instance) error {
}

// initialize structure
err := dataroot.Initialize(dataDir, 0o0755)
err := dataroot.Initialize(dataDir, utils.PublicReadPermission)
if err != nil {
return err
}
Expand Down
5 changes: 3 additions & 2 deletions service/netquery/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/safing/portmaster/base/config"
"github.com/safing/portmaster/base/dataroot"
"github.com/safing/portmaster/base/log"
"github.com/safing/portmaster/base/utils"
"github.com/safing/portmaster/service/netquery/orm"
"github.com/safing/portmaster/service/network"
"github.com/safing/portmaster/service/network/netutils"
Expand Down Expand Up @@ -127,7 +128,7 @@ type (
// Note that write connections are serialized by the Database object before being
// handed over to SQLite.
func New(dbPath string) (*Database, error) {
historyParentDir := dataroot.Root().ChildDir("databases", 0o700)
historyParentDir := dataroot.Root().ChildDir("databases", utils.AdminOnlyPermission)
if err := historyParentDir.Ensure(); err != nil {
return nil, fmt.Errorf("failed to ensure database directory exists: %w", err)
}
Expand Down Expand Up @@ -225,7 +226,7 @@ func (db *Database) Close() error {

// VacuumHistory rewrites the history database in order to purge deleted records.
func VacuumHistory(ctx context.Context) (err error) {
historyParentDir := dataroot.Root().ChildDir("databases", 0o700)
historyParentDir := dataroot.Root().ChildDir("databases", utils.AdminOnlyPermission)
if err := historyParentDir.Ensure(); err != nil {
return fmt.Errorf("failed to ensure database directory exists: %w", err)
}
Expand Down
3 changes: 2 additions & 1 deletion service/profile/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/safing/portmaster/base/database/migration"
"github.com/safing/portmaster/base/dataroot"
"github.com/safing/portmaster/base/log"
"github.com/safing/portmaster/base/utils"
_ "github.com/safing/portmaster/service/core/base"
"github.com/safing/portmaster/service/mgr"
"github.com/safing/portmaster/service/profile/binmeta"
Expand Down Expand Up @@ -70,7 +71,7 @@ func prep() error {
}

// Setup icon storage location.
iconsDir := dataroot.Root().ChildDir("databases", 0o0700).ChildDir("icons", 0o0700)
iconsDir := dataroot.Root().ChildDir("databases", utils.AdminOnlyPermission).ChildDir("icons", utils.AdminOnlyPermission)
if err := iconsDir.Ensure(); err != nil {
return fmt.Errorf("failed to create/check icons directory: %w", err)
}
Expand Down
Loading
Loading