Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hardening file permissions #132

Closed
monsieuremre opened this issue Oct 23, 2023 · 29 comments
Closed

Hardening file permissions #132

monsieuremre opened this issue Oct 23, 2023 · 29 comments

Comments

@monsieuremre
Copy link
Contributor

Hardening file permissions and removing suid bits from binaries are quite different tasks. Handling them in the same service is not optimal because on of the two is significantly simple and breaks nothing. That is file permissions. These two functionalities have to be seperated.

Manually removing suid bits essentially is not optimal because we would have to maintain a long whitelist. This has no place here. This can be implemented in the apparmor-profiles-everything repo by removing the set_suid capability from the init profile and having seperate profiles for all suid applications. These can be ported from https://github.com/roddhjav/apparmor.d.

When it comes to file permission hardneing, the following hardening options should be applied upon install and be reverted on uninstall:

    # Restrict access to compilers from user account to prevent creation of malicious binaries
    # Even if they are not there. When they are installed in the future, these will apply, because of the override.
    dpkg-statoverride --add --update root root 0700 /usr/bin/as
    dpkg-statoverride --add --update root root 0700 /usr/bin/g++
    dpkg-statoverride --add --update root root 0700 /usr/bin/gcc
    dpkg-statoverride --add --update root root 0700 /usr/bin/cc
    dpkg-statoverride --add --update root root 0700 /usr/bin/clang

    # Protect config files and cron
    dpkg-statoverride --add --update root root 0400 /etc/cups/cupsd.conf
    dpkg-statoverride --add --update root root 0600 /boot/grub/grub.cfg
    dpkg-statoverride --add --update root root 0600 /etc/syslog.conf
    dpkg-statoverride --add --update root root 0700 /root/.ssh
    dpkg-statoverride --add --update root root 0600 /etc/crontab
    dpkg-statoverride --add --update root root 0700 /etc/cron.d
    dpkg-statoverride --add --update root root 0700 /etc/cron.daily
    dpkg-statoverride --add --update root root 0700 /etc/sudoers.d
    dpkg-statoverride --add --update root root 0700 /etc/cron.hourly
    dpkg-statoverride --add --update root root 0700 /etc/cron.weekly
    dpkg-statoverride --add --update root root 0700 /etc/cron.monthly

    # Make sure CISOfy recommended options are applied to the following
    dpkg-statoverride --add --update root root 0644 /etc/group
    dpkg-statoverride --add --update root root 0644 /etc/group-
    dpkg-statoverride --add --update root root 0644 /etc/hosts.allow
    dpkg-statoverride --add --update root root 0644 /etc/hosts.deny
    dpkg-statoverride --add --update root root 0644 /etc/issue
    dpkg-statoverride --add --update root root 0644 /etc/issue.net
    dpkg-statoverride --add --update root root 0644 /etc/motd
    dpkg-statoverride --add --update root root 0644 /etc/passwd
    dpkg-statoverride --add --update root root 0644 /etc/passwd-

    # Protect root, boot, home
    dpkg-statoverride --add --update root root 0700 /root
    dpkg-statoverride --add --update root root 0700 /boot
    dpkg-statoverride --add --update root root 0755 /home

   # Each users home directory to himself
    for user in $(ls /home); do
        if [ "$user" != "lost+found" ]; then
            dpkg-statoverride --add --update $user $user 0700 /home/$user
        fi
    done

    # To not leak information
    dpkg-statoverride --add --update root root 0700 /usr/lib/modules
    dpkg-statoverride --add --update root root 0700 /usr/src
    dpkg-statoverride --add --update root root 0700 /lib/modules

As I said, this is to be done upon install. No daemon or service is needed. We will revert these on uninstall.

@adrelanos
Copy link
Member

@adrelanos
Copy link
Member

Hardening file permissions and removing suid bits from binaries are quite different tasks. Handling them in the same service is not optimal because on of the two is significantly simple and breaks nothing. That is file permissions. These two functionalities have to be seperated.

Isn't SDPH very suitable because it already has the required infrastructure for enabling/disabling compiler lock, interpreter lock?

Enabling/disabling the various functionalities could be governed through /etc/permission-hardening.d settings folder?

Manually removing suid bits essentially is not optimal because we would have to maintain a long whitelist.

The mentioned settings folder already contains such a whitelist.

