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

Add more hardening options to the default profile in etc #151

Closed
wants to merge 1 commit into from

Conversation

monsieuremre
Copy link
Contributor

Disabling coredumps and setting a secure umask value. Comments explain more in detail. I am really sorry for the many pull requests, this is the last one. I don't mean to spam the project. They are mostly really small but significant changes. I thought that it would be easier to manage for the maintainer if they are seperated.

@adrelanos
Copy link
Member

The group does not need to be restricted. If username is user2 then the group on Debian is user2, which is fine. Quote from permission-lockdown script:

      ## Debian by default uses USERGROUPS=yes in /etc/adduser.conf.
      ## The group which the user is being added to has the same name as the user.
      ## If the username is user then the name of the group is also user.
      ## Some background information here:
      ## https://unix.stackexchange.com/questions/156473/reasons-behind-the-default-groups-and-users-on-linux
      ## In short, this is useful for "file sharing". A if user1 wants to share data with user2 the command
      ## required to run is sudo addgroup user1 user2.

In the past, we've changed the default umask (though through a different implementation), had run into unreasonable issues and then settled for a different solution (permission-lockdown). That's quite some time ago and I'd have to re-read this:
https://forums.whonix.org/t/change-default-umask/7416


Changes in /etc/profile.d can result in difficult to predict, strange, hard to debug issues such as this:

where some extra output by /etc/profile.d resulted in broken upgrades.


The suggested umask change:
Really needed? I mean... Is there any example where a created file currently has insecure permissions thereby making it readable by another Linux user account?


The suggested ulimit -c 0 to disable core dumps change:
Really needed? How can core dumps still be created without that setting?

@adrelanos
Copy link
Member

All your contributions are most helpful and appreciated!

@monsieuremre
Copy link
Contributor Author

Really needed? How can core dumps still be created without that setting?
No. They can't be created. But disabling core dumps with all means possible is a better approach to not let any possibility of the user accidentally enabling them.

Really needed? I mean... Is there any example where a created file currently has insecure permissions thereby making it readable by another Linux user account?

Debian uses 0022 per default. It means when we create a directory for example, the permission is set to 755. This means, the group and others can read and execute, but they can't write. With 0077 we would deny them even reading. All the security guides I read recommend a minimum of 0027 umask. Debian's official guide is one of them.

I know this does not break anything on a personal computer, or on a vm. But I don't know anything about qubes. They do their stuff differently and there are more things to consider there.

@monsieuremre
Copy link
Contributor Author

Having read the old forum threads, I gather that most problems that are not qubes related are because you chose to not set the group to 7 as well. A umask of 0077 would eliminate these problems. When it comes to qubes, I don't know what the problem was, and I can't find what the problem was in detail. So as I said, we are most likely clear apart from qubes. But I don't know if the problem with qubes was the umask value itself or the fact that we put files under /etc/profile.d/ at all. Are there more resources?

@adrelanos
Copy link
Member

adrelanos commented Nov 3, 2023 via email

@adrelanos
Copy link
Member

Why is ulimit -c 0 extra, duplicated? If doing ulimit -c 0 then by extension would we have to duplicate all the sysctl etc.?

@adrelanos
Copy link
Member

As for default umask, could we use libpam-umask package instead as Debian mentions in the link that you shared? That seems cleaner.

/etc/profile.d seems hackish. Last resort only. It's hard to know in which situations it applies and when it doesn't in the various environments such as:

  • different shells
  • terminal emulator
  • X
  • Wayland
  • login shell
  • terminal emulator
  • non-interactive session

Check this out:
https://github.com/Kicksecure/security-misc/tree/master/usr/share/pam-configs

@adrelanos
Copy link
Member

@adrelanos
Copy link
Member

As for default umask, could we use libpam-umask package instead as Debian mentions in the link that you shared? That seems cleaner.

https://github.com/Kicksecure/security-misc/blob/master/usr/share/pam-configs/umask-security-misc

