Bug 12562

Summary: Multiple command-injections and privilege escalations in misc-progs
Product: IPFire Reporter: ipfire
Component: ---Assignee: Michael Tremer <michael.tremer>
Status: CLOSED FIXED QA Contact:
Severity: Security    
Priority: - Unknown - CC: arne.fitzenreiter, michael.tremer, peter.mueller, peter.mueller
Version: 2Keywords: Security
Hardware: all   
OS: Linux   
Attachments: 0001-Drop-launch-ether-wake.patch
0002-core154-Manually-set-capabilites-for-etherwake.patch
0003-misc-progs-Introduce-run.patch
0004-misc-progs-backupctrl-Use-new-run-function.patch
0005-misc-progs-pakfire-Use-new-run-function.patch
0006-misc-progs-Add-functions-to-sanitise-input-arguments.patch
0007-misc-progs-addonctrl-Sanitise-add-on-names-before-us.patch
0008-misc-progs-extrahdctrl-Use-new-run-function.patch
0009-misc-progs-mpfirectrl-Use-new-run-function.patch
0010-misc-progs-smartctrl-Sanitise-device-name.patch
0011-misc-progs-sshctrl-Sanitise-runtime-for-tempstart.patch
0012-misc-progs-sambactrl-Remove-unused-smbsafeconfpdc-co.patch
0013-misc-progs-sambactrl-Sanitise-username.patch
0014-samba-Remove-option-to-chose-user-group-and-shell.patch
0015-samba-Add-helper-script-to-pipe-password.patch
0001-misc-progs-addonctrl-Replace-all-sprintf-with-snprin.patch

Description ipfire 2020-12-30 22:03:16 UTC 
Introduction
------------
The misc-progs utilities are often setuid and can be executed by the group "nobody". They use functions like "safe_system" and often do not perform input validation.

See for example:
https://github.com/ipfire/ipfire-2.x/blob/master/src/misc-progs/launch-ether-wake.c#L29

As it stands, these binaries are a trivial way to elevate privileges should a vulnerable service be exploited. 

I didn't see misc-progs in ipfire 3. However, since ipfire 2 seems to be the release that's offered for general use, this does effect ipfire users today.


Example
------
In the following example, tested on the newest ipfire release, a user "test" has been created that's also a member of the group "nobody". 

-su-5.0$ cat /etc/os-release 
NAME="IPFire"
VERSION="2.25"
ID=ipfire
VERSION_ID=2
PRETTY_NAME="IPFire 2.25 (x86_64) - core153"
ANSI_COLOR="0:31"
-su-5.0$ id
uid=1001(test) gid=1001(test) groups=1001(test),99(nobody)
-su-5.0$ /usr/local/bin/launch-ether-wake test ';/bin/bash;%'
/usr/sbin/etherwake: option requires an argument -- 'i'
usage: ether-wake [-i <ifname>] [-p aa:bb:cc:dd[:ee:ff]] 00:11:22:33:44:55
   Use '-u' to see the complete set of options.
[root@ipfire test]# id
uid=0(root) gid=0(root) groups=0(root)

So, an attacker that takes control over a process that's in the nobody group, e. g. the httpd apache server or unbound, can trivially gain root access.

It doesn't look that ether-wake is the only command that's affected. It seems some commands in misc-progs try to filter user input, while others do not. 

Possible solutions
------------------
An obvious solution is to validate all inputs for the commands. So, in the above example, launch-ether-wake could check whether an interface exists (or whether the parameter consists of characters that are valid for interfaces) or whether the mac address is valid. 

The comment for the safe_system function https://github.com/ipfire/ipfire-2.x/blob/master/src/misc-progs/setuid.c#L56 actually that you should filter for semicolons etc. Commands like backupctrl https://github.com/ipfire/ipfire-2.x/blob/master/src/misc-progs/backupctrl.c#L25 however don't filter for semicolons, only a few chars like &&. Therefore, the checks can probably be considered insufficient. 


So, possible first steps towards improving the situation could be the following:

1. Implementing a function like safe_system(), except that it would not invoke system() but rather the binary directly, e. g. using the exec() family of functions. This avoids those injection commands. It should be used in all calls that only call a single command. In the example above, the exploit only works because safe_system() uses /bin/sh but there is no reason to call /bin/sh. Had it called etherwake directly, the exploit would not have worked.   

2. Currently, some commands try to filter unwanted characters in argv[1] before calling safe_system(), but miss some characters or don't have any filtering at all. Therefore, a central function could be implemented that would be used by all commands. Note however that this is still a blacklisting approach and thus not ideal.

3. Whitelist as much as possible. If a parameter is supposed to be a mac address, make sure it is one. Or if a parameter is expected to be only alphanumeric characters, check for this too.
Comment 1 Michael Tremer 2020-12-31 16:27:37 UTC 
Thank you for reporting this vulnerability. It helps us a lot to make IPFire more secure.

I will have a look at this and submit patches to this ticket. So far I can confirm it and I have marked this ticket at security-sensitive so that it won't be visible on the bug tracker. I will remove that mark as soon as a patch has been submitted and has been approved.
Comment 2 Michael Tremer 2021-01-06 14:57:50 UTC 
I have now come up with various fixes for these vulnerabilities which I would like to post here and I am asking for a review of them. If you agree with the changes, I will post them to the mailing list.