This has no place here. This can be implemented in the apparmor-profiles-everything repo by removing the set_suid capability from the init profile and having seperate profiles for all suid applications.

I don't see apparmor-profiles-everything going anywhere anytime soon as the former contributor is no longer active. It had unresolved issues that nobody knew how to fix. So I don't think this is a realistic solution.

https://github.com/roddhjav/apparmor.d

Interesting.

As I said, this is to be done upon install. No daemon or service is needed. We will revert these on uninstall.

It needs to be configurable so one can user compilers, interpreters even without uninstalling security-misc.

@monsieuremre
Copy link
Contributor Author

monsieuremre commented Oct 24, 2023

We are already mounting most anything with nosuid. The only place left for a suid binary is under /usr where the user explicitly has to install programs with apt. Then this is a risk the user is willing to take. The suid bit in these programs serve a purpose mostly. To protect from suid privilege escalation exploits, the option would be checking if there is an apparmor profile being enforced for the suid binaries. This is a more robust solution. For example, I see that the anon-apps-config removes the suid bit from /bin/ping. Debian already ships ping with a dedicated profile, that is already enforced:

include <tunables/global>
profile ping /{usr/,}bin/{,iputils-}ping {
  include <abstractions/base>
  include <abstractions/consoles>
  include <abstractions/nameservice>

  capability net_raw,
  capability setuid,
  network inet raw,
  network inet6 raw,

  /{,usr/}bin/{,iputils-}ping mixr,
  /etc/modules.conf r,

  # Site-specific additions and overrides. See local/README for details.
  include if exists <local/bin.ping>
}

With this profile, having the capability to set uid would not be something much exploitable. The binary can not the anything at all almost, it just gets the ping.

It needs to be configurable so one can user compilers, interpreters even without uninstalling security-misc.

This makes it so that running compilers require privilege. That is, one would have to sudo gcc instead of gcc. This of course might not be optimal, but still usable.

And also, what exactly is the point in having a service run? We can just add a hook for boot and a post invoke hook for apt.

@adrelanos
Copy link
Member

We are already mounting most anything with nosuid. The only place left for a suid binary is under /usr where the user explicitly has to install programs with apt. Then this is a risk the user is willing to take.

If you search for SUID, SGID there is already a lot going on. And a lot of SUID, SGID are really non-essential in context of Kicksecure or single users systems, which are most systems nowadays.

The suid bit in these programs serve a purpose mostly.

Yes but some are from a different time period. Or look at this list. passwd, chage, expiry, ... these as the wiki opinionates:

These tools probably are used much nowadays on Linux desktop single user computers. If you need any of this, you are better off using root.

To protect from suid privilege escalation exploits, the option would be checking if there is an apparmor profile being enforced for the suid binaries.

I am not looking forward to write, maintain, debug apparmor profiles for very little used tools such as passwd, chage, expiry, etc. which are better run as root / with sudo.

This is a more robust solution.

Not sure more robust. It's much harder to maintain AppArmor profiles. I spend a few days today trying to harden the Tor Browser apparmor profile and still not sure everything is functional including its internal updater.

For example, I see that the anon-apps-config removes the suid bit from /bin/ping. Debian already ships ping with a dedicated profile, that is already enforced:

nosuid is even more secure than AppArmor.

With this profile, having the capability to set uid would not be something much exploitable. The binary can not the anything at all almost, it just gets the ping.

Relying on the security of AppArmor when not needed.

Counter to the advice by security researcher Solar Designer, quote QubesOS/qubes-issues#2695

Ideally, there should be no SUID binaries reachable from the user account, as otherwise significant extra attack surface inside the VM is exposed (dynamic linker, libc startup, portions of Linux kernel including ELF loader, etc.)


It needs to be configurable so one can user compilers, interpreters even without uninstalling security-misc.

This makes it so that running compilers require privilege. That is, one would have to sudo gcc instead of gcc. This of course might not be optimal, but still usable.

Not great. Better to re-enable compiler access than running compilers with root. Because that leads to follow-up issues.
Maybe source code refusing to compile or failing to compile (missing environment variables, different PATH, different user, missing configs/files) and confusing resulting file permissions.

And also, what exactly is the point in having a service run? We can just add a hook for boot and a post invoke hook for apt.

