-
Notifications
You must be signed in to change notification settings - Fork 50
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
Better Secure Remount #152
Better Secure Remount #152
Conversation
First, I need to set expectations right. I am not sure this will get merged. There's some good and some issues. Going to reply inline. |
@@ -61,6 +61,8 @@ pam-auth-update --package | |||
/usr/libexec/security-misc/permission-lockdown | |||
permission_hardening | |||
|
|||
/usr/libexec/security-misc/generate-secure-remount-config |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
rm $config_file | ||
fi | ||
touch $config_file | ||
chmod u+x $config_file |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
touch $config_file | ||
chmod u+x $config_file | ||
|
||
echo "/ / ext4 defaults,remount 0 2" >> $config_file |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
|
||
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
mount --fstab $config_file --all | ||
|
||
echo "Partitions securely remounted by security-misc" >&2 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -0,0 +1,12 @@ | |||
[Unit] | |||
Description=Remount partitions with hardened mount options. | |||
After=sysinit.target |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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=?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- https://lists.freedesktop.org/archives/systemd-devel/2019-December/043844.html
- https://lists.freedesktop.org/archives/systemd-devel/2019-December/043845.html
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.
There was a problem hiding this comment.
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).
|
||
if grep --quiet " /home " /etc/fstab | ||
then | ||
echo "/home /home none defaults,nosuid,nodev,remount 0 2" >> $config_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults
could undo what the user customized in /etc/fstab
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, we can easily get rid of the 'defaults' for remounts and just stick to it when doing 'bind'. I think that might even be better regardless. Will push with changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
then | ||
echo "/home /home none defaults,nosuid,nodev,remount 0 2" >> $config_file | ||
else | ||
echo "/home /home none defaults,nosuid,nodev,bind 0 2" >> $config_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this undo potential user customizations in /etc/fstab
such as noatime
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will test and see.
Terminology:
So yeah, I see a lot of (potential) issues with the suggested implementation as mentioned above inline. Other issues:
Why not iterate on top of the git master implementation?
A lot of work already went into
The remaining issue (umout at shutdown) of the git master implementation might be already fixed in the systemd version in Debian trixie. I think this much more important to test but I didn't get to it yet. That's why I am hesitant to completely throw away the existing implementation and start over. I am however eager to get rid of dracut and replace it with early systemd if that is possible without race conditions.
|
This completely understandable. A lot of work went into the current implementation. It is significantly more tested. It doesn't make sense to throw it all away. Seems a little risky. We can find a middle ground here I think. First of all, I can iterate on the old implementation to modernize it with the new approach. I can:
But I think I can better port all of these to the current implementation because it would pretty much be a rewrite of a good chunk either way. And by that, I mean I already did. I already fixed all the stuff you pointed out as problematic, which they actually were. I already tested using findmnt instead of parsing /etc/fstab with the new implementationf and it works. I also got rid of the generate once on install approach and now we generate the fstab on each startup. This adds a very miniscule extra latency, but is still quite considerably faster than dracut. I had already fixed enforcing file systems, so thats even on the tree. The others, I didn't push yet. Should I push? Or should I reiterate on the old base and do a more or less complete rewrite, which would yield the same result. Does this option sound reasonable to you? If there are more problems with the new approach, please point out, I would be more than happy to fix them to comply with the expectations of this package. |
This is quite easy to add too. I did this on purpose because I don't believe the current settings will result in any breakage whatsoever. How do I know? Well, I have been daily driving these options on Debian for 3 months and I have been doing basic development and all the other daily stuff. And nothing broke. I think defining more levels to this options introduces more complexity that is not really needed. So I think, an option to disable would more than suffice. But If you wish, I can easily respect the boot paramaters as in the old implementation, no problem. Please do tell if you definetely still want this. |
Disabled for now.
Ok.
Yes. (If not messing with defaults, noatime, file systems, and whatnot. I am not sure yet it is even possible to create an fstab file without running into corner case issues.)
Yes.
Nice.
Can push at any time to see what changed. Iterate based on old one seems best since it has nice declarations [1] and tested configuration by kernel parameters.
I will check now again inline. Variable could be unconditionally set to [1] Old implementation remount-secure seems very cleanly implemented using the snippets such as:
That would be good to keep if reasonable. |
|
||
if grep --quiet " /tmp " /etc/fstab | ||
then | ||
echo "/tmp /tmp none defaults,nosuid,nodev,noexec,remount 0 2" >> $config_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also stuff at the end 0 2
shouldn't be unnecessarily touched.
Things change if it gets deployed (on a supposedly stable version) for 1000s of users. Even the very old implementation before worked in principle for me but then some users complained about random unbootable systems if I remember right. This was due to race conditions when exactly the remount happened. And then the "no answer" on the systemd mailing list didn't increase my confidence doing this using systemd either. Therefore I am still worried the systemd approach needs to be reverted later but I guess at least the "write an fstab file instead of multiple calls to
There's some weird installers that still do weird stuff such as executing files in /tmp and also noexec for the home folder is controversial. So best to have this configurable. |
|
||
if [ -f $config_file ] | ||
then | ||
rm $config_file |
There was a problem hiding this comment.
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?
I am disabling the remount-secure dracut module. |
pending new systemd based implementation Kicksecure#152
In favor of the new implementation, or planning to drop the functionality? |
monsieuremre:
> I am disabling the remount-secure dracut module.
In favor of the new implementation, or planning to drop the functionality?
In favor for the upcoming systemd solution.
(And to prevent boot slowdown in case I move this to the testers
repository before that.)
|
I renamed the dracut module only. This is no change of plan about what I
said above at all.
Looking forward to the systemd based solution as discussed.
|
This reverts commit 479ab61. Kicksecure#152
I reverted (restored) the old systemd unit for remount-secure. https://github.com/Kicksecure/security-misc/blob/master/lib/systemd/system/remount-secure.service Please edit it to make it work without race conditions. Note there is still a systemd-present to disable it by default. (Was also git reverted by the revert.) But that does not hinder development. Enabling it is simple using:
|
There was a merge conflict with the systemd unit that I resolved but the systemd unit being the critical part here needs your review anyhow. |
Ok. The service looks nice and better than mine anyway. It is only the binary |
Nice. Though, not yet tested.
Right.
Two options:
That way makes it easier to compare the two different approaches and easier to go back and forth between the two. |
Hmm. I'm not sure why we're not just going for the new thing and ditching the old one. It gets the unmount order wrong and results in errors. |
monsieuremre:
Hmm. I'm not sure why we're not just going for the new thing and ditching the old one. It gets the unmount order wrong and results in errors.
Because I am not sure it will lead to race conditions during boot,
mysterious broken boot, the reason why the very first attempt stalled
for years.
|
Is there a Kicksecure ISO repo? There should be. We might be better of creating a secure debian ISO, which guarantees everything installs securely by default. Especially for stuff like this, where there is no direct solution unless we touch /etc/fstab. |
monsieuremre:
Is there a Kicksecure ISO repo?
ISO repo? No, there's no separate one. I don't think there needs to be a
separate one. It will just be integrated with derivative-maker. The APT
repository will also be using the same packages just made compatible
with the ISO (mostly inventing calamares config files and what else
comes up).
There should be. We might be better of creating a secure debian ISO, which guarantees everything installs securely by default.
Once there's an ISO, fstab can be hardened as well.
Especially for stuff like this, where there is no direct solution unless we touch /etc/fstab.
dracut /etc/fstab.sys might be the alternative fstab file we're looking
for. dracut prefers /etc/fstab.sys over /etc/fstab. See dracut source code.
|
This makes no difference still because that file is also meant to be edited by system administrators. Also handling things without extra liabilities like dracut is more preferable. I think we can reasonably eliminate all possible race conditions with this implementation. |
We are a package and we can't own /etc/fstab, ok. But what is stopping us from dynamically editing fstab exactly? We can backup the old config and very safely update the fstab to be hardened and not break anything on the system. This would by no means would be a 'clean' solution, but it would be the one and only non-band-aid direct solution, in my opinion. This can be managed with post pre install scripts. It can get really dirty, true. But i think what I have here is a rather 'clean' solution. Clean in a sense it does not break anything and is completely reversable and it is just a piece of script so we don't really 'own' any file, but of course still not 'clean clean'. So in theory it should also be compatible with Qubes. I went and thought about all the possible edge cases there can be. I believe everything is covered. Tho still this is not a config file, it is a dynamic adjustment script for the fstab. Would a similar solution be in the scope of this package with adjustments? |
I wrote this elsewhere but it depends what we try to accomplish: upgrade mount options for:
Doing 1. seems straight forward to contribute a hardened fstab to grml-debootstrap and Qubes as much as hardened mount options are accepted by default or as an opt-in. The others I have other ideas which goes too far for now which can be looked into when 1. is done.
It's an unclean solution. These tend to create follow-up mess and time required to clean it up. Example: It seems simple such as #147 which seems like "just add this 1 line to add an extra dependency to add libpam-tmpdir" but the follow-up issues were impossible to foresee, huge and time consuming to debug and fix. It broke 2 build tools which are chroot based, cowbuilder and grml-debootstrap. One could argue cowbuilder is unnecessary (which I would disagreed with) but grml-debootstrap is necessary for the build process, not easily replaced and even if replaced the replacement likely could have the same issues. Now still derivative-maker has to carry a lightweight patched/forked version of grml-debootstrap until patches are merged upstream to fix libpam-tmpdir compatibility at the root in a clean way. And even after it's done, this would still cause follow-up issues, hard to remember and debug in the future. Somebody using Kicksecure and attempting to use some build tool that uses chroot such as cowbuilder, grml-debootstrap would be broken. Things broken default in Kicksecure but not in Debian. And seemingly mysterious. Requiring probably tool-specific workarounds. Awful.
For example for the Kicksecure or Whonix web server a backup version of fstab wouldn't help much. If the server doesn't boot anymore, I cannot even easily access the backup to restore it. After server reboot, if it fails to boot, there is no more ssh access. That's not a great way to spend my day. Yeah, it can be recovered somehow but until that's done that's another 4-8 hours lost only for that.
But if it's not and the VM fails to boot then it's a headache to debug. Hence, hardening options should be suggested upstream first as much as possible so any issues caused by it are noticed earlier during upstream development. I wouldn't mind so much about these follow-up issues if I knew someone else would magically take care of these but I know this won't happen and it will all be blamed on me. |
So meanwhile what would be most helpful as contribution:
Harden it as much as possible. Send a PR for /usr/share/doc/security-misc/fstab or so. Based on that I could send PRs to grml-debootstarp (and maybe even Qubes). This also I might use during build of new VMs. That could become the new default fstab for new VM builds. (ISO to be worries about later once that is done.)
This is currently blocked for me because I don't have a hardened fstab file yet. |
I see no reason for it to break because the dynamic editing is done very carefully. And tho it is not the cleanest, I can't think of anything else that is more direct, simple, easy to manage and would not randomly break. The backup is there to reverse if so wished, it is not there as a fallback when things break, the idea is that things won't break, in an ideal scenario. I honestly can't find any direct and viable alternative other than this. |
Actually, nevermind. I think I found a solution that you want. I'm gonna create a new pull and close this one. |
Ding dong. Known problems with our current remount implementation are:
I come with a brand new approach. No dracut, no hook. No slow startup, just as fast as normal boot. No errrors in shutdown. So it is all the good stuff without the issues. We get all the hardneing benefits, and even more for some partitions. A systemd oneshot service executes this script, that does the remounting.
Well, how do we know what to remount and what to bind? These can change radically depending on the specific system.
The answer is, we generate a very solid config file for that very specific system on package install. We use this config file as our fstab to the remounting. The good old 'normal' fstab is still there and functions just a fine, does its thing on startup. We do our thing right after the normal fstab. And since it is a fstab file that we do the remounting with, tho a custom one, unmounting goes just in order perfectly.
Please do test. If you have any problems, let's see them. But as I see it, this is the same protection and a better experience. Ready to merge, I believe, because I tested it and adjusted the startup order so that all services start a ok, and real fast.