Add defence in depth when dropping privileges#400
Conversation
Do not continue in a possible root owned directory
| return 1; | ||
| } | ||
|
|
||
| if (chdir("/") != 0) { |
There was a problem hiding this comment.
Why are we changing the working directory? Is it an observable change of behavior? If yes, is it backward-compatible? I think this should be documented in any case.
There was a problem hiding this comment.
I think currently logrotate does not give any guarantee about the current working directory (when spawning the compress or mail process or when running any scripts).
In config.c we call chdir(), but be at the end we return into the previously directory.
When run by systemd the working directory is by default set to /, see https://www.freedesktop.org/software/systemd/man/systemd.exec.html section WorkingDirectory=.
I am not sure if cron implementations (cronie, ...) give any guarantees.
Adding to the man-page that the current working directory in scripts is unspecified and should not be relied on would make sense to me.
There was a problem hiding this comment.
Thank you for digging the information. So the change should not be needed when logrotate is triggered by systemd as a system service, which is what many Linux distros do these days.
There was a problem hiding this comment.
It makes a difference when logrotate is started from a root crontab, where the working directory is the home directory, i.e. /root.
There was a problem hiding this comment.
I have no strong opinion on this. It can improve security in some corner cases but also break compatibility in other corner cases. We should probably prefer the former to stay on the safe side.
No description provided.