The checks work locally(at least with debian stretch and gcc-6). I believe that the Travis-CI fails because $TRAVIS_BUILD_DIR is group-writeable. My changes prevent using logrotate on directories where the group can write at some point of the path. The following lines in .travis.yml should make sure that the TRAVIS_BUILD_DIR isn't writeable by the group:
script:

• chmod g-w -R $TRAVIS_BUILD_DIR

Sorry but this code is overly complex and I am afraid that it introduces more problems than it could ever solve. For example, you first count the directories using your hard-wired open-coded implementation of dirname() but then you split the path using platform-dependent implementation of dirname(), which may see a different count of directories. It is also not clear why you first allocate an array of directory names and process them later on, instead of processing the names as you read them. Also the conditions deciding what is trusted and what not do not reflect the common use cases as the test-suite already suggests.

There is an existing permission check implemented in findNeedRotating(). So you should take it as the baseline and think only about the scenarios that are not covered by the current check and need to be improved (ideally in a way that does not break current safe enough deployments):

Hi, thank you very much for your feedback. I allocated the memory first because traversing with dirname() starts from the last directory up to the root. In order to prevent that during the check somebody just exchanges the directories(because the check would then start from the last directory up to the root), I do the checks the other way(from root to the last part of the path).

I am aware that this check breaks some installations. I tried to make it a little bit tighter and hoped that we find a good solution by discussing it.

The check now prevents both: race-conditions by changing the directories and race-condition by replacing the files(if that is even possible). We could weaken the checks by just preventing changing the directories. Then we would not check the logdir itself but start checking it's parent. I believe that we would break less, but with the price that we won't cover filebased race-conditions.

As far as I know the function findNeedRotating() doesn't check the complete path. It just checks the parent directory. So lets say we have the following directory:
/home/user/logdir/file.log
/home = root
/home/user = user
/home/user/logdir = root
/home/user/logdir/file.log root.user rw-rw----

in such a scenario the user could simply replace logdir(even if its owned by root) because he has write access to /home/user. If user could replace the logdir by a symlink using the exploit I mentioned on my blog, he could gain write access to a file in any directory.
Therefore I think it is crucial to check the complete path with all its links and directories. And this is a kind of complex task.

If we ignore the prevention for file-rotating, we could just check with is_trusted_dir() everything above logdir. In this example that would be:

/home
/home/user

If those have proper permissions, it seems to me that just replacing the logdir by a user isn't possible anymore. This would have a much lesser impact on existing installations, and could nevertheless give additional security. So configurations like the following would then not suffer from the changes:
/var =root
/var/log =root
/var/log/mysql = mysql

PS: About the hard-wired open-coded implementation of dirname() for counting the numDirs: I would change that so that it also uses dirname() instead

On Tuesday, January 22, 2019 12:22:48 PM CET whotwagner wrote: Hi, thank you very much for your feedback. I allocated the memory first because traversing with dirname() starts from the last directory up to the root. In order to prevent that during the check somebody just exchanges the directories(because the check would then from the last directory up to the root), I do the checks the other way(from root to the last part of the path).
I do not think that the order of checks matters as long as you call lstat() on paths only. The attacker could wait till all your checks are complete and then change the file system arbitrarily.

I think the order matters. The permissions on the parent directory defines if someone can rename the directory itself. If / is safe /var can't be renamed. If /var/ is safe /var/log can't be renamed. If /var/log/ is safe /var/log/mysql can't be renamed. Aso.

On Tuesday, January 22, 2019 10:29:01 PM CET whotwagner wrote: As far as I know the function findNeedRotating() doesn't check the complete path. It just checks the parent directory.
True but that was not the reason why your attack vector was successful, was it? The actual reason was that logrotate did not check the owner of the directory because logrotate currently checks permission bits and group only. It is technically not so difficult to extend the current check for owner, too. I am only afraid that it could also break some valid setups. People sometimes use logrotate on file systems like FAT, which are not able to track ownership properly. Moreover, I am not sure whether it is logrotate's job to check permissions of the root directory etc. If these directories are writable by non-privileged users, then logrotate is likely not the only program that is affected by such bogus setup.

In my attack, it is possible to write into an arbitary directory as soon as the attacker is able to change any point of the path to the logdirectory. A check of the owner on the logdirectory would not fix this. If the path to the logdirectory could be modified at any point, an attacker could simply replace the the logdirectory during the time window of the racecondition. Even though my exploit targets the weak permissions of the parent of the logdirectory(the example i used was /tmp and the logdirectory was /tmp/log), it could easily be rewritten to use any other point in the whole path.