@adrelanos
Copy link
Member

As for umask, that seems to work. Please test and let me know if that resolves the umask part.

@adrelanos
Copy link
Member

That umask setting works for user and root but it has the same issue as before.

Files created by root in /etc aren't readable by "others".

Now tb-starter package has this issue:

bash -n /etc/torbrowser.d/50_user.conf
bash: /etc/torbrowser.d/50_user.conf: Permission denied

Maybe update-torbrowser which runs as user user reading config files in /etc is a weird implementation and therefore should get a special fix and call it a day? If this was the case, then yes. But I don't think so. Others applications implemented in the same way:

  • /etc/thunderbird
  • /etc/firefox-esr

Therefore I reverted my pam based implementation for now.

@adrelanos
Copy link
Member

Reverted the revert. Quote pam_umask man page.

   usergroups
       If the user is not root and the username is the same as
       primary group name, the umask group bits are set to be the
       same as owner bits (examples: 022 -> 002, 077 -> 007).

   nousergroups
       This is the direct opposite of the usergroups option
       described above, which can be useful in case pam_umask has
       been compiled with usergroups enabled by default and you want
       to disable it at runtime.

Maybe that could fix the root issue.

@adrelanos
Copy link
Member

session optional pam_umask.so usergroups umask=027

Does not work. Files created by root still unreadable by others.

session optional pam_umask.so usergroups

Works as expected.

  • user for files created in /home/user folder: files no longer readable by others (so that folder is "special" due to permission-lockdown / pam mkhomedir and not a good place for testing)
  • user for files created in /tmp folder: -rw-rw-r-- (bad)
  • root: files readable by others (good because needed for /etc)

@adrelanos
Copy link
Member

adrelanos commented Nov 3, 2023

It might be possible to script Linux PAM config so pam_umask does not apply to root.

adrelanos added a commit to adrelanos/security-misc that referenced this pull request Nov 3, 2023
since this causes permission issues in `/etc/`

Kicksecure#151
@adrelanos
Copy link
Member

It might be possible to script Linux PAM config so pam_umask does not apply to root.

Done.

@adrelanos
Copy link
Member

Readme was updated for pam_umask use.

@monsieuremre
Copy link
Contributor Author

How would core dumps we accidentally enabled?

They wouldn't normally. But disabling them wherever possible is a net positive and brings no harm.

Why is ulimit -c 0 extra, duplicated?

I don't understand how it's duplicated.

Does not work. Files created by root still unreadable by others.

This is the expected funtionality. That's why we set it to 0027 or 0077. So that others can't read. I don't understand how this is a problem. Does it break anything? Root is arguably the most important user that should have this umask.

@adrelanos
Copy link
Member

How would core dumps we accidentally enabled?

They wouldn't normally. But disabling them wherever possible is a net positive and brings no harm.

Why is ulimit -c 0 extra, duplicated?

I don't understand how it's duplicated.

Because core dump disabling is already disabled using the appropriate configuration files I see no benefit in doing it in /etc/profile.d again. Instead there's a bit harm:

  • It gets more difficult for those who really need to enable core dumps to figure out how to re-enable it since now it's disabled in multiple places.
  • Carrying unnecessary files.

Does not work. Files created by root still unreadable by others.

This is the expected funtionality. That's why we set it to 0027 or 0077. So that others can't read. I don't understand how this is a problem. Does it break anything? Root is arguably the most important user that should have this umask.

It breaks reading config files in /etc for:

  • tb-updater
  • Firefox
  • Thunderbird
  • Probably other applications that run as user user but that read config files created by the system administator (or even packages) in /etc.

@monsieuremre
Copy link
Contributor Author

It gets more difficult for those who really need to enable core dumps to figure out how to re-enable it since now it's disabled in multiple places.
It already is disabled in multiple places and they are documented. Anyone who needs coredumps would not go through this hardening. And if they choose to disable, we document all the places we disable coredumps.

