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.

Sign up for GitHub

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 file permissions on windows #1758

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Fix file permissions on windows #1758

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1cf591a
[service] Set file permissions on windows
vlabo Nov 26, 2024
447df43
[service] Fix minor windows permission bugs
vlabo Nov 26, 2024
67b74e6
[service] Fix permission bugs
vlabo Nov 26, 2024
7d0171b
[service] Fix windows non admin user start
vlabo Nov 26, 2024
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
12 changes: 8 additions & 4 deletions base/database/storage/fstree/fstree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +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 

File is not `gci`-ed with --skip-generated -s standard -s default (gci)
"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 @@ -288,10 +289,13 @@
defer t.Cleanup() //nolint:errcheck

// Set permissions before writing data, in case the data is sensitive.
if !onWindows {
if err := t.Chmod(perm); err != nil {
return err
}
if onWindows {
err = acl.Chmod(filename, perm)
} else {
err = t.Chmod(perm)
}
if err != nil {
return err
}

if _, err := t.Write(data); err != nil {
Expand Down
8 changes: 7 additions & 1 deletion base/updater/fetch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"path/filepath"
"time"

"github.com/hectane/go-acl"

Check failure on line 17 in base/updater/fetch.go

View workflow job for this annotation

GitHub Actions / Linter 

File is not `gci`-ed with --skip-generated -s standard -s default (gci)
"github.com/safing/jess/filesig"
"github.com/safing/jess/lhash"
"github.com/safing/portmaster/base/log"
Expand Down Expand Up @@ -136,7 +137,12 @@
return fmt.Errorf("%s: failed to finalize file %s: %w", reg.Name, rv.storagePath(), err)
}
// set permissions
if !onWindows {
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 {
Expand Down
10 changes: 9 additions & 1 deletion base/utils/atomic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
"io"
"io/fs"
"os"
"path"
"runtime"

"github.com/hectane/go-acl"

Check failure on line 12 in base/utils/atomic.go

View workflow job for this annotation

GitHub Actions / Linter 

File is not `gci`-ed with --skip-generated -s standard -s default (gci)
"github.com/safing/portmaster/base/utils/renameio"
)

Expand Down Expand Up @@ -40,7 +43,12 @@
defer tmpFile.Cleanup() //nolint:errcheck

if opts.Mode != 0 {
if err := tmpFile.Chmod(opts.Mode); err != nil {
if runtime.GOOS == "windows" {
err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode)
} else {
err = tmpFile.Chmod(opts.Mode)
}
if err != 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

Use filepath.Join instead of path.Join for cross-platform compatibility.

The path package is designed for slash-separated paths (like URLs), while filepath is designed for OS-specific path manipulation. On Windows, using path.Join could lead to incorrect path separators.

-import "path"
+import "path/filepath"

-			err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode)
+			err = acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode)

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

💡 Codebase verification

🛠️ Refactor suggestion

The review comment is correct. The temporary file path is incorrectly constructed.

Based on the code analysis:

  1. renameio.TempFile creates a temporary file with pattern .{basename} in the directory determined by tempDir function
  2. The tempDir function either uses the caller-specified directory or falls back to the destination file's directory
  3. The current code incorrectly assumes the temp file is at TempDir/dest, but it's actually at a path returned by tmpFile.Name()

The suggested fix to use tmpFile.Name() is the correct approach as it will use the actual temporary file path rather than an incorrect assumption about its location.

🔗 Analysis chain

Verify the temporary file path construction.

The code attempts to chmod the file at TempDir/dest, but renameio.TempFile might use a different path pattern for the temporary file. This could result in permission changes being applied to the wrong file.

Additionally, consider refactoring to use the actual temporary file path:

 	if opts.Mode != 0 {
 		if runtime.GOOS == "windows" {
-			err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode)
+			err = acl.Chmod(tmpFile.Name(), opts.Mode)
 		} else {
 			err = tmpFile.Chmod(opts.Mode)
 		}

Let me check how the tempDir function constructs the temporary directory path.

Let me check the complete implementation of tempDir to understand how it determines the final temporary directory path.

Let me check how the TempFile is being used in the atomic.go file to understand the complete context.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how renameio.TempFile constructs temporary paths
rg -A 10 'func TempFile' --type go

Length of output: 762

Script:

#!/bin/bash
# Check how tempDir function constructs temporary directory paths
rg -A 5 'func tempDir' --type go

Length of output: 411

Script:

#!/bin/bash
# Get the complete tempDir function implementation
rg -A 15 'func tempDir' --type go

Length of output: 1109

Script:

#!/bin/bash
# Get the context of TempFile usage in atomic.go
rg -B 10 -A 10 'TempFile' base/utils/atomic.go

Length of output: 807

return fmt.Errorf("failed to update mode bits of temp file: %w", err)
}
}
Expand Down
11 changes: 8 additions & 3 deletions base/utils/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"io/fs"
"os"
"runtime"

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

