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 3 commits
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
8 changes: 2 additions & 6 deletions base/database/storage/fstree/fstree.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"strings"
"time"

"github.com/hectane/go-acl"
"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 +288,8 @@ func writeFile(filename string, data []byte, perm os.FileMode) error {
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
7 changes: 1 addition & 6 deletions base/updater/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,13 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"strings"
"sync"

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

const (
onWindows = runtime.GOOS == "windows"
)

// ResourceRegistry is a registry for managing update resources.
type ResourceRegistry struct {
sync.RWMutex
Expand Down Expand Up @@ -98,7 +93,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
2 changes: 1 addition & 1 deletion base/updater/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func TestMain(m *testing.M) {
DevMode: true,
Online: true,
}
err = registry.Initialize(utils.NewDirStructure(tmpDir, 0o0777))
err = registry.Initialize(utils.NewDirStructure(tmpDir, utils.PublicWritePermission))
if err != nil {
panic(err)
}
Expand Down
24 changes: 11 additions & 13 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
} 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,17 @@ 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)
}
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)
// Set permissions.
err = SetDirPermission(path, perm)
// Ignore windows permission error. For none admin users it will always fail.
if !isWindows {
return err
}
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())
}
77 changes: 77 additions & 0 deletions base/utils/permissions_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
//go:build windows

package utils

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

var (
systemSID *windows.SID
adminsSID *windows.SID
usersSID *windows.SID
)

func init() {
var err error
systemSID, err = windows.StringToSid("S-1-5") // NT Authority / SYSTEM
if err != nil {
panic(err)
}
adminsSID, err = windows.StringToSid("S-1-5-32-544") // Administrators
if err != nil {
panic(err)
}
usersSID, err = windows.StringToSid("S-1-5-32-545") // Users
if err != nil {
panic(err)
}
}
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 and variable scope

Several improvements are needed in the initialization code:

  1. Replace panic with proper error handling
  2. Add validation for SID strings
  3. Make variables package-private since they're only used internally

Consider this safer implementation:

-var (
+var (
-	systemSID *windows.SID
-	adminsSID *windows.SID
-	usersSID  *windows.SID
+	systemSID *windows.SID
+	adminsSID *windows.SID
+	usersSID  *windows.SID
+	initErr   error
)

+// validateSID checks if the provided SID string is valid
+func validateSID(sidStr string) bool {
+	sid, err := windows.StringToSid(sidStr)
+	return err == nil && sid != nil
+}

func init() {
-	var err error
+	// Validate SID strings first
+	if !validateSID("S-1-5") || !validateSID("S-1-5-32-544") || !validateSID("S-1-5-32-545") {
+		initErr = fmt.Errorf("invalid SID string")
+		return
+	}

	systemSID, err = windows.StringToSid("S-1-5") // NT Authority / SYSTEM
	if err != nil {
-		panic(err)
+		initErr = fmt.Errorf("failed to initialize system SID: %w", err)
+		return
	}
	// ... similar changes for other SIDs
}

+// ensureInitialized checks if initialization was successful
+func ensureInitialized() error {
+	if initErr != nil {
+		return fmt.Errorf("windows permissions not properly initialized: %w", initErr)
+	}
+	return nil
+}

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


// 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
}
Comment on lines +34 to +44
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

Refactor duplicate functions

SetDirPermission and SetExecPermission are identical. Consider combining them or implementing specific behavior for each type.

Consider this implementation:

-func SetDirPermission(path string, perm FSPermission) error {
-	SetFilePermission(path, perm)
-	return nil
-}
-
-func SetExecPermission(path string, perm FSPermission) error {
-	SetFilePermission(path, perm)
-	return nil
-}

+// FileType represents the type of file being secured
+type FileType int
+
+const (
+	TypeDirectory FileType = iota
+	TypeExecutable
+	TypeRegularFile
+)
+
+func SetPermission(path string, perm FSPermission, fileType FileType) error {
+	if err := ensureInitialized(); err != nil {
+		return err
+	}
+	
+	// Add file type specific handling if needed
+	switch fileType {
+	case TypeDirectory:
+		// Directory specific checks/handling
+	case TypeExecutable:
+		// Executable specific checks/handling
+	}
+	
+	return setFilePermission(path, perm)
+}

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


// SetFilePermission sets the permission of a non executable file.
func SetFilePermission(path string, perm FSPermission) {
switch perm {
case AdminOnlyPermission:
// Set only admin rights, remove all others.
acl.Apply(
path,
true,
false,
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, systemSID),
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, adminsSID),
)
case PublicReadPermission:
// Set admin rights and read/execute rights for users, remove all others.
acl.Apply(
path,
true,
false,
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, systemSID),
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, adminsSID),
acl.GrantSid(windows.GENERIC_READ|windows.GENERIC_EXECUTE, usersSID),
)
case PublicWritePermission:
// Set full control to admin and regular users. Guest users will not have access.
acl.Apply(
path,
true,
false,
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, systemSID),
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, adminsSID),
acl.GrantSid(windows.GENERIC_ALL|windows.STANDARD_RIGHTS_ALL, usersSID),
)
}
}
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
)

// AsUnixDirExecPermission 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
}

// AsUnixFilePermission 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
Loading
Loading