-
Notifications
You must be signed in to change notification settings - Fork 574
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
init script security fixes #5850
Conversation
ICINGA2_ERROR_LOG=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2/error.log | ||
ICINGA2_STARTUP_LOG=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2/startup.log | ||
ICINGA2_LOG=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2/icinga2.log | ||
ICINGA2_LOG_DIR=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2 |
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 could be undefined in installations where this configuration file is not overridden on upgrade. prepare-dirs should have a fallback definition if ICINGA2_LOG_DIR is not defined.
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.
Aye, will use dirname -- $ICINGA2_LOG
as fallback
e5abc62
to
b22fc6c
Compare
Added a default for ICINGA2_LOG_DIR and quoted all the paths in case somebody has spaces or other wonky stuff in their config |
I'd wait on @orlitzky 's feedback. |
I'm skeptical about the invisible but important requirement that all of those parent directories are owned/writable only by root. If the user is supposed to verify that the permissions on the parent directories are correct, why not just have him verify that the permissions on the log directory are correct too? (I know that the answer is "because they don't know what they're doing," -- that's why I'm worried about leaving it up to them to set all those variables responsibly.) And there's a typo "mdkir" in there =) I was up all night so it might be another day before I can give more intelligent feedback. |
It's not only that they are supposed what they are doing when they change the default location, but that we can't guarantee security when they have a custom installation :/ |
I'm better now... in this stanza,
the last
But once these things are handled safely, the value of Unrelated: is it just me, or is one of |
16d9e71
to
1bb1735
Compare
Sadly no, as The chown has to be recursive because theoretically you could delete icinga2/cmd and make it a symlink somewhere between two chowns. |
1bb1735
to
4b51d9a
Compare
Oh, duh, good catch. You can always pass It looks like I did a bad job of explaining the "su" problem with humor. There are a few different implementations, and not all of them treat A cross-platform "su -s" would solve a lot of these problems straight away. As far as I know, all of them generally work the same with |
Won't work either :( We run adduser with --system in the debian packages, ie. no login shell |
It is quite common to modify the application user to not have a terminal, for security reasons. That's not just Debian as default, likely this affects more setups. There are community packages where we don't have control over, and cannot just break this. Imho. |
A short update on the current state of things: I'll make sure the CVE fix will be in the 2.8.2, even if no epiphany for #5991 strikes me before we have a release. |
etc/initsystem/prepare-dirs
Outdated
chown $ICINGA2_USER:$ICINGA2_GROUP $ICINGA2_PID_FILE | ||
if [ ! -e "$ICINGA2_RUN_DIR"/icinga2 ]; then | ||
mkdir "$ICINGA2_RUN_DIR"/icinga2 | ||
mdkir "$ICINGA2_RUN_DIR"/icinga2/cmd |
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.
typo: mdkir
(it might make sense to chain these all together with &&
?)
etc/initsystem/prepare-dirs
Outdated
chown $ICINGA2_USER:$ICINGA2_COMMAND_GROUP $ICINGA2_LOG | ||
# Could be undefined in installations where sysconf is not overridden on upgrade | ||
if [ -z "$ICINGA2_LOG_DIR" ]; then | ||
$ICINGA2_LOG_DIR=dirname -- "$ICINGA2_LOG" |
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'll bet this should be ICINGA2_LOG_DIR=$(dirname -- "$ICINGA2_LOG")
703842d
to
fa65079
Compare
5194288
to
bf1a935
Compare
bf1a935
to
6ce4cab
Compare
6ce4cab
to
7b9f2a7
Compare
cb99775
to
1b62551
Compare
1b62551
to
c33bc28
Compare
…connect_all`. Fixes #5915
They are now read form the sysconfig file which is owned by root
c33bc28
to
87adc88
Compare
Also fixes CVE-2018-6533 |
This assumes the parent directories for run dir (usually /var/run), log dir (usually /var/log) and cache dir (usually /var/cache) are all owned by and can only be written to by root.
Still a bit of testing to do before we can merge this. I'm also open for suggestions, the current solution is quite ugly
refs #5793