Fix for privilege escalation vulnerability

[go-compile]

Description

Patch for a privilege escalation vulnerability in the service component of AdGuardHome.

Please note that this pull request does not come with a issue number because it was privately disclosed via email to @ameshkov and @ainar-g; due to the sensitive nature of security disclosure.

Patch Details

This patch sets the correct executable permissions for AdGuardHome when running the AdGuardHome -s install command. This includes setting the owner as root:root and permissions to 0755 (blocks write access to all users except root). Furthermore, the binary is moved to /usr/bin/ to add it to the global path and vitally place it in a non-root read-only directory, otherwise the previous steps would not be enough to protect against this vulnerability.

Patch Knock On Effects (all accounted for)

Additional changes include, disabling the installation of AdGuardHome as a service on Windows due the vulnerability being unable to be patched (Go lacks the proper API bindings to set file ownership and permissions on Windows). It is highly recommended a msi installer is created to install the service, rather than allow a user to try install it manually without any guidance. Moreover, even with guidance the process is complex and time consuming. Do note this can easily be changed in internal/home/service.go on line 698.

Other alterations. There is a issue where AdGuardHome will write it's data to /usr/bin/ if the executable is within that path. This is an inappropriate directory for data and thus should not be used. Instead if the program is within /usr/bin/ AdGuardHome now assumes a new directory /var/lib/AdGuardHome (auto created recursively if it does not exist). Note, this does not interfere with the desired behavior of allowing AdGuardHome to use the current executable dir as it's workDir, it only redirects the workDir if the binary is within user/bin (essentially if AdGuardHome is running as a service it will store data in the new location, unless overwritten with the existing workdir CLI option).

Testing

This PR has been tested on Ubuntu wsl.

[ameshkov]

Thank you for the submission!

Added some comments there. However, let's wait for @ainar-g's input before changing anything, he may add some corrections.

[codecov-commenter]

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (bd99e3e) 51.74% compared to head (60177ce) 51.66%.

Files	Patch %	Lines
internal/home/service.go	0.00%	19 Missing ⚠️
internal/home/home.go	0.00%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6748      +/-   ##
==========================================
- Coverage   51.74%   51.66%   -0.08%     
==========================================
  Files         185      185              
  Lines       14884    14907      +23     
==========================================
  Hits         7702     7702              
- Misses       6431     6454      +23     
  Partials      751      751              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ainar-g]

Additional notes:

• It is not clear how this change affects the BSDs, and in particular macOS, which really wants applications to be installed in /Applications, for example.

• It seems like it's breaking Docker? There are assumptions about AdGuard Home being installed to /opt/ there.

• The install script should probably be changes as well to fully implement the change for new installations as well.

[go-compile]

It is not clear how this change affects the BSDs, and in particular macOS, which really wants applications to be installed in /Applications, for example.

Regarding macOS, I am very unfamiliar with the system. But if /Applications is owned by root and denies writes by non-root users, I don't see we can't just define const serviceInstallDir = "/usr/bin/AdGuardHome" to const serviceInstallDir = "/Applications/AdGuardHome". Then also add/Applications to protectedDirectories. These variables could be moved to os_linux.go, os_darwin within the aghos package.

I will test AdGuardHome on a OpenBSD vm at some point and verify it works; currently it uses the same patch as the one for linux.

It seems like it's breaking Docker? There are assumptions about AdGuard Home being installed to /opt/ there.

I don't see why it would break docker as Docker doesn't install the service and the checks added only run when -s install is ran?

The install script should probably be changes as well to fully implement the change for new installations as well.

I am not as familiar with the script, and it is very large. But would redefining $agh_dir to equal the new location, obtained by the command whereis or hard coded work?

...

# Function install_service tries to install AGH as service.
install_service() {
	# Installing the service as root is required at least on FreeBSD.
	use_sudo='0'
	if [ "$os" = 'freebsd' ]
	then
		use_sudo='1'
	fi

	if ( cd "$agh_dir" && maybe_sudo ./AdGuardHome -s install )
	then
                $agh_dir = "/usr/bin"
		return 0
	fi

	log "installation failed, removing $agh_dir"

...

Testing Results

Operating System	Result
Ubuntu	Working
Windows	Working
OpenBSD	Not tested
MacOS	Platform not available

[ghost]

Notes from a macOS perspective, while it's not strictly required by the system, there are reasons as to why most users should be running it from the Applications folder:

• Any apps that interface with AdGuard Home will presume it is installed into the Applications folder.
• On a multi-user environment, installing to the Applications folder is required so that all users can use it.
• Running AdGuard Home from the Applications folder can be more secure as it separates them from user data and system files.