Skip to content

Mitigation of a race condition in createOutputFile()#235

Closed
whotwagner wants to merge 1 commit intologrotate:masterfrom
whotwagner:racecond
Closed

Mitigation of a race condition in createOutputFile()#235
whotwagner wants to merge 1 commit intologrotate:masterfrom
whotwagner:racecond

Conversation

@whotwagner
Copy link

@whotwagner whotwagner commented Jan 16, 2019

If the "create"-option is set, there is a time window between rotating the logfile
and creating/touching a new empty file. If a user is in control of the parent
directory, he/she could replace it during this time window with a symbolic link to
any directory. If logrotate was executed by root, the new file will be created first
with permissions of root and then the ownerships will be set to the user. After the
file was created, the user might be able to write any content to this file.

@whotwagner
Mitigation of a race condition in createOutputFile()
b67fe03 
If the "create"-option is set, there is a time window between rotating the logfile
and creating/touching a new empty file. If a user is in control of the parent
directory, he/she could replace it during this time window with a symbolic link to
any directory. If logrotate was executed by root, the new file will be created first
with permissions of root and then the ownerships will be set to the user. After the
file was created, the user might be able to write any content to this file.
@kdudka
Copy link
Member

kdudka commented Jan 17, 2019 •
edited
Loading

First off, thank you for helping us to improve logrotate! But I do not think that the proposed change could reliably mitigate the attack vector you were describing for multiple reasons:

  1. The createOutputFile() function itself is already resistant to the symlink-based attacks because it operates on a file descriptor. So if the file with the originally opened name is replaced after the call of open(), the createOutputFile() function continues to use the opened file and it is not affected by the replacement. The problem you were describing happens between the rename() operations, as I understand it. I believe that it can happen while using nocreate, too. So a reliable fix simply cannot be implemented in createOutputFile().

  2. You call lstat() on the name of the parent directory. So if a non-privileged user has write access to any (direct or indirect) ancestor of the parent directory, which your patch does not check for, they can replace the directory by anything else after your call of lstat().

  3. The symlink does not need to be created at the parent directory directly. You would need to check also all indirect parents up to the root. And even then the code would be prone to race conditions unless you rewrite logrotate to operate on open file descriptors only. This would basically mean to start using openat(), renameat(), etc. everywhere in the code.

The change also introduces a memory leak but it is not important at this step of the review.

@whotwagner
Copy link
Author

whotwagner commented Jan 17, 2019 •
edited
Loading

Hi,

  1. After my pull request I reallized that a reliable fix cant be implemented in createOutputFile(). I even think that a reliable fix might not be possible(at least not in a way without other problems/costs ).
  2. oops. didn't see that

I am going to close this pull request.

@whotwagner whotwagner closed this Jan 17, 2019
@kdudka
Copy link
Member

kdudka commented Jan 17, 2019

Yes, it is not easy to make 20+ years old software immediately perfectly secure. On the other hand, we should at least improve the documentation. It currently roughly says what the su directive does but the documentation fails to explain why administrators should proactively use the su directive in certain cases.

@whotwagner
Copy link
Author

whotwagner commented Jan 17, 2019

I fully understand you that it isn't easy to fix certain things and I think its a great idea to make admins aware.

kdudka added a commit that referenced this pull request Jan 18, 2019
@kdudka
logrotate.8: encourage admins to use the su directive
51aabca 
... to rotate files in directories that are directly or indirectly in
control of non-privileged users.  Originally reported in the following
pull request:

#235
@kdudka kdudka mentioned this pull request Jan 18, 2019
Closed
@kdudka
Copy link
Member

kdudka commented Jan 18, 2019

I have submitted pull request #236 to improve the documentation.

whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
d84b876 
Rotating logfiles for which just one directory of the absolute path could be modified by another user is dangerous. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping insecure logfiles. A function is_trusted_dir() is introduced. It checks the complete path including symlinks for insecure configurations.
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
e925b24 
It is dangerous to rotate logfiles located in a directory-path that could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. A function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
e3b8320 
It is dangerous to rotate logfiles located in a directory-path that could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. A function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
685ebc2 
It is dangerous to rotate logfiles that located in a directory-path which could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. The function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
@whotwagner whotwagner mentioned this pull request Jan 21, 2019
Closed
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
fd33324 
It is dangerous to rotate logfiles that located in a directory-path which could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. The function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
14cbd98 
It is dangerous to rotate logfiles that located in a directory-path which could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. The function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
887ecef 
It is dangerous to rotate logfiles that located in a directory-path which could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. The function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
whotwagner pushed a commit to whotwagner/logrotate that referenced this pull request Jan 21, 2019
Disallow execution of logrotate with an insecure logfile path
08480ba 
It is dangerous to rotate logfiles that located in a directory-path which could be modified by another user. Due to race conditions a privilege escalation could happen. This was already discussed at the pull-request logrotate#235

This commit makes logrotate skipping logfiles that are located in a insecure directory. The function is_trusted_dir() is introduced. It checks the complete path of a logfile-directory including symlinks for insecure configurations.
kdudka added a commit that referenced this pull request Jan 28, 2019
@kdudka
logrotate.8: encourage admins to use the su directive
3e170c0 
... to rotate files in directories that are directly or indirectly in
control of non-privileged users.  Originally reported in the following
pull request:

#235

Closes #236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@whotwagner @kdudka