const isWindows = runtime.GOOS == "windows"
Expand All @@ -20,8 +22,7 @@ func EnsureDirectory(path string, perm os.FileMode) error {
if f.IsDir() {
// directory exists, check permissions
if isWindows {
// TODO: set correct permission on windows
// acl.Chmod(path, perm)
return acl.Chmod(path, perm)
} else if f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}
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 potential nil pointer dereference in permission check

The permission check in the else condition could cause a panic when the file doesn't exist, as f would be nil.

Apply this fix:

 if isWindows {
   return acl.Chmod(path, perm)
-} else if f.Mode().Perm() != perm {
+} else if f != nil && f.Mode().Perm() != perm {
   return os.Chmod(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
if isWindows {
// TODO: set correct permission on windows
// acl.Chmod(path, perm)
return acl.Chmod(path, perm)
} else if f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}
if isWindows {
return acl.Chmod(path, perm)
} else if f != nil && f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}

Expand All @@ -38,7 +39,11 @@ func EnsureDirectory(path string, perm os.FileMode) error {
if err != nil {
return fmt.Errorf("could not create dir %s: %w", path, err)
}
return os.Chmod(path, perm)
if isWindows {
return acl.Chmod(path, perm)
} else if f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}
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 unreachable and potentially dangerous code

There are several issues in this section:

  1. The f variable is nil in this code path as we just created the directory
  2. The permission check is redundant as we just created the directory with the specified permissions
  3. The else condition would cause a nil pointer panic

Apply this fix:

 if isWindows {
   return acl.Chmod(path, perm)
-} else if f.Mode().Perm() != perm {
-  return os.Chmod(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
}
return os.Chmod(path, perm)
if isWindows {
return acl.Chmod(path, perm)
} else if f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}
}
if isWindows {
return acl.Chmod(path, perm)
}

}
// other error opening path
return fmt.Errorf("failed to access %s: %w", path, err)
Expand Down
14 changes: 12 additions & 2 deletions base/utils/renameio/writefile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package renameio

