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

Merged
merged 4 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
12 changes: 8 additions & 4 deletions base/database/storage/fstree/fstree.go
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
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
15 changes: 12 additions & 3 deletions base/utils/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
"io/fs"
"os"
"runtime"

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

const isWindows = runtime.GOOS == "windows"
Expand All @@ -20,8 +22,9 @@
if f.IsDir() {
// directory exists, check permissions
if isWindows {
// TODO: set correct permission on windows
// acl.Chmod(path, perm)
// Ignore windows permission error. For none admin users it will always fail.
acl.Chmod(path, perm)

Check failure on line 26 in base/utils/fs.go

View workflow job for this annotation

GitHub Actions / Linter

Error return value is not checked (errcheck)
return nil
} else if f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}
Expand All @@ -38,7 +41,13 @@
if err != nil {
return fmt.Errorf("could not create dir %s: %w", path, err)
}
return os.Chmod(path, perm)
if isWindows {
// Ignore windows permission error. For none admin users it will always fail.
acl.Chmod(path, perm)

Check failure on line 46 in base/utils/fs.go

View workflow job for this annotation

GitHub Actions / Linter

Error return value is not checked (errcheck)
return nil
} else {
return os.Chmod(path, perm)
}
Comment on lines +44 to +50
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 to reduce duplication and simplify control flow

The Windows permission handling code is duplicated and the control flow could be simplified.

-		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)
-		}
+		if isWindows {
+			// Ignore windows permission error. For non-admin users it will always fail.
+			if err := acl.Chmod(path, perm); err != nil {
+				log.Debugf("Failed to set permissions on %s: %v", path, err)
+			}
+			return nil
+		}
+		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 {
// Ignore windows permission error. For none admin users it will always fail.
acl.Chmod(path, perm)
return nil
} else {
return os.Chmod(path, perm)
}
if isWindows {
// Ignore windows permission error. For non-admin users it will always fail.
if err := acl.Chmod(path, perm); err != nil {
log.Debugf("Failed to set permissions on %s: %v", path, err)
}
return nil
}
return os.Chmod(path, perm)
🧰 Tools
🪛 GitHub Check: Linter

[failure] 46-46:
Error return value is not checked (errcheck)

🪛 golangci-lint (1.62.2)

46-46: Error return value is not checked

(errcheck)

}
// 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
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
2 changes: 1 addition & 1 deletion cmds/portmaster-start/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func fixExecPerm(path string) error {
return nil
}

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

Expand Down
1 change: 1 addition & 0 deletions go.mod
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
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
2 changes: 1 addition & 1 deletion service/updates/helper/electron.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ func EnsureChromeSandboxPermissions(reg *updater.ResourceRegistry) error {
sandboxFile := filepath.Join(unpackedPath, "chrome-sandbox")
if err := os.Chmod(sandboxFile, 0o0755|os.ModeSetuid); 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
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