Patrick
December 20, 2019, 9:15am
15
Let’s not use add_statoverride_entry for both, case of nosuid and regular modes. That is because when we’re using nosuid that is totally different. In that case we iterate over folders. For other modes, we don’t iterate but set specific modes. There is not any code that would be repeated. Therefore made that add_nosuid_statoverride_entry.
This is fixed.
Patrick:
run: dpkg-statoverride --remove /bin/
Do we want to do this?
For now, no longer doing this. That was only only a bug when using nosuid entries. Not a deliberate choice in config. If this was wanted, another entry should be added to config (similar to /boot/ 0700 root root but would have to think about mode/owner/group but there may be no fitting config file and does not seem important).
1 Like
Patrick
December 20, 2019, 9:18am
16
I don’t think we should support iterating over folders for nosuid while at the same time setting specific modes/owner/groups. To specific what I mean:
This is OK:
/bin/ nosuid
This is also OK:
/etc/permission-hardening.conf 0600 root root
This does not seem useful:
/bin/ nosuid 0600 root root
This does not seem useful:
/bin/ nosuid root root
If both nosuid and a mode/owner/group change is wanted, better add another config line.
1 Like
Patrick
December 20, 2019, 9:46am
17
We are hitting ARG_MAX
getconf ARG_MAX
more info:https://www.linuxjournal.com/article/6060
How can I prevent arguments to `xargs` from being prefixed with spaces? - Unix & Linux Stack Exchange said xargs can split it up.
Will use
done < <( find "${fso_without_trailing_slash}/" -print0 | xargs -I{} -0 stat -c "%n %a %U %G" {} )
to fix it.
1 Like
Patrick
December 20, 2019, 10:03am
18
It’s fixed. But by fixing the parsing of /lib it’s also very slow. Needs 3 minutes to finish.
(And only using dry mode, i.e. not running the dpkg-statoverwrite --add / --remove commands yet actuary. I don’t think that would change much to the worse, though.)
time sudo usr/lib/security-misc/permission-hardening
fso: /home/
real 3m3.306s
The xtrace (sudo bash -x usr/lib/security-misc/permission-hardening or setting set -x) at the top of the script looks efficient. We don’t call stat a million times and we are using bash built-ins.
1 Like
Patrick
December 20, 2019, 10:17am
19
Added some benchmarking code (but not clean, will not publish unless deemed required).
fso: /bin/ | benchmark: 00:00:01
Parsing /lib/ and /usr/lib/ takes far most of the time.
Fortunately /lib does not have any suid binaries by default on my system.
Maybe we can mount /lib as nodev,nosuid. As per Kurt Seifried - LASG / Installation we can. Then we could remove /lib from permission hardening config and safe 1 minute.
Related: (re-)mount home [and other?] with noexec (and nosuid [among other useful mount options]) for better security? - #21 by madaidan
Parsing /usr/lib though seems important.
1 Like
Patrick
December 20, 2019, 11:20am
21
I want to add a usability feature.
As user user.
stat -c "%n %a %U %G" /bin/su
/bin/su 755 root root
which su
/bin/su
I.e. su is still executable by user user even though it has suid removed. This is bad because su fails open, i.e. in weird ways. It does not report “missing suid”. What I preferred:
sudo chmod o-x /bin/su
stat -c "%n %a %U %G" /bin/su
/bin/su 754 root root
which su
su
bash: /bin/su: Permission denied
Now user user cannot execute su anymore. It is failing closed.
1 Like
Patrick
December 20, 2019, 11:31am
22
This is the script output as of now.
run: dpkg-statoverride --add --update root root 0755 /home
new_mode is either 755 or 754. I.e. has still execution permission for others or group.
For config entries stating nosuid only: Would it be a good idea to hardcode / change newmode of these to 744? I.e. to remove exeution permission for others and `group?
if [ "$new_mode" = "755" ]; then
new_mode=744
fi
if [ "$new_mode" = "754" ]; then
new_mode=744
fi
if [ "$new_mode" = "745" ]; then
new_mode=744
fi
I guess the question is:
Are there suid or guid binaries which are still useful if suid / guid has been removed from these?
1 Like
Patrick
December 20, 2019, 12:05pm
23
Could you please review if it is sane to remove suid / guid from the following binaries? @madaidan
run: dpkg-statoverride --add --update root root 755 /bin/fusermount
Does the suid default whitelist need to be expanded?
1 Like
Patrick
December 20, 2019, 12:58pm
24
Found some issue.
Dec 20 07:54:47 disp3633 permission-hardening[26043]: + seq -w 000 4777
Need to use a better regex here.
Another issue.
user@disp3633:~$ sudoedit /usr/lib/security-misc/permission-hardening
That is because there is a significant time gap between suid removal from /usr/bin and re-adding the whitelist of /usr/bin/sudo. Therefore we need to implement the whitelist another way. Not by first removing suid and then re-adding. Better to not modify file permissions of sudo (and other white listed ones) at all. Otherwise this can cause a lot bugs which are timing dependent (when other scripts use sudo).
1 Like
Patrick
December 20, 2019, 1:03pm
25
Found some issue.
Dec 20 07:54:47 disp3633 permission-hardening[26043]: + seq -w 000 4777
Need to use a better regex here.
Not perfect but functional.
committed 01:02PM - 20 Dec 19 UTC
no longer use seq due to issue
https://forums.whonix.org/t/permission-hardening… /8655/13
1 Like
Patrick
December 20, 2019, 1:59pm
27
fso: /lib/
Can reproduce manually too.
shopt -s globstar
stat -c “%n %a %U %G” /lib/**
> bash: /usr/bin/stat: Argument list too long
We are hitting ARG_MAX
getconf ARG_MAX
more info:"Argument list too long": Beyond Arguments and Limitations | Linux Journal
How can I prevent arguments to `xargs` from being prefixed with spaces? - Unix & Linux Stack Exchange said xargs can split it up.
Will use
done < <( find "${fso_without_trailing_slash}/" -print0 | xargs -I{} -0 stat -c "%n %a %U %G" {} )
to fix it.
That does not work. It will “forget” to process some files.
find /usr/bin | wc -l
1149
Added a counter.
INFO: fso_to_process: ‘/usr/bin/’ | counter: ‘575’
1 Like
Patrick
December 20, 2019, 2:50pm
29
sudo /usr/lib/security-misc/permission-hardening
sudo: /usr/bin/sudo must be owned by uid 0 and have the setuid bit set
On Qubes there is also /bin/sudo and /bin/brwap. Fixed.
committed 02:48PM - 20 Dec 19 UTC
1 Like
Patrick
December 20, 2019, 3:28pm
30
Found a way to crazy speed up things. Using find with -perm /u=s,g=s. Will add.
1 Like
It’s fine. Do what you want. You’re better than me at writing bash scripts anyway.
You can just add another rule for su:
/bin/su 0700 root root
What’s the point of reading binaries if it can’t even be executed? Maybe just give a mode of 700.
It’s fine to remove it from most of them but whitelist sudo, virtualbox, polkit, dbus and chromium. Chromium only needs suid if unprivileged user namespaces are disabled which Debian does by default.
1 Like
Btw, I noticed in the script you refer to setgid as “guid”. It’s not “guid” but “sgid”. GUID is different and may cause confusion.
1 Like
Patrick
December 20, 2019, 5:03pm
33
Could you please review if it is sane to remove suid / guid from the following binaries?
It’s fine to remove it from most of them but whitelist sudo, virtualbox, polkit, dbus and chromium. Chromium only needs suid if unprivileged user namespaces are disabled which Debian does by default.
VirtualBox has “plenty”. 6 at the moment.
In future, more/less might be added or paths might change. In that case, I would like to avoid hunting down bugs due to that.
Any reason not to add a nosuid whitelist matching feature? Could match for /virtualbox/.
I don’t see how such a matching feature could be abused. Threat model:
Btw, I noticed in the script you refer to setgid as “guid”. It’s not “guid” but “sgid”. GUID is different and may cause confusion.
Fixed in git master.
1 Like
Are any of these actually used? Is there any breakage from removing their SUID bits? If not, then we can just ignore them.
I doubt VboxNetDHCP would be used seeing as Whonix doesn’t even use DHCP at all.
Would be better to match for /usr/lib/virtualbox/ in case some random directory that just happens to have the name “virtualbox” gets whitelisted.
1 Like