import "os"
import (
"os"
"runtime"

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

// WriteFile mirrors os.WriteFile, replacing an existing file with the same
// name atomically.
Expand All @@ -14,7 +19,12 @@ func WriteFile(filename string, data []byte, perm os.FileMode) error {
}()

// Set permissions before writing data, in case the data is sensitive.
if err := t.Chmod(perm); err != nil {
if runtime.GOOS == "windows" {
err = acl.Chmod(t.path, perm)
} else {
err = t.Chmod(perm)
}
if err != nil {
return err
}

Expand Down
8 changes: 7 additions & 1 deletion cmds/portmaster-start/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"strings"
"time"

"github.com/hectane/go-acl"
"github.com/spf13/cobra"
"github.com/tevino/abool"

Expand Down Expand Up @@ -272,7 +273,12 @@ func fixExecPerm(path string) error {
return nil
}

if err := os.Chmod(path, 0o0755); err != nil { //nolint:gosec // Set execution rights.
if runtime.GOOS == "windows" {
err = acl.Chmod(path, 0o0755)
} else {
err = os.Chmod(path, 0o0755)
}
if err != nil { //nolint:gosec
return fmt.Errorf("failed to chmod %s: %w", path, err)
}

Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ require (
github.com/gorilla/websocket v1.5.3
github.com/hashicorp/go-multierror v1.1.1
github.com/hashicorp/go-version v1.7.0
github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb
github.com/jackc/puddle/v2 v2.2.2
github.com/lmittmann/tint v1.0.5
github.com/maruel/panicparse/v2 v2.3.1
Expand Down
3 changes: 3 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ github.com/hashicorp/go-version v1.7.0 h1:5tqGy27NaOTB8yJKUZELlFAS/LTKJkrmONwQKe
github.com/hashicorp/go-version v1.7.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/hcl v0.0.0-20170914154624-68e816d1c783/go.mod h1:oZtUIOe8dh44I2q6ScRibXws4Ajl+d+nod3AaR9vL5w=
github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb h1:PGufWXXDq9yaev6xX1YQauaO1MV90e6Mpoq1I7Lz/VM=
github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb/go.mod h1:QiyDdbZLaJ/mZP4Zwc9g2QsfaEA4o7XvvgZegSci5/E=
github.com/inconshreveable/log15 v0.0.0-20170622235902-74a0988b5f80/go.mod h1:cOaXtrgN4ScfRrD9Bre7U1thNq5RtJ8ZoP4iXVGRj6o=
github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
Expand Down Expand Up @@ -388,6 +390,7 @@ golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5h
golang.org/x/sys v0.0.0-20190312061237-fead79001313/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190411185658-b44545bcd369/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190529164535-6a60838ec259/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190626221950-04f50cda93cb/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190826190057-c7b8b68b1456/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Expand Down
10 changes: 8 additions & 2 deletions service/updates/helper/electron.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"runtime"
"strings"

"github.com/hectane/go-acl"
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

Remove unused import

The go-acl package is imported but never used in this file. This appears to be an oversight, possibly from an incomplete implementation of Windows-specific permission handling.

-	"github.com/hectane/go-acl"
📝 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
"github.com/hectane/go-acl"
🧰 Tools
🪛 GitHub Check: Linter

[failure] 11-11:
"github.com/hectane/go-acl" imported as acl and not used (typecheck)

[failure] 11-11:
"github.com/hectane/go-acl" imported as acl and not used) (typecheck)

"github.com/safing/portmaster/base/log"
"github.com/safing/portmaster/base/updater"
)
Expand Down Expand Up @@ -46,11 +47,16 @@ func EnsureChromeSandboxPermissions(reg *updater.ResourceRegistry) error {
filepath.Ext(pmElectronUpdate.Path()),
)
sandboxFile := filepath.Join(unpackedPath, "chrome-sandbox")
if err := os.Chmod(sandboxFile, 0o0755|os.ModeSetuid); err != nil {
if runtime.GOOS == "windows" {
err = acl.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
} else {
err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
}
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: Inconsistent OS handling and unnecessary Windows logic

There are several issues with the Windows-specific changes:

  1. The function documentation states it's Linux-specific, but now includes Windows logic
  2. There's an early return for non-Linux systems (line 26) that makes this Windows-specific code unreachable
  3. Setting SUID bit on Windows is unnecessary as Windows doesn't use the SUID concept
  4. The Unix permission mask might not translate correctly to Windows ACLs

Consider removing the Windows-specific code since:

  1. The function is designed for Linux-specific Chrome sandbox SUID requirements
  2. The early return makes this code unreachable
  3. SUID permissions aren't applicable to Windows
-	if runtime.GOOS == "windows" {
-		err = acl.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
-	} else {
-		err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
-	}
+	err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
📝 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
if runtime.GOOS == "windows" {
err = acl.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
} else {
err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
}
err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)

if err != nil {
log.Errorf(suidBitWarning, 0o0755|os.ModeSetuid, sandboxFile)

return fmt.Errorf("failed to chmod: %w", err)
}

log.Debugf("updates: fixed SUID permission for chrome-sandbox")

return nil
Expand Down
8 changes: 7 additions & 1 deletion service/updates/upgrader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"
"time"

"github.com/hectane/go-acl"
processInfo "github.com/shirou/gopsutil/process"
"github.com/tevino/abool"

Expand Down Expand Up @@ -349,7 +350,12 @@ func upgradeBinary(fileToUpgrade string, file *updater.File) error {
}

// check permissions
if !onWindows {
if onWindows {
err = acl.Chmod(fileToUpgrade, 0o0755)
if err != nil {
return fmt.Errorf("failed to set permissions on %s: %w", fileToUpgrade, err)
}
} else {
info, err := os.Stat(fileToUpgrade)
if err != nil {
return fmt.Errorf("failed to get file info on %s: %w", fileToUpgrade, err)
Expand Down
Loading