What should the file be called? Should it be part of permission-lockdown or its own file?
Yes, but I don’t know how to determine if it is invalid.
You already did the right thing with error checking.
if ! [ -e "${file}" ]; then
echo "ERROR: File '${file}' does not exist!"
break
fi
if ! seq -w 000 4777 | grep -qw "${mode}"; then
echo "ERROR: Mode '${mode}' is invalid!"
break
fi
Just change from break to “continue” (meaning “continue the loop with the next iteration” rather than “break this loop right now”).
No strong opinion. Could be there.
Or own file maybe better? Otherwise the existing apparmor profile needs too many changes?
/usr/lib/security-misc/permission-lockdown currently works on folders inside /home.
The new script is more for files outside of /home.
I am not worried much since we can always refactor the code after testing it a bit.
/usr/lib/security-misc/permission-lockdown is currently only run by debian/security-misc.postinst but other hooks could call it too. (Such as a systemd unit file which runs it before sysinit.target.)
1 Like
I meant with the mode. It’s not the best way of checking it as it allows some invalid ones.
I think it would be best for it to be its own separate file. Is just “permission-hardening” a good name?
1 Like
Perfect.
1 Like
We can use this for things like SUID Disabler and Permission Hardener too.
Since the script just passes the arguments into chmod, we can wipe out all SUID binaries/libraries by adding this to permission-hardening.conf as long as an exception is made in the error checking.
/{,usr/,usr/local/}{,s}bin/ -R u-s root root
/{,usr/,usr/local/}{,s}bin/ -R g-s root root
/{,usr/,usr/local/}lib{,32,64}/ -R u-s root root
/{,usr/,usr/local/}lib{,32,64}/ -R g-s root root
2 Likes
Yes. Solving SUID Disabler and Permission Hardener too would be amazing! Yes, stuff we don’t use (such as pkexec) should surely have SUID removed. But we’ll need a SUID whitelist too? For now, we shouldn’t remove SUID from sudo, right? I don’t understand how such a whitelist would be possible with above code? Maybe first remove all SUID, then opt-in re-add sudo?
For the upcoming boot to auto login into admin vs limited user boot option (multiple boot modes for better security: persistent user | live user | persistent secureadmin | persistent superadmin | persistent recovery mode) it would be great to automatically create a configuration file snippet that whitelists sudo or removes sudo from whitelist. My idea was for boot to auto login into limited user to even have SUID on sudo removed. (And re-add when booting into admin mode.)
I would also like to have an interpreter lock opt-in. Dropping a configuration snippet (opt-in interpreter lock) vs deleting a configuration snippet (opt-out interpreter lock) would be great too for that.
1 Like
pkexec is used internally by a bunch of applications like cannot use pkexec - #3 by AnonymousUser
Yes, SUID in sudo should stay.
Yes, the commands are executed in order so as long as the sudo line is below the lines that remove SUID, we’ll be fine.
We can create a /etc/permission-hardening.d/ directory for configuration snippets. Maybe create a systemd service to create /etc/permission-hardening.d/no-sudo.conf or similar.
1 Like
Sounds good!
hidepid broke pkexec. Therefore we’re using a wrapper, pkexec wrapper. It should solve most if not all issues. I am not sure about the remaining issues with pkexec.
Maybe pkexec wrapper issues. Didn’t get to debug these yet. Could be unrelated to pkexec.
Either way, we’re not using real pkexec so pkexec can have its SUID removed until it becomes compatible with hidepid.
2 Likes
To allow for the -R option, all the variables need to be shifted over as $2 is now -R instead of the mode.
Is this a good way to fix it?
if [ "${mode}" = "-R" ]; then
recursive="-R"
mode="${owner}"
owner="${group}"
group="${capability}"
capability="$(awk '{print $6}' <<< ${line})"
fi
chmod "${recursive}" "${mode}" "${file}"
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]
1 Like
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}"
1 Like
That won’t work as the $5 is taken by capabilities. It will only work when capabilities are being set.
1 Like
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.
- It can’t store
u-sorg-sas 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
- With recursion, dpkg-statoverride will only save the value of the directory. This does nothing for if /usr/bin/sudo gets updated for example. I’ve worked around this by looping over every single file in $file if recursion is enabled, and adding each file to the database:
https://paste.debian.net/hidden/95291f85/
Number 2 seems especially bad and hacky.
1 Like
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” =~ ^[0-9a-zA-Z_-]+$ ]]; 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.
1 Like
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)
1 Like
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
1 Like
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.
1 Like
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.
- Not using
--update: dpkg-statoverride won’t apply the actual mode change. chown/chmod has still to be used manually. - Using
--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.
1 Like
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.
1 Like