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.

Sign up for GitHub

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

Jump to bottom

Better Secure Remount #152

Closed
wants to merge 10 commits into from
Closed

Better Secure Remount #152

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c019c2f
remount-secure.service
monsieuremre Nov 4, 2023
85c450f
secure-remount
monsieuremre Nov 4, 2023
3763235
generate remount config on install
monsieuremre Nov 4, 2023
fa61337
generate-secure-remount-config
monsieuremre Nov 4, 2023
8ce90bc
/boot/efi super duper hardening and file system correction
monsieuremre Nov 4, 2023
3284b5a
before
monsieuremre Nov 4, 2023
c42e6b2
secure-remount
monsieuremre Nov 4, 2023
c0a5fab
don't force ext4
monsieuremre Nov 4, 2023
9959688
config isnt executable
monsieuremre Nov 4, 2023
7a5e745
Merge branch 'master' into better-secure-remount
adrelanos Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions debian/security-misc.postinst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ pam-auth-update --package
/usr/libexec/security-misc/permission-lockdown
permission_hardening

/usr/libexec/security-misc/generate-secure-remount-config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why (only) at package install time and not a boot time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is not expected for the partitioning scheme of a system to change. This would normally only happen with a fresh install or a reinstall. And that would guarantee the package being reinstalled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A system administrator might change /etc/fstab at any time.

For example, Qubes has a package that ships /etc/fstab and there have been updated to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this at boot time as discussed. (But does not hurt also doing this at package install time - makes this easier to debug. But then the systemd unit will take care of it anyway.)


## https://phabricator.whonix.org/T377
## Debian has no update-grub trigger yet:
## https://bugs.debian.org/481542
Expand Down
12 changes: 12 additions & 0 deletions lib/systemd/system/remount-secure.service
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[Unit]
adrelanos marked this conversation as resolved.
Show resolved Hide resolved
Description=Remount partitions with hardened mount options.
After=sysinit.target
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something similar to this also applies:
https://github.com/Kicksecure/security-misc/pull/148/files#r1380569447

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do this as early as possible (for better security, avoid race conditions) something like this would be desirable:

Before=sysinit.target
After=?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally something like...

[Unit]
After=sysinit.target
Before=basic.target

[Install]
WantedBy=basic.target

But this isn't possible.

basic.target: Found ordering cycle on testcmd.service/start

Looking for a solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot important stuff happening during sysinit.target. To see:
systemctl list-dependencies sysinit.target

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I can try, but might not work. I don't know when we do all the mounting the first time. We don't want to be before that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so those won't work. I still took it a little earlier. We do the remounting before networking.service and dbus.service. Before those started, malware doesn't have much of a chance probably? I think. We can identify more 'sensitive' services like these to put them in the before condition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux. There was "no answer" to my question how to have a systemd unit do the mount options hardening without race conditions:

But maybe a new systemd target could be invented one that runs after sysinit.target but before basic.target. systemd is probably flexible enough to support such a use case. The question is how.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the critical point. Can it be done without race conditions (which broke the very old implementation and made me even port to dracut).

## so that these services don't fail to start
Before=systemd-logind.service power-profiles-daemon.service switcheroo-control.service

[Service]
Type=oneshot
ExecStart=/usr/libexec/security-misc/secure-remount

[Install]
WantedBy=sysinit.target
67 changes: 67 additions & 0 deletions usr/libexec/security-misc/generate-secure-remount-config
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#!/bin/bash

config_file="/usr/lib/security-misc_secure-remount.conf"

if [ -f $config_file ]
then
rm $config_file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also just unconditionally delete it using rm -f?

fi
touch $config_file
chmod u+x $config_file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Why make it executable? Later the config file is used with mount --fstab $config_file --all. The config file itself is never executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an actual oopsie. That line will go.


echo "/ / ext4 defaults,remount 0 2" >> $config_file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding ext4 is a regression. The existing implementation handles that better where the file system handling is generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system can be read from fstab directly. Or better putting none as the filesystem would result in the same behavior with the current implementation for binds. I have to try it also for remounts, but I think that would work too. So we can just put none and have the same result with the current implementation. Will report after testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and as predicted, using none results in using the previous filesystem on that directory, so the exact same functionality as our current implementation. Pushed the changes.

echo "/dev/shm /dev/shm ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file
echo "/dev /dev ext4 defaults,nosuid,noexec,remount 0 2" >> $config_file
echo "/run /run ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file

if grep --quiet " /boot " /etc/fstab
adrelanos marked this conversation as resolved.
Show resolved Hide resolved
then
echo "/boot /boot ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file
else
echo "/boot /boot ext4 defaults,nosuid,nodev,noexec,bind 0 2" >> $config_file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work on systems without /boot (containers / KVM direct kernel boot)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think /boot exists as a directory in any case. This isn't much different from our current approach. If it is not a partition, we bind the directory. Would work with direct kernel boot as well.

fi

if grep --quiet " /boot/efi " /etc/fstab
then
echo "/boot/efi /boot/efi vfat defaults,nosuid,nodev,noexec,umask=0077,remount 0 2" >> $config_file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be avoided by doing the same for /boot which then would cover /boot/efi as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On efi systems /boot/efi is automatically a seperate partition than /boot. This is the debian default installer behavior, and is also recommended when installing manually, for backwards compatibility with legacy boot. So /boot/efi is mounted seperately, on a different literal partition. That's why it has to be seperately remounted. If the system isn't uefi, we don't do the remounting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually correction, it is not only recommended, it is mandatory that /boot/efi is a seperate partition. Because the file system has to be vfat apparently. So yes, mounting /boot won't cover /boot/efi in any case if it exists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we won't run into the "double mount" issues we had before.

fi

if grep --quiet " /home " /etc/fstab
adrelanos marked this conversation as resolved.
Show resolved Hide resolved
then
echo "/home /home ext4 defaults,nosuid,nodev,remount 0 2" >> $config_file
else
echo "/home /home ext4 defaults,nosuid,nodev,bind 0 2" >> $config_file
fi

if grep --quiet " /tmp " /etc/fstab
adrelanos marked this conversation as resolved.
Show resolved Hide resolved
then
echo "/tmp /tmp ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file
else
echo "tmpfs /tmp tmpfs defaults,nosuid,nodev,noexec 0 0" >> $config_file
fi

if grep --quiet " /var/log/audit " /etc/fstab
then
echo "/var/log/audit /log/audit ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file
fi

if grep --quiet " /var/tmp " /etc/fstab
then
echo "/var/tmp /var/tmp ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file
else
echo "/tmp /var/tmp none defaults,nosuid,nodev,noexec,bind 0 0" >> $config_file
fi

if grep --quiet " /var/log " /etc/fstab
then
echo "/var/log /var/log ext4 defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file
else
echo "/var/log /var/log ext4 defaults,nosuid,nodev,noexec,bind 0 2" >> $config_file
fi

if grep --quiet " /var " /etc/fstab
then
echo "/var /var ext4 defaults,nosuid,nodev,remount 0 2" >> $config_file
else
echo "/var /var ext4 defaults,nosuid,nodev,bind 0 2" >> $config_file
fi
7 changes: 7 additions & 0 deletions usr/libexec/security-misc/secure-remount
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/bin/bash

config_file="/usr/lib/security-misc_secure-remount.conf"

mount --fstab $config_file --all
adrelanos marked this conversation as resolved.
Show resolved Hide resolved

echo "Partitions securely remounted by security-misc" >&2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: >&2 is writing to sterr, should be done for errors. Should not be done for info/success messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry copy pasta error, will fix.