Depends what you mean by a service. In context of systemd, a service an be a simple Type=oneshot which just runs the script at the appropriate time during the boot process. It's not a long running process, daemon, that's indeed not implemented and not needed in this case. The systemd unit ("service") is the hook at boot time.

APT post invoke hooks, can, should be used sparingly. These aren't used much. And not needed here. It's already implemented though a systemd Type=oneshot unit that also runs at package installation time. So unless we're hitting a limitation of APT / init (systemd), APT post invoke hooks aren't needed. The init script way is the more common, appropriate one.

@monsieuremre
Copy link
Contributor Author

Hmm, I see. Yes it makes sense. For compilers too. A shell script under /usr/bin like toggle compiler use can be made. I will look into this.

@monsieuremre
Copy link
Contributor Author

Then I have to ask: why is the service disabled by default? Any bugs? Do we have any problems or some missing functionality? If thats the case I will have a look at it. Else I can just create a pull with my extended new stuff for the config file.

@adrelanos
Copy link
Member

issue with SUID Disabler: Sometimes a Qubes VM does not start up.

issue references:

Yeah. That one blocked the progression into enabling this by default. That could use some help... Any idea?

Good that you asked. Now I am looking at the issue with a fresh mind and new ideas.

Does some disabled SUID that Qubes (indirectly through a chain) somehow requires slow up the Qubes Template or App Qubes boot process?

Or is there nothing wrong with the script itself, with the SUIDs its disabling and simply the execution sometimes takes too long on slow computers so the Qubes boot process times out.

No other known bugs or missing functionality.

PR with extended functionality would be appreciated.

@adrelanos
Copy link
Member

Maybe independently from that issue, maybe the script should be speed up by launching the actual commands it executes into the background (command &). It of course would collect the pids, wait for their execution and check their exit codes.

But then it might also not speed up a lot presumably dpkg-statoverride has a database locking mechanism to avoid parallel execution damaging its database. In theory it could even get slower due to the competing dpkg statoverride database locking requests.

sudo /usr/libexec/security-misc/permission-hardening | grep "run:"
run: dpkg-statoverride --add --update root root 0755 /usr/bin/passwd
run: dpkg-statoverride --add --update root root 0755 /bin/passwd
run: dpkg-statoverride --add --update root root 745 /bin/mount
run: dpkg-statoverride --add --update root root 745 /usr/bin/mount
run: dpkg-statoverride --add --update root root 0755 /home
run: dpkg-statoverride --add --update user user 0700 /home/user
run: dpkg-statoverride --add --update root root 0700 /root
run: dpkg-statoverride --add --update root root 0700 /boot
run: dpkg-statoverride --add --update root root 0600 /etc/permission-hardening.d
run: dpkg-statoverride --add --update root root 0700 /lib/modules
run: dpkg-statoverride --add --update root root 744 /bin/umount
run: dpkg-statoverride --add --update root root 744 /bin/su
run: dpkg-statoverride --add --update root tty 744 /bin/write
run: dpkg-statoverride --add --update root crontab 744 /bin/crontab
run: dpkg-statoverride --add --update root shadow 744 /bin/expiry
run: dpkg-statoverride --add --update root root 744 /bin/gpasswd
run: dpkg-statoverride --add --update root shadow 744 /bin/chage
run: dpkg-statoverride --add --update root tty 744 /bin/wall
run: dpkg-statoverride --add --update root root 744 /bin/chfn
run: dpkg-statoverride --add --update root root 744 /bin/newgrp
run: dpkg-statoverride --add --update root root 744 /bin/chsh
run: dpkg-statoverride --add --update root root 0755 /bin/ping
run: setcap -r /bin/ping

Yet another solution would be not running SUID Disabler at boot time and only at package installation and upgrade time. That would be a bit less secure because it wouldn't revert any changes at boot time. The question is anyhow why these permissions would be incorrect except if root changed these.

For fighting malware it seems to be the wrong tool. That's more into verified boot / dmverity. Related:
https://www.kicksecure.com/wiki/Verified_Boot

@adrelanos
Copy link
Member

5f4222c

@adrelanos
Copy link
Member

dpkg-statoverride --add --update root root 0700 /root/.ssh

Already implicitly covered by the existing configuration:

/root/ 0700 root root

dpkg-statoverride --add --update root root 0700 /root

Duplicate.