It breaks reading config files in /etc for

Permissions don't apply retroactively. So older files' permissions will stay the same. We can make exceptions for these files also for the future if they are countable, for example by manually granting read permission for others with the permission-hardener daemon. It is also an admin's responsibility to set the read permissions manually after creating configuration files. So manual stuff, no problem. Only the packages might be problematic. If these are too many to create exceptions for, we just stay with the current workaround.

@adrelanos
Copy link
Member

It gets more difficult for those who really need to enable core dumps to figure out how to re-enable it since now it's disabled in multiple places.
It already is disabled in multiple places and they are documented. Anyone who needs coredumps would not go through this hardening. And if they choose to disable, we document all the places we disable coredumps.

It breaks reading config files in /etc for

Permissions don't apply retroactively.

Retroactive is relative. Once the new umask for root enabled, and after another package gets installed, the umask hardening might apply.

We can make exceptions for these files also for the future if they are countable, for example by manually granting read permission for others with the permission-hardener daemon.

It is also an admin's responsibility to set the read permissions manually after creating configuration files.

An admin trying to harden / preconfigure for example /etc/firefox-esr or /etc/thunderbird will have no clue whatsoever that the config files will be readable by root only but not by the actual applications to be configured. Since these are graphical applications, there might be no error message. If lucky, there might be a warning or error message from starting the application from a terminal emulator.

So manual stuff, no problem. Only the packages might be problematic. If these are too many to create exceptions for, we just stay with the current workaround.

It cannot be guestimated how many such cases exist.

@monsieuremre
Copy link
Contributor Author

Hmm. Thinking. Yeah maybe. Current umask for everyone but root is already not that bad anyway. We can still add the coredump limit tho. I see no harm there. The use case of coredumps is really to my knowledge wastly limited to kernel development and debugging and stuff. Those people will figure how to disable coredumps if they choose to harden anyway, considering that would make their work harder.

@adrelanos adrelanos mentioned this pull request Nov 22, 2023
@adrelanos
Copy link
Member

How to even re-enable coredumps as of now?
Is this implemented in debug-misc?

I don't want to configure us into a corner and then when somebody asks how to re-enable functionality, nobody knows the answer and it's a major effort to re-enable it.

@monsieuremre
Copy link
Contributor Author

I don't think we should worry about that much. Coredumps are of no use on a daily driver. Debugging and development are the only cases where someone would want to enable it, which I say, these people probably won't do kernel development or debugging on their daily driver. It is true that it might be useful for debugging and development in a general sense that is not limited to the kernel, but I don't think it is far fetched to assume these people are in a position to re-enable coredumps themselves.

@adrelanos
Copy link
Member

but I don't think it is far fetched to assume these people are in a position to re-enable coredumps themselves.

Unfortunately that is a wrong assumption. I know, discussed similar things with skilled developers for C and whatnot but they would fail with the sysadmin part because that's a much different skill. Documentation exists on the internet but scattered.

Hence, my development standard has always been that that is hardened should be documented with known ways on how to undo it. If even you cannot easily answer above questions in much more time than required to type it that means that it's already too difficult.

The core dump might have been a slip through. Something I'll look into.

Complete, clean, documented, reversible solutions are welcome.

Otherwise incomplete, quick solutions with hardening only but without documentation and not easily reversible will take some time until I can complete them.

@monsieuremre
Copy link
Contributor Author

Complete, clean, documented, reversible solutions are welcome.

Got it. Should not be difficult to document.

@monsieuremre
Copy link
Contributor Author

  • Umask is already handled with pam for non-root users.

  • Coredumps are already disabled and disabling within /etc is better left to the user.

Ergo, closing.

@monsieuremre monsieuremre deleted the profile branch January 17, 2024 12:05
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

Successfully merging this pull request may close these issues.

None yet

2 participants