The lauch-ether-wake binary has been dropped and I have given etherwake CAP_NET_RAW. Therefore the web UI can now call it without need for the helper.

I have removed that commands that are composed from input variables are executed using a shell. Therefore the entire class of shell command injection should be removed. I have changed safe_system() so that it is a wrapper for the new function and not all of the code has to be changed.

After these changes, all vulnerabilities should be fixed. Please let me know if you find anything that I overlooked.

Thank you again for your help to make IPFire better software :)
Comment 3 Michael Tremer 2021-01-06 14:58:56 UTC 
Created attachment 831 [details]
0001-Drop-launch-ether-wake.patch
Comment 4 Michael Tremer 2021-01-06 14:59:12 UTC 
Created attachment 832 [details]
0002-core154-Manually-set-capabilites-for-etherwake.patch
Comment 5 Michael Tremer 2021-01-06 14:59:26 UTC 
Created attachment 833 [details]
0003-misc-progs-Introduce-run.patch
Comment 6 Michael Tremer 2021-01-06 14:59:39 UTC 
Created attachment 834 [details]
0004-misc-progs-backupctrl-Use-new-run-function.patch
Comment 7 Michael Tremer 2021-01-06 14:59:51 UTC 
Created attachment 835 [details]
0005-misc-progs-pakfire-Use-new-run-function.patch
Comment 8 Michael Tremer 2021-01-06 15:00:02 UTC 
Created attachment 836 [details]
0006-misc-progs-Add-functions-to-sanitise-input-arguments.patch
Comment 9 Michael Tremer 2021-01-06 15:00:16 UTC 
Created attachment 837 [details]
0007-misc-progs-addonctrl-Sanitise-add-on-names-before-us.patch
Comment 10 Michael Tremer 2021-01-06 15:00:29 UTC 
Created attachment 838 [details]
0008-misc-progs-extrahdctrl-Use-new-run-function.patch
Comment 11 Michael Tremer 2021-01-06 15:00:40 UTC 
Created attachment 839 [details]
0009-misc-progs-mpfirectrl-Use-new-run-function.patch
Comment 12 Michael Tremer 2021-01-06 15:00:52 UTC 
Created attachment 840 [details]
0010-misc-progs-smartctrl-Sanitise-device-name.patch
Comment 13 Michael Tremer 2021-01-06 15:01:05 UTC 
Created attachment 841 [details]
0011-misc-progs-sshctrl-Sanitise-runtime-for-tempstart.patch
Comment 14 Michael Tremer 2021-01-06 15:01:16 UTC 
Created attachment 842 [details]
0012-misc-progs-sambactrl-Remove-unused-smbsafeconfpdc-co.patch
Comment 15 Michael Tremer 2021-01-06 15:01:27 UTC 
Created attachment 843 [details]
0013-misc-progs-sambactrl-Sanitise-username.patch
Comment 16 Michael Tremer 2021-01-06 15:01:38 UTC 
Created attachment 844 [details]
0014-samba-Remove-option-to-chose-user-group-and-shell.patch
Comment 17 Michael Tremer 2021-01-06 15:01:49 UTC 
Created attachment 845 [details]
0015-samba-Add-helper-script-to-pipe-password.patch
Comment 18 ipfire 2021-01-06 19:56:31 UTC 
Hi Michael,

Currently, I only have time for a quick review. 

Patch 1: OK
Patch 2: OK
Patch 4: OK
Patch 5: OK
Patch 7: I personally would prefer calling run() instead of safe_system(), just as a matter of principle, since you only execute a single command here. Similarly, I would prefer snprintf() over sprintf(), even though you did check for overflows before. This way, people attention is not taken by these calls to sprintf() and analyzer will not produce warnings.
Patch 8: OK
Patch 9: OK

I'll have to check the rest once I have more time. To be clear, the "OK" only refers to this bug. So if the setuid binary calls a perl script, I did not check whether the perl script deals properly with the arguments or whether it has other bugs. 

Regards,
Albert
Comment 19 Michael Tremer 2021-01-07 12:17:22 UTC 
Created attachment 846 [details]
0001-misc-progs-addonctrl-Replace-all-sprintf-with-snprin.patch

Thanks for looking into this so far.

(In reply to ipfire from comment #18)
> Patch 7: I personally would prefer calling run() instead of safe_system(),
> just as a matter of principle, since you only execute a single command here.
> Similarly, I would prefer snprintf() over sprintf(), even though you did
> check for overflows before. This way, people attention is not taken by these
> calls to sprintf() and analyzer will not produce warnings.

I replaced all of those calls with snprintf(). See the attached patch.

> I'll have to check the rest once I have more time. To be clear, the "OK"
> only refers to this bug. So if the setuid binary calls a perl script, I did
> not check whether the perl script deals properly with the arguments or
> whether it has other bugs. 

This is what this ticket is about. I would like to release these fixes as soon as possible. If there is anything else in any of the scripts that are being called, that would be a new ticket and I would like to work on that after this. One at a time.
Comment 20 Michael Tremer 2021-01-25 17:19:20 UTC 
Hello Albert,

I would like to close the development cycle very soon and I would like these patches to be included to patch the problem as soon as possible for our users.

Did you have some more time to review them or do you need a little bit longer?
Comment 21 Michael Tremer 2021-01-27 21:08:37 UTC 
I have merged all patches into "next" and they are scheduled for release soon.

This bug report is also public again now.

Thank you for your help and your valuable contribution to IPFire.