Not sure I got that. Maybe change syntax of the file since we’re inventing a new file format anyhow. Have no “maybe” / optional fields in the file format. All file format entries can be mandatory. Easier to parse. Change from:
[filename] [mode] [owner] [group] [capability]
recursive yes:
y [filename] [mode] [owner] [group] [capability]
recursive no:
n [filename] [mode] [owner] [group] [capability]
How about adding “-R” to the end of an entry to make it recursive? e.g.
/{,usr/,usr/local/}{,s}bin/ u-s root root -R
We can then just use
recursive="$(awk '{print $6}' <<< ${line})"
chmod "${recursive}" "${mode}" "${file}"That won’t work as the $5 is taken by capabilities. It will only work when capabilities are being set.
I’ve decided to just use whether the entry starts with a Y to indicate recursion.
dpkg-statoverride complicates a lot of things with recursion and u-s/g-s rules.
u-s or g-s as a mode. I’ve worked around this by getting the current permission of the file and setting that as $mode:if [ "${mode}" = "u-s" ] || [ "${mode}" = "g-s" ]; then
mode="$(stat -c %a ${file})"
fi
https://paste.debian.net/hidden/95291f85/
Number 2 seems especially bad and hacky.
Added some enhancements as suggested by wooledge.org and shellcheck.
[[ “$line” =~ ^#.*$ ]] && continue
That is a good idea. However, a blacklist of bad characters is impossible. There are too many weird, even binary characters to know them all. What about all whitelist…
if ! [[ “$line” =~ [1]+$ ]]; then
?
Maybe we can safe a call to chmod / chown for better (boot) speed? dpkg-statoverride supports:
If --update is specified and path exists, it is immediately set to the new owner and mode.
--update tested. Works for me.
0-9a-zA-Z_- ↩︎
I might not be following…
user@disp1376:~$ ls -la /bin/su
-rwsr-xr-x 1 root root 63568 Jan 10 2019 /bin/su
user@disp1376:~$ stat -c %a /bin/su
4755
user@disp1376:~$ sudo dpkg-statoverride --add --update root root 0755 /bin/su
user@disp1376:~$ ls -la /bin/su
-rwxr-xr-x 1 root root 63568 Jan 10 2019 /bin/su
user@disp1376:~$ stat -c %a /bin/su
755
Do you mean you’d preferred to express things things as u-s (“human readable”) rather than octal (for example 755)? I would agree. I don’t like octal at all. I use human readable version whenever possible. Octal does not work well with my brain at all. When I see octal, I always have to double check using chmod calculator, ls and stat and then actually test things to work as expected. If dpkg-statoverride supported human readable, I’d already request to use that in the config file rather than octal.
Not bad.
When /root is owned by user root and group root and using permission “others-all” (o-rwx), then users other than root such as user user cannot read any file inside /root/some-file because the higher level folder (/root) already keeps them out.
What do we need recursion for? For example to remove SUID from all files residing in /bin? First, recursive remove all SUID from /bin. Then re-add SUID for whitelisted (specifically mentioned in config below or in a lexical higher configuration file) binaries?
Would it help if we simplified the config file syntax? Perhaps for recursion for SUID removal, don’t support the full syntax? Have specific variables or a separate config file with folders which are parses in recursive order to remove all SUID?
And then file-by-file (non-recursion) re-adding of SUID could be the existing file format? Maybe no need to express everything in the same config file? Would that help?
Not sure we should create a huge database adding every file? This could slow down things when the script runs (considering we’ll run at boot time) (and might even slow down apt upgrades?). This database could have a lot entries of files which are obsolete after release upgrade. Ever growing database. Maybe when using recursion (therefore mentioned a “special” recursion feature) it would be better to detect only actual SUID binaries and then add these to dpkg-statoverwrite --add --update? stat seems very efficient. A single call.
stat -c "%n %a" /bin/*
Quickly lists all permissions that a folder.
…
/bin/zless 755
/bin/zmore 755
/bin/znew 755
Then we’d just process those files that need it.
I also don’t think we need to recurse some folders that are already handled elsewhere such as /home. It’s mounted with nosuid/nodev anyhow.
((re-)mount home [and other?] with noexec (and nosuid [among other useful mount options]) for better security? - #2 by madaidan)
for real_file in $(find ${file})
That needs some work:
https://mywiki.wooledge.org/UsingFind
Or maybe bash globstar would be easier?
https://mywiki.wooledge.org/glob#globstar_.28since_bash_4.0-alpha.29
I only added that so the script would ignore comments in permission-hardening.conf. Whitelisting only good characters doesn’t seem that useful.
--update sounds like a better replacement for the part where it removes and adds entries again.
To save calls to chmod/chown we can check if the file is already set at that mode:
if ! [ "$(stat -c %a ${file})" = "${mode}" ]
No. chmod 755 -R /bin sets everything in /bin to 755 which will probably mess up permissions for tons of things.
But chmod u-s -R /bin will only remove the setuid bit from everything in /bin and leave other permissions the exact same.
u-s is far less likely to break things and a better approach,
I actually prefer octal over human readable. It’s simpler IMO.
Yes, but recursion isn’t needed just for keeping things out.
If for example, we want non-owner users to read everything in /usr/example/ but not execute anything, we can add:
/usr/example/ 744 root root -R
Or if we want to remove the SUID bit from everything /bin/ but still allow anyone to execute it, we can add:
/bin u-s root root -R
Yes but the setuid bit can be reset during updates if we don’t also use dpkg-statoverride recursively.
The way the syntax is now seems fine. It’s just dpkg-statoverride which is the main problem.
Yes, that’s why it’s bad.
Sounds better. Maybe something like:
stat -c "%n %a" /bin/* | awk '$2 == "4755"'
Although this will miss files like 4711.
http://lkml.iu.edu/hypermail/linux/kernel/1011.2/01436.html
This lists a few more possible infoleaks.
- /proc/<PID>/stack
- /proc/mtrr
- /proc/asound/cards
- /sys/devices/*/*/resources
- /proc/net/ptype
- /sys/kernel/slab/*/ctor
- /sys/module/*/sections/*
They’re probably mitigated by kptr_restrict anyway though.
Indeed. Since all inputs of that script are trusted. Could only be done by root users or if this is protected by apparmor-profile-everything even - how do we call this - superroot users?
Just useful in case a user accidentally copies weird characters there.
--update isn’t for updating dpkg-statoverride database.
user@disp5873:~$ sudo dpkg-statoverride --add root root 0755 /bin/su
user@disp5873:~$ sudo dpkg-statoverride --add --update root root 0755 /bin/su
dpkg-statoverride: error: an override for ‘/bin/su’ already exists; aborting
As per dpkg-statoverride man page --update can only be used in combination with --add.
--update: dpkg-statoverride won’t apply the actual mode change. chown/chmod has still to be used manually.--update: dpkg-statoverride runs “chown/chmod” for us.Indeed.
Indeed. So let’s use stat first and then only process the files that need processing.
I’ve wrote a snippet to identify SUID. Only one external call to stat.
There are no octal permissions with 2 or less numbers.
There are no octal permissions more than 4 numbers.
It’s either 3 numbers for non-SUID or 4 for SUID.
Therefore looking at the string length of the octal permission 3 vs 4 (and having error cases in case its ever less or more).
#!/bin/bash
set -e
while read -r line ; do
if ! read -r file_name existing_mode ; then
continue
fi
string_length_existing_mode="${#existing_mode}"
if [ "$string_length_existing_mode" = "4" ]; then
echo "file_name: '$file_name' | existing_mode: '$existing_mode'"
fi
if [ "$string_length_existing_mode" -gt "4" ]; then
error_code=2
echo "error 2..." >&2
continue
fi
if [ "$string_length_existing_mode" -lt "3" ]; then
error_code=3
echo "error 3..." >&2
continue
fi
done < <( stat -c "%n %a" /usr/bin/* )
file_name: ‘/usr/bin/at’ | existing_mode: ‘6755’
file_name: ‘/usr/bin/bsd-write’ | existing_mode: ‘2755’
file_name: ‘/usr/bin/chage’ | existing_mode: ‘2755’
file_name: ‘/usr/bin/crontab’ | existing_mode: ‘2755’
file_name: ‘/usr/bin/dotlockfile’ | existing_mode: ‘2755’
file_name: ‘/usr/bin/expiry’ | existing_mode: ‘2755’
file_name: ‘/usr/bin/newgidmap’ | existing_mode: ‘4755’
file_name: ‘/usr/bin/newuidmap’ | existing_mode: ‘4755’
file_name: ‘/usr/bin/passwd’ | existing_mode: ‘4755’
file_name: ‘/usr/bin/pkexec.security-misc-orig’ | existing_mode: ‘4755’
file_name: ‘/usr/bin/sudo’ | existing_mode: ‘4755’
On those we could run for example…
chmod ug-s /usr/bin/at
stat -c "%a" /usr/bin/at
755
And then feed that to dpkg-statoverwrite.
Does that help?
If not, could you please (half/somewhat/mostly) populate the permission files (perhaps just examples of each case)? Maybe that would help me to understand the issue better. Then I can see what I can do to finish that script.
I think we should just restrict it to root and not go any further. If we restricted it from even root, it would be a pain to configure.
The mode for permission-hardening.conf is already set at 600 (read-write for root, nothing for others).
That sounds alright then.
Ah, that makes sense. Misunderstood the first time.
That’s not great. There’s still the sticky bit (e.g. 1755) which this script would misidentify as SUID.
It would if the sticky bit is accounted for.
Luckily the bash test built-in can check for any of these.
The following script sets the following variables:
suidsgidstickybitTo either true or false.
#!/bin/bash
set -e
while read -r line ; do
if ! read -r file_name existing_mode ; then
continue
fi
string_length_existing_mode="${#existing_mode}"
suid=false
sgid=false
stickybit=false
if [ "$string_length_existing_mode" = "4" ]; then
if test -u "$file_name" ; then
suid=true
echo "suid - file_name: '$file_name' | existing_mode: '$existing_mode'"
fi
if test -g "$file_name" ; then
sgid=true
echo "gid - file_name: '$file_name' | existing_mode: '$existing_mode'"
fi
if test -k "$file_name" ; then
stickybit=true
echo "sticky - file_name: '$file_name' | existing_mode: '$existing_mode'"
fi
fi
if [ "$string_length_existing_mode" -gt "4" ]; then
error_code=2
echo "error 2..." >&2
continue
fi
if [ "$string_length_existing_mode" -lt "3" ]; then
error_code=3
echo "error 3..." >&2
continue
fi
done < <( stat -c "%n %a" /usr/bin/* )
suid - file_name: ‘/usr/bin/at’ | existing_mode: ‘6755’
gid - file_name: ‘/usr/bin/at’ | existing_mode: ‘6755’
gid - file_name: ‘/usr/bin/bsd-write’ | existing_mode: ‘2755’
gid - file_name: ‘/usr/bin/chage’ | existing_mode: ‘2755’
gid - file_name: ‘/usr/bin/crontab’ | existing_mode: ‘2755’
gid - file_name: ‘/usr/bin/dotlockfile’ | existing_mode: ‘2755’
gid - file_name: ‘/usr/bin/expiry’ | existing_mode: ‘2755’
suid - file_name: ‘/usr/bin/newgidmap’ | existing_mode: ‘4755’
suid - file_name: ‘/usr/bin/newuidmap’ | existing_mode: ‘4755’
suid - file_name: ‘/usr/bin/passwd’ | existing_mode: ‘4755’
suid - file_name: ‘/usr/bin/pkexec.security-misc-orig’ | existing_mode: ‘4755’
suid - file_name: ‘/usr/bin/sudo’ | existing_mode: ‘4755’
Does that help?
Yes. This should be added to permission-hardening to remove all SUID/SGID bits.
Maybe instead of using
/bin u-s root root -R
we can add a nosuid argument e.g.
/bin nosuid
which would use the script above to find and remove all SUID bits and add entires to dpkg-statoverride.
I’ve tried implementing this. What do you think of Debian paste error?
I’ve simplified your script above and used it in this. I use stat -c "%n %a %U %G" $(find ${file} -type f) instead of stat -c "%n %a %U %G" ${file}/* as the * won’t find files like /usr/bin/dir/file. It will only look for /usr/bin/file. $(find ...) probably isn’t the correct way to do this though.
I’ve also used --update for dpkg-statoverride instead of chmod/chown.
The while read -r line part in set_file_perms is still breaking due to it parsing empty lines and I don’t know how to fix that.
If you want match sub folders one option is bash’s:
shopt -s globstar
And then use ** instead of *, I think. Gotta test. More comments soon.
There’s a global variable
nosuid=true
and once it’s set it is never reset/unset/set to something else as far as I can see.
file globstartest
#!/bin/bash
set -x
shopt -s globstar
for file_name in /usr/share/** ; do
true "$file_name"
done
./globstartest
excerpt:
- for file_name in /usr/share/**
- true /usr/share/zuluCrypt/translations/zuluCrypt-gui/de_DE.qm
- for file_name in /usr/share/**
- true /usr/share/zuluCrypt/translations/zuluCrypt-gui/en_US.qm
- for file_name in /usr/share/**
- true /usr/share/zuluCrypt/translations/zuluCrypt-gui/fr_FR.qm
- for file_name in /usr/share/**
- true /usr/share/zuluCrypt/zuluCrypt.pdf
I made some fixes: Debian paste error
It seems to work. Need to test it more though.
Example of a config file: Debian paste error
This doesn’t work properly for some reason. Some SUID binaries still exist.
root@host:/usr/lib/security-misc# find / -perm /4000 -user root 2>/dev/null
/bin/umount
/bin/mount
/bin/su
/usr/bin/sudo
/usr/bin/firejail
/usr/bin/gpasswd
/usr/bin/bwrap
/usr/bin/passwd
/usr/bin/chsh
/usr/bin/pkexec.security-misc-orig
/usr/lib/policykit-1/polkit-agent-helper-1
/usr/lib/dbus-1.0/dbus-daemon-launch-helper
root@host:/usr/lib/security-misc#
Some directories like /usr/local/lib/python2.7/site-packages are SGID and removing that might break things.
When executing the script I also get some errors but they don’t seem important.
root@host:/usr/lib/security-misc# ./permission-hardening
dpkg-statoverride: warning: stripping trailing /
dpkg-statoverride: warning: no override present
dpkg-statoverride: warning: stripping trailing /
dpkg-statoverride: warning: no override present
suid - file_name: '/usr/bin/sudo' | existing_mode: '4755'
dpkg-statoverride: warning: stripping trailing /
dpkg-statoverride: warning: no override present
dpkg-statoverride: warning: stripping trailing /
dpkg-statoverride: warning: no override present
ERROR: File '/lib32/' does not exist!
dpkg-statoverride: warning: stripping trailing /
dpkg-statoverride: warning: no override present
ERROR: File '/usr/lib32/' does not exist!
ERROR: File '/usr/lib64/' does not exist!
dpkg-statoverride: warning: stripping trailing /
dpkg-statoverride: warning: no override present
ERROR: File '/usr/local/lib32/' does not exist!
ERROR: File '/usr/local/lib64/' does not exist!
stat: cannot stat '/usr/bin/bwrap/**': Not a directory
stat: cannot stat '/usr/lib/policykit-1/polkit-agent-helper-1/**': Not a directory
stat: cannot stat '/usr/lib/dbus-1.0/dbus-daemon-launch-helper/**': Not a directory
root@host:/usr/lib/security-misc#
There is still a code path where unset nosuid might not be set.
if [ "${mode}" = "nosuid" ]; then
nosuid=true
I’d suggest to add nosuid="" or nosuid=false on top. I.e.
nosuid=""
if [ "${mode}" = "nosuid" ]; then
nosuid=true
Then we could be sure it is reset in any case.
Could you please add the license header?
Need more debugging. I recocommend to set -x at the top or to echo the dpkg-statoverwrite command before executing it.
Because it’s a folder.
if ! [ -e "${file}" ]; then
will return non-zero, i.e. throw that error message. To check for folder
if [ -d "${file}" ]; then
Also we might not call it file but file_system_object since it could be anything (file, folder, device, socket). Also file is a non-ideal name since file is a standard command line tool.
It is probably running:
stat -c "%n %a %U %G" /usr/bin/bwrap/**
stat: cannot stat ‘/usr/bin/bwrap/**’: Not a directory
This won’t work. add_statoverride_entry needs to check if it is a file or a folder.