I was checking some servers and found a paths like this:
/var (root)
/var/www (root)
/var/www/projektname (www-data)
/var/www/projektname/logdir (root) (but with additional acls so that the files are writeable by www-data)

This scenario is defacto vulnerable. since www-data could replace logdir and has write-permissions to the file that gets created. Similar scenarios might occur at servers of webhosting providers.

I believe that checking the permissions of the whole path until the logdir itself makes sense, since the problem isn't always obvious.

On Wednesday, January 23, 2019 8:21:21 PM CET whotwagner wrote: I think the order matters.
And I am trying to explain you that it does not.
The permissions on the parent directory defines if someone can rename the directory itself. If / is safe /var can't be renamed. If /var/ is safe /var/log can't be renamed. If /var/log/ is safe /var/log/mysql can't be renamed. Aso.
You can see that I already mentioned this while reviewing your previous PR: #235 (comment) The problem is that the code stats file paths instead of file descriptors. So the actual state of the file system may change arbitrarily before/during/after all the checks.

This time, I read the SEI CERT C Coding Standard first, before I started to write this commit. The algorithm is the recommendation from the following site: https://wiki.sei.cmu.edu/confluence/display/c/FIO15-C.+Ensure+that+file+operations+are+performed+in+a+secure+directory

So I am a bit confused now. Maybe their recommendation does not solve the problem . Then we could discuss this with them. Or my implementation is wrong, then please tell me where it differs from the original one in a bad way so that I fix it.

The page you refer to says:

When checking directories, it is important to traverse from the root to the leaf to avoid a dangerous race condition in which an attacker who has privileges to at least one of the directories can rename and re-create a directory after the privilege verification.

It does not say any details about where the race condition comes from, or why changing the order of checks would help here. If you go from leaf to root and check that all parts of the path are not writable by non-privileged users, how could a non-privileged user rename and re-create a directory after the check?

Back to logrotate ... the basic question is: Can we ever change logrotate to insist on using su when it rotates in directories owned by non-privileged users while running as root?

If the answer is no, then all the other proposals are irrelevant, as I understand it.

Back to logrotate ... the basic question is: Can we ever change logrotate to insist on using su when it rotates in directories owned by non-privileged users while running as root?

I would say: "yes we can". The problem is too complex to leave the user/admins alone with it. I know that applying those changes would mean that maintainer of linux distributions would have some more work. It also means that existing installations need some configuration changes. But it would improve the security of the affected systems. And it would ensure, that also future installations/configurations of logrotate aren't prone to such race conditions.

@glensc @cgzones @kerolasa What is your opinion on this?

Can we change logrotate to insist on using su when it rotates in directories owned by non-privileged users while running as root?

It's not obvious to me how using su would fix in-between path tampering due data race(s). If something in this security front ought to be done I think it should be based on inotify(7). If I'm not wrong recursive inotify does not exist, so one has to setup before rotating monitoring to each directory all the way to rotation base directory. Then if something suspicious happens while logrotation performing expected file operations abort execution, and log what was the reason to stop prematurely.

It's not obvious to me how using su would fix in-between path tampering due data race(s).

If one uses su in logrotate's configuration, logrotate drops privileges while rotating logs. Consequently, non-privileged attackers using timed symlink attacks can only cause damage to files they have access to. This could hardly be called an attack ;-)

The concept of insecure directories also affects the configuration directive olddir.
An attacker might create a hard-link to /etc/shadow as /var/log/olddir/log.1 and when logrotate rotates and the shred option is enabled, the file gets emptied.

All these attacks can be stopped by setting the Linux kernel sysctl protected_hardlinks to 1, so unprivileged users can no longer create hard-links to privileged files like /etc/shadow.
This option is available since Linux 3.6 (torvalds/linux@800179c).

Should we check if /proc/sys/fs/protected_hardlinks exists and warn (even error out) if its value is 0?

I am not sure whether this is logrotate's job really. Are there any similar programs that have implemented such a check already?

How would this effect non-privledged users with logs and state files in their own directory?

I believe this pull request is now obsoleted by pull request #455, so I am closing this one to keep the discussion at one place.

@DG12 As I understand it, non-privileged users rotating logs in their own directories should not be affected by this change.