dpkg-statoverride --add --update root root 0700 /boot
dpkg-statoverride --add --update root root 0755 /home

Existing configuration already has:

/home/ 0755 root root
/home/user/ 0700 user user
/root/ 0700 root root
/boot/ 0700 root root

Each users home directory to himself
for user in $(ls /home); do

Need to make sure that is safe. (Weird names in /home. Maybe not likely.)
Need a test if it is a folder.

    if [ "$user" != "lost+found" ]; then

Even worth excluding lost+found?

        dpkg-statoverride --add --update $user $user 0700 /home/$user

A loop to cover all users and not just /home/user would be nice. Though, not sure it's needed. See:
/usr/libexec/security-misc/permission-lockdown

dpkg-statoverride --add --update root root 0700 /lib/modules

Already got:

/lib/modules/ 0700 root root

@adrelanos
Copy link
Member

dpkg-statoverride --add --update root root 0400 /etc/cups/cupsd.conf

Could use whole folder /etc/cups?


Please test, add:
https://github.com/Kicksecure/security-misc/blob/master/etc/permission-hardening.d/30_default.conf

@monsieuremre
Copy link
Contributor Author

dpkg-statoverride --add --update root root 0700 /root/.ssh

If I'm not mistaken, implicit coverage only applies to the folder .ssh itself and not its subdirectories. But I'm going to have to test that.

Need to make sure that is safe. (Weird names in /home. Maybe not likely.)

I'm on it.

Could use whole folder /etc/cups?

I don't think so. Some files there need to be world readable for printers to work.

@monsieuremre
Copy link
Contributor Author

# Each users home directory to himself
  for user in $(dir /home); do # lists directories only
      if [ grep -q "$user" /etc/passwd ]; then # check if user exists
          dpkg-statoverride --add --update $user $user 0700 /home/$user
      fi
  done

I can create a pull. Where should I add this?

@adrelanos
Copy link
Member

# Each users home directory to himself
  for user in $(dir /home); do # lists directories only
      if [ grep -q "$user" /etc/passwd ]; then # check if user exists
          dpkg-statoverride --add --update $user $user 0700 /home/$user
      fi
  done

I can create a pull. Where should I add this?

Even needed because there is /usr/libexec/security-misc/permission-lockdown?

@adrelanos
Copy link
Member

Otherwise seems more suitable in permission-lockdown than permission-hardening.

Then permission-lockdown should be the only script that locks down home folder permissions and permission-hardening should stay out of it for a cleaner design.

@monsieuremre
Copy link
Contributor Author

then I assume /home/user/ 0700 user user this has no place in permission hardening as well. It makes big assumptions and renders this package non-universal. We can get the same functionality with write I will create in a moment.

@monsieuremre
Copy link
Contributor Author

Does permission_lockdown get executed only once, or at all? This home directory loop is simple and necessary. It cannot be done with the config file because directory names are not static. This should be added as a function in permission hardening.

@adrelanos
Copy link
Member

then I assume /home/user/ 0700 user user this has no place in permission hardening as well.

Correct.

It makes big assumptions and renders this package non-universal.

Unimportant detail... Though, at least it failed gracefully with just a notification. Something like:

INFO: fso: '/home/user' - does not exist. This is likely normal.

In other words, non-existing folders aren't an issue.

@adrelanos
Copy link
Member

adrelanos commented Oct 26, 2023

Does permission_lockdown get executed only once, or at all?

Every time at package install/update time.

This home directory loop is simple and necessary. It cannot be done with the config file because directory names are not static.

True.

This should be added as a function in permission hardening.

Why not a function in permission-lockdown?

Thereby the logic in permission-hardening could stay generic, simpler code which is already a lot.

@monsieuremre
Copy link
Contributor Author

The problem isn't non existing files. It is that only the user user would get the protections and other users would just not get anything. If you are ok with the idea, the permission hardening script can just execute my loop as well after having pares the config files. This ensures home directory protections for everyone. I can create a reverse loop for the undo script. Would you be willing to merge something like this?

And I also have to ask: what is the point of permisison-lockdown. It is prone to several things and has unnecessary complexity. A simple chmod -R would do the same thing as the loop there.

@monsieuremre
Copy link
Contributor Author

Why not a function in permission-lockdown?
Thereby the logic in permission-hardening could stay generic, simpler code which is already a lot.

