From c0a5e389d9cc985f399f83736c338fdef1913433 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Sun, 18 Feb 2024 16:24:20 +0000 Subject: [PATCH 1/9] fix: service binary overwrite privilege escalation vuln --- internal/home/home.go | 3 +++ internal/home/service.go | 48 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/internal/home/home.go b/internal/home/home.go index 22f6130c75f..adaeb6351a1 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -748,6 +748,7 @@ func writePIDFile(fn string) bool { // initConfigFilename sets up context config file path. This file path can be // overridden by command-line arguments, or is set to default. func initConfigFilename(opts options) { + // TODO: if running as service the config location should be /etc/AdGuardHome/AdGuardHome.yaml Context.configFilename = stringutil.Coalesce(opts.confFilename, "AdGuardHome.yaml") } @@ -760,6 +761,8 @@ func initWorkingDir(opts options) (err error) { return err } + // TODO: if running as a service use /var/lib/AdGuardHome + if opts.workDir != "" { // If there is a custom config file, use it's directory as our working dir Context.workDir = opts.workDir diff --git a/internal/home/service.go b/internal/home/service.go index 1a80ca07321..13248373adb 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -304,6 +304,11 @@ func handleServiceStatusCommand(s service.Service) { // handleServiceStatusCommand handles service "install" command func handleServiceInstallCommand(s service.Service) { + // Set the binary's permissions and move to /usr/bin (if on linux) + if err := secureBinary(); err != nil { + log.Fatal(err) + } + err := svcAction(s, "install") if err != nil { log.Fatalf("service: executing action %q: %s", "install", err) @@ -681,3 +686,46 @@ rc_bg=YES rc_cmd $1 ` + +// secureBinary is used before service.Install(). This function protects AdGuardHome from +// privilege escalation vulnerabilities caused by writable files +func secureBinary() error { + switch runtime.GOOS { + case "windows": + // TODO: support windows service support securely + // Set file owner to admin/system and public permissions read-only + return errors.Error("you currently cannot install adguardhome as a service on window") + default: + return secureBinaryUnix() + } +} + +func secureBinaryUnix() error { + // Installalation can only be completed with root privileges, so check and handle if not + if os.Getuid() != 0 { + return errors.Error("permission denied. Root privileges required") + } + + // Get current file path + binary := os.Args[0] + + // Change owner to root:root + err := os.Chown(binary, 0, 0) + if err != nil { + return err + } + + // Set permissions to root(read,write,exec), group(read,exec), public(read) + // This combined with changing the owner make the file undeletable without root privlages + // UNLESS THE PARENT FOLDER IS WRITABLE! + if err := os.Chmod(binary, 0755); err != nil { + return err + } + + // Move binary to the PATH in a folder which is read-only to non root users + if err := os.Rename(binary, "/usr/bin/AdGuardHome"); err != nil { + return err + } + + return nil +} From 60177cebf2602c52a50a5ecdb6eebda8d974f489 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Sun, 18 Feb 2024 17:09:16 +0000 Subject: [PATCH 2/9] feat: if running as a service set workdir to /var/lib/AdGuardHome Fixes issue where the service would write the data dir into /usr/bin if the binary was within /usr/bin --- internal/home/home.go | 12 +++++++++--- internal/home/service.go | 10 ++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/home/home.go b/internal/home/home.go index adaeb6351a1..aa781f7b5dc 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -748,7 +748,6 @@ func writePIDFile(fn string) bool { // initConfigFilename sets up context config file path. This file path can be // overridden by command-line arguments, or is set to default. func initConfigFilename(opts options) { - // TODO: if running as service the config location should be /etc/AdGuardHome/AdGuardHome.yaml Context.configFilename = stringutil.Coalesce(opts.confFilename, "AdGuardHome.yaml") } @@ -761,11 +760,18 @@ func initWorkingDir(opts options) (err error) { return err } - // TODO: if running as a service use /var/lib/AdGuardHome - if opts.workDir != "" { // If there is a custom config file, use it's directory as our working dir Context.workDir = opts.workDir + } else if !execDirAvaliable() { + // If running as a service and from /usr/bin/ use /var/lib for working dir instead of + // /usr/bin/data + Context.workDir = "/var/lib/AdGuardHome" + + // Create dir if it does not already exist + if err := os.MkdirAll(Context.workDir, 0755); err != nil { + return err + } } else { Context.workDir = filepath.Dir(execPath) } diff --git a/internal/home/service.go b/internal/home/service.go index 13248373adb..2f3f2a026a5 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -4,6 +4,7 @@ import ( "fmt" "io/fs" "os" + "path/filepath" "runtime" "strconv" "strings" @@ -729,3 +730,12 @@ func secureBinaryUnix() error { return nil } + +// execDirAvaliable returns true if the executable's current folder is avaliable to be +// used as a workDir. +// If AdGuardHome is running as a service, it should not use the binary's location as a +// workDir, thus this function will return false. +func execDirAvaliable() bool { + // If installed in /usr/bin do not use /usr/bin/data to store files + return filepath.Dir(os.Args[0]) != "/usr/bin" +} From 2c4ebe71d9238852130f672181cf571e177cd8df Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 13:32:19 +0000 Subject: [PATCH 3/9] fix: use os.Executable instead of os.Args[0] to get prog path --- internal/home/service.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/home/service.go b/internal/home/service.go index 2f3f2a026a5..ad45926167d 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -708,10 +708,13 @@ func secureBinaryUnix() error { } // Get current file path - binary := os.Args[0] + binary, err := os.Executable() + if err != nil { + return err + } // Change owner to root:root - err := os.Chown(binary, 0, 0) + err = os.Chown(binary, 0, 0) if err != nil { return err } @@ -724,6 +727,7 @@ func secureBinaryUnix() error { } // Move binary to the PATH in a folder which is read-only to non root users + // If already moved, this is a no-op if err := os.Rename(binary, "/usr/bin/AdGuardHome"); err != nil { return err } From 11a7ac2013aa70d174d39e53b9975e346e5c4e51 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 13:35:31 +0000 Subject: [PATCH 4/9] fix: use os.Executable instead of os.Args[0] to get prog dir --- internal/home/service.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/home/service.go b/internal/home/service.go index ad45926167d..8639590154c 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -740,6 +740,11 @@ func secureBinaryUnix() error { // If AdGuardHome is running as a service, it should not use the binary's location as a // workDir, thus this function will return false. func execDirAvaliable() bool { + binary, err := os.Executable() + if err != nil { + panic(err) + } + // If installed in /usr/bin do not use /usr/bin/data to store files - return filepath.Dir(os.Args[0]) != "/usr/bin" + return filepath.Dir(binary) != "/usr/bin" } From 8afecf5a95261f73afdf4d450a1e98a713dea467 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 17:29:34 +0000 Subject: [PATCH 5/9] refactor: clean up and move binary security checks to aghos --- internal/aghos/binarysecurity_other.go | 83 ++++++++++++++++++++++++ internal/aghos/binarysecurity_windows.go | 47 ++++++++++++++ internal/home/home.go | 10 ++- internal/home/service.go | 64 +----------------- 4 files changed, 139 insertions(+), 65 deletions(-) create mode 100644 internal/aghos/binarysecurity_other.go create mode 100644 internal/aghos/binarysecurity_windows.go diff --git a/internal/aghos/binarysecurity_other.go b/internal/aghos/binarysecurity_other.go new file mode 100644 index 00000000000..7cadc4d6738 --- /dev/null +++ b/internal/aghos/binarysecurity_other.go @@ -0,0 +1,83 @@ +//go:build !windows + +package aghos + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/log" +) + +// protectedDirectories are directories which contain other application binaries, +// as such AdGuard Home should never attempt store application data here, at risk of +// overwriting other files. Moreover, these directories are innapproriate for storage of +// config files or session storage. +var protectedDirectories = []string{ + "/usr/bin" + "/usr/sbin" + "/user/bin" +} + +// serviceInstallDir is a executable path in a directory which secure permissions +// which prevent the manipulation of the binary. +const serviceInstallDir = "/usr/bin/AdGuardHome" + +// SecureBinary is used before service.Install(). This function protects AdGuardHome from +// privilege escalation vulnerabilities caused by writable files +func SecureBinary() error { + // Installalation can only be completed with root privileges, so check and handle if not + if os.Getuid() != 0 { + return errors.Error("permission denied. Root privileges required") + } + + // Get current file path + binary, err := os.Executable() + if err != nil { + return fmt.Errorf("os.Executable(): %w", err) + } + + // Change owner to root:root + err = os.Chown(binary, 0, 0) + if err != nil { + return fmt.Errorf("os.Chown() %q: %w", binary, err) + } + + // Set permissions to root(read,write,exec), group(read,exec), public(read) + // This combined with changing the owner make the file undeletable without root privlages + // UNLESS THE PARENT FOLDER IS WRITABLE! + if err := os.Chmod(binary, 0755); err != nil { + return fmt.Errorf("os.Chmod() %q: %w", binary, err) + } + + + // Move binary to the PATH in a folder which is read-only to non root users + // If already moved, this is a no-op + if err := os.Rename(binary, serviceInstallDir); err != nil { + return fmt.Errorf("os.Rename() %q to %q: %w", binary, installDir, err) + } + + return nil +} + +// CurrentDirAvaliable returns true if it is okay to use this directory to store application +// data. +func CurrentDirAvaliable() (bool, error) { + binary, err := os.Executable() + if err != nil { + return fmt.Errorf("os.Executable(): %w", err) + } + + for i := 0; i < len(protectedDirectories); i++ { + // Check if binary is within a protected directory + if strings.HasPrefix(binary, protectedDirectories[i]) { + // The binary is within a protected directory + return false, nil + } + } + + // The binary is outside of all checked protected directories + return true, nil +} \ No newline at end of file diff --git a/internal/aghos/binarysecurity_windows.go b/internal/aghos/binarysecurity_windows.go new file mode 100644 index 00000000000..53a1015976e --- /dev/null +++ b/internal/aghos/binarysecurity_windows.go @@ -0,0 +1,47 @@ +//go:build windows + +package aghos + +import ( + "fmt" + "os" + "strings" +) + +// securePrefixDirectories is a list of directories where a service binary +// has the appropriate permissions to mitigate a binary planting attack +var securePrefixDirectories = []string{ + "C:\\Program Files", + "C:\\Program Files (x86)", + + // Some Windows users place binaries within /Windows/System32 to add it to %PATH% + "C:\\Windows", +} + +// SecureBinary is used before service.Install(). This function protects AdGuardHome from +// privilege escalation vulnerabilities caused by writable files +func SecureBinary() error { + // Get current file path + binary, err := os.Executable() + if err != nil { + return fmt.Errorf("os.Executable(): %w", err) + } + + for i := 0; i < len(securePrefixDirectories); i++ { + // Check if binary is within a secure folder write protected folder + if strings.HasPrefix(binary, securePrefixDirectories[i]) { + // The binary is within a secure directory already + return nil + } + } + + // No secure directories matched + return fmt.Errorf("insecure binary location for service instalation: %q. Please view: https://adguard-dns.io/kb/adguard-home/running-securely/", binary) +} + +// CurrentDirAvaliable returns true if it is okay to use this directory to store application +// data. +func CurrentDirAvaliable() (bool, error) { + // We do not mind what directory is used on Windows + return true, nil +} diff --git a/internal/home/home.go b/internal/home/home.go index aa781f7b5dc..fe44771a83d 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -760,17 +760,23 @@ func initWorkingDir(opts options) (err error) { return err } + // Can we use the current directory to store application data? + pwdAvaliable, err := aghos.CurrentDirAvaliable() + if err != nil { + return err + } + if opts.workDir != "" { // If there is a custom config file, use it's directory as our working dir Context.workDir = opts.workDir - } else if !execDirAvaliable() { + } else if pwdAvaliable { // If running as a service and from /usr/bin/ use /var/lib for working dir instead of // /usr/bin/data Context.workDir = "/var/lib/AdGuardHome" // Create dir if it does not already exist if err := os.MkdirAll(Context.workDir, 0755); err != nil { - return err + return fmt.Errorf("os.MkdirAll: %s: %w", Context.workDir, err) } } else { Context.workDir = filepath.Dir(execPath) diff --git a/internal/home/service.go b/internal/home/service.go index 8639590154c..eac64f6ed99 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -4,7 +4,6 @@ import ( "fmt" "io/fs" "os" - "path/filepath" "runtime" "strconv" "strings" @@ -306,7 +305,7 @@ func handleServiceStatusCommand(s service.Service) { // handleServiceStatusCommand handles service "install" command func handleServiceInstallCommand(s service.Service) { // Set the binary's permissions and move to /usr/bin (if on linux) - if err := secureBinary(); err != nil { + if err := aghos.SecureBinary(); err != nil { log.Fatal(err) } @@ -687,64 +686,3 @@ rc_bg=YES rc_cmd $1 ` - -// secureBinary is used before service.Install(). This function protects AdGuardHome from -// privilege escalation vulnerabilities caused by writable files -func secureBinary() error { - switch runtime.GOOS { - case "windows": - // TODO: support windows service support securely - // Set file owner to admin/system and public permissions read-only - return errors.Error("you currently cannot install adguardhome as a service on window") - default: - return secureBinaryUnix() - } -} - -func secureBinaryUnix() error { - // Installalation can only be completed with root privileges, so check and handle if not - if os.Getuid() != 0 { - return errors.Error("permission denied. Root privileges required") - } - - // Get current file path - binary, err := os.Executable() - if err != nil { - return err - } - - // Change owner to root:root - err = os.Chown(binary, 0, 0) - if err != nil { - return err - } - - // Set permissions to root(read,write,exec), group(read,exec), public(read) - // This combined with changing the owner make the file undeletable without root privlages - // UNLESS THE PARENT FOLDER IS WRITABLE! - if err := os.Chmod(binary, 0755); err != nil { - return err - } - - // Move binary to the PATH in a folder which is read-only to non root users - // If already moved, this is a no-op - if err := os.Rename(binary, "/usr/bin/AdGuardHome"); err != nil { - return err - } - - return nil -} - -// execDirAvaliable returns true if the executable's current folder is avaliable to be -// used as a workDir. -// If AdGuardHome is running as a service, it should not use the binary's location as a -// workDir, thus this function will return false. -func execDirAvaliable() bool { - binary, err := os.Executable() - if err != nil { - panic(err) - } - - // If installed in /usr/bin do not use /usr/bin/data to store files - return filepath.Dir(binary) != "/usr/bin" -} From 5acde57e1155adb2472fd0ca949e149bc333dfc8 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 17:32:41 +0000 Subject: [PATCH 6/9] fix: rename binarysecurity_other.go to plural others --- .../aghos/{binarysecurity_other.go => binarysecurity_others.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename internal/aghos/{binarysecurity_other.go => binarysecurity_others.go} (100%) diff --git a/internal/aghos/binarysecurity_other.go b/internal/aghos/binarysecurity_others.go similarity index 100% rename from internal/aghos/binarysecurity_other.go rename to internal/aghos/binarysecurity_others.go From 9ce23a6488b1a92d2850252795da9dcb77f91961 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 17:37:16 +0000 Subject: [PATCH 7/9] fix: typos --- internal/aghos/binarysecurity_others.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/aghos/binarysecurity_others.go b/internal/aghos/binarysecurity_others.go index 7cadc4d6738..a7410df4cef 100644 --- a/internal/aghos/binarysecurity_others.go +++ b/internal/aghos/binarysecurity_others.go @@ -5,10 +5,9 @@ package aghos import ( "fmt" "os" - "path/filepath" + "strings" "github.com/AdguardTeam/golibs/errors" - "github.com/AdguardTeam/golibs/log" ) // protectedDirectories are directories which contain other application binaries, @@ -16,13 +15,13 @@ import ( // overwriting other files. Moreover, these directories are innapproriate for storage of // config files or session storage. var protectedDirectories = []string{ - "/usr/bin" - "/usr/sbin" - "/user/bin" + "/usr/bin", + "/usr/sbin", + "/user/bin", } // serviceInstallDir is a executable path in a directory which secure permissions -// which prevent the manipulation of the binary. +// which prevent the manipulation of the binary. const serviceInstallDir = "/usr/bin/AdGuardHome" // SecureBinary is used before service.Install(). This function protects AdGuardHome from @@ -52,11 +51,10 @@ func SecureBinary() error { return fmt.Errorf("os.Chmod() %q: %w", binary, err) } - // Move binary to the PATH in a folder which is read-only to non root users // If already moved, this is a no-op if err := os.Rename(binary, serviceInstallDir); err != nil { - return fmt.Errorf("os.Rename() %q to %q: %w", binary, installDir, err) + return fmt.Errorf("os.Rename() %q to %q: %w", binary, serviceInstallDir, err) } return nil @@ -67,7 +65,7 @@ func SecureBinary() error { func CurrentDirAvaliable() (bool, error) { binary, err := os.Executable() if err != nil { - return fmt.Errorf("os.Executable(): %w", err) + return false, fmt.Errorf("os.Executable(): %w", err) } for i := 0; i < len(protectedDirectories); i++ { @@ -80,4 +78,4 @@ func CurrentDirAvaliable() (bool, error) { // The binary is outside of all checked protected directories return true, nil -} \ No newline at end of file +} From 16f809bb5e0d7f8b595eb2896a8627d53577cc89 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 17:41:49 +0000 Subject: [PATCH 8/9] fix: shadowed err --- internal/aghos/binarysecurity_others.go | 6 ++++-- internal/home/home.go | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/aghos/binarysecurity_others.go b/internal/aghos/binarysecurity_others.go index a7410df4cef..bfd790d36bd 100644 --- a/internal/aghos/binarysecurity_others.go +++ b/internal/aghos/binarysecurity_others.go @@ -47,13 +47,15 @@ func SecureBinary() error { // Set permissions to root(read,write,exec), group(read,exec), public(read) // This combined with changing the owner make the file undeletable without root privlages // UNLESS THE PARENT FOLDER IS WRITABLE! - if err := os.Chmod(binary, 0755); err != nil { + err = os.Chmod(binary, 0755) + if err != nil { return fmt.Errorf("os.Chmod() %q: %w", binary, err) } // Move binary to the PATH in a folder which is read-only to non root users // If already moved, this is a no-op - if err := os.Rename(binary, serviceInstallDir); err != nil { + err = os.Rename(binary, serviceInstallDir) + if err != nil { return fmt.Errorf("os.Rename() %q to %q: %w", binary, serviceInstallDir, err) } diff --git a/internal/home/home.go b/internal/home/home.go index fe44771a83d..925c2d0294a 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -775,7 +775,8 @@ func initWorkingDir(opts options) (err error) { Context.workDir = "/var/lib/AdGuardHome" // Create dir if it does not already exist - if err := os.MkdirAll(Context.workDir, 0755); err != nil { + err := os.MkdirAll(Context.workDir, 0755) + if err != nil { return fmt.Errorf("os.MkdirAll: %s: %w", Context.workDir, err) } } else { From 311ddecd264b195e8b54f9927affce531087d744 Mon Sep 17 00:00:00 2001 From: go-compile <97609133+go-compile@users.noreply.github.com> Date: Tue, 20 Feb 2024 18:10:55 +0000 Subject: [PATCH 9/9] fix: change dir if \!CurrentDirAvaliable() --- internal/home/home.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/home/home.go b/internal/home/home.go index 925c2d0294a..cea85f70143 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -769,7 +769,7 @@ func initWorkingDir(opts options) (err error) { if opts.workDir != "" { // If there is a custom config file, use it's directory as our working dir Context.workDir = opts.workDir - } else if pwdAvaliable { + } else if !pwdAvaliable { // If running as a service and from /usr/bin/ use /var/lib for working dir instead of // /usr/bin/data Context.workDir = "/var/lib/AdGuardHome"