Can do. On it.

@adrelanos
Copy link
Member

And I also have to ask: what is the point of permisison-lockdown. It is prone to several things and has unnecessary complexity. A simple chmod -R would do the same thing as the loop there.

It selectively applies these restrictions to user home folders only if they haven't been locked down before, preventing repetitive changes, preventing undoing user customization. If a user set up customized permissions then a chmod -R could undo a lot work.

All features I intent to design in a way that these can be disabled by the system administrator, don't annoy the user, don't restrict customization, don't get into the way of advanced use cases.

@monsieuremre
Copy link
Contributor Author

I did a better implementation. We can also cover all files, but setting them to 600 would render them unexecutable. I still implemented that part too, and commented it out.

@adrelanos
Copy link
Member

This can also be implemented as a script pair. lock_compiler and lock_compiler_undo

Permission Hardener is script that parses configuration files located in /etc/permission-hardening.d. This permission hardening process is triggered either at the time of package installation or when manually executed by the user.

Now we'd like to enhance the script by providing users with the ability to enable or disable specific functionalities. Namely, enabling or disabling the compiler or interpreter lock.

Is it common practice in Debian for settings management scripts/programs to create or delete configuration snippets within .d folders? Are there any standard guidelines or examples to follow in this regard?

@adrelanos
Copy link
Member

This can also be implemented as a script pair. lock_compiler and lock_compiler_undo

Wondering about the names of the script...

- sign better than _?

We could also move permission-hardener (and permission-hardener-undo) from /usr/libexec to /usr/bin to make these scripts more easily accessible.

While we are at it we could even rename the horribly long name SUID Disabler and Permission Hardener (and wiki page title) to something more catchy.

Or even manage to introduce a globally unique acronym that then users can easily find on search engines.

Let's please brainstorm names and a command line parameter design.

  • permission-hardener all on
  • permission-hardener all off
  • permission-hardener suid on [1]
  • permission-hardener suid off
  • permission-hardener caps on [1] [2]
  • permission-hardener caps off
  • permission-hardener compiler on
  • permission-hardener compiler off
  • permission-hardener interpreter on
  • permission-hardener interpreter off

Even permission-hardener is a lot to type.


[1] Not sure I'd spent effort to implement that. Might not be worth it. Then all off seems to cover that.
[2] capabilities (ping)

@monsieuremre
Copy link
Contributor Author

This can also be implemented as a script pair. lock_compiler and lock_compiler_undo

Wondering about the names of the script...

- sign better than _?

We could also move permission-hardener (and permission-hardener-undo) from /usr/libexec to /usr/bin to make these scripts more easily accessible.

While we are at it we could even rename the horribly long name SUID Disabler and Permission Hardener (and wiki page title) to something more catchy.

Or even manage to introduce a globally unique acronym that then users can easily find on search engines.

Let's please brainstorm names and a command line parameter design.

* `permission-hardener all on`

* `permission-hardener all off`

* `permission-hardener suid on` [1]

* `permission-hardener suid off`

* `permission-hardener caps on` [1] [2]

* `permission-hardener caps off`

* `permission-hardener compiler on`

* `permission-hardener compiler off`

* `permission-hardener interpreter on`

* `permission-hardener interpreter off`

Even permission-hardener is a lot to type.

[1] Not sure I'd spent effort to implement that. Might not be worth it. Then all off seems to cover that. [2] capabilities (ping)

This is great stuff. I am going to have look at what I can do.

@adrelanos
Copy link
Member

So we don't have to parse on/off for each, best to make a syntax similar to systemctl. Here:

  • permission-hardener enable all
  • permission-hardener disable all
  • permission-hardener enable compiler
  • permission-hardener disable compiler
  • permission-hardener enable interpreter
  • permission-hardener disable interpreter

@monsieuremre
Copy link
Contributor Author

Hmm. To be honest this is no piece of cake to implement. I am not sure if this would be the right approach either. This is actually extremely simple to implement in a full system apparmor policy. Want to run compilers -> gotta use sudo. But there being profiles for allowed compilers makes sure we dont open a larger attack surface. This appoach can also work but I have to think a simpler way to allow the user. By default we should block access to all of them either way.

@monsieuremre
Copy link
Contributor Author

For simplicity, using the related pull from now on for further discussions on the topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants