Developer
Status:
|
NOT LOGGED IN
|
BerliOS Developer Foundries
|
|
|
|
Project: Smb4K - Bugs
Summary |
Home Page |
Bugs |
Patches |
Lists |
Surveys |
News |
CVS |
Files |
FTP |
Screenshots |
Memberlist |
Admin |
Submit A Bug
| Open Bugs | Admin
[ Bug #9630 ] security: weaknesses in smb4k/core/smb4kfileio.cpp
Date:
2006-Dec-05 21:16 |
Submitted By:
kees |
Assigned To:
none |
Category:
None |
Priority:
5 |
Bug Group:
None |
Resolution:
Fixed |
Summary:
security: weaknesses in smb4k/core/smb4kfileio.cpp |
Original Submission:
Hi! While doing a general audit of smb4k, I found a number of issues that could use some attention in smb4kfileio.cpp:
priv escalation: writeFile uses mktemp, allowing a difficult race on sudoers file writing. If an attacker were able to replace this file between mktemp and the open, they could inject additional lines into the sudoers file. (I recommend never using mktemp, and only using mkstemp.)
information leak: writeFile stores the contents of sudoers without enforcing strict permissions, allowing world-readable contents. Normally the sudoers file is not world-readable, but making changes to it via smb4k allows for it. (Adding a mask on open would solve this.)
data destruction: remove_lock_file race allows arbitrary user-owned files to be mucked with. The lock file used by smb4k can easily be raced and allow for manipulation of user-owned files by an attack.
Thanks! |
Followups
| Comment |
Date |
By | | Fixed in 0.8.0 and security patches. | 2006-Dec-21 19:09 | dustpuppy | | I rewrote the functions Smb4KFileIO::createLockFile() and Smb4KFileIO::removeLockFile() and implemented the fstat() system call to get the information about the file. Since fstat() uses a file descriptor instead of the file path, I think the problem is fixed now...!? | 2006-Dec-14 10:49 | dustpuppy | Hi! Thanks for working on this.
I took a look here:
http://cvs.berlios.de/cgi-bin/viewcvs.cgi/smb4k/smb4k/smb4k/core/smb4kfileio.cpp.diff?r1=1.97&r2=1.98
This is a good start, however, you need to use "fdstat" rather than "lstat", so you can check the file you opened, rather than the file on disk (which, again, could have changed since you opened it).
Thanks! | 2006-Dec-12 19:28 | kees | | I implemented a fix to the race. Now the file will be opened first and then checked if it is a regular one. If it is not, it'll be closed and an error will be reported. | 2006-Dec-12 12:26 | dustpuppy | Hi! Yeah, that's the "race", however moving it into one if statement won't fix the issue since each test is done consecutively. (Now, don't get me wrong: it's a very hard race to "win", but it's still technically a vulnerability.)
The usual way to handle this is to open the file first, and then use "fstat" to do the reg/fifo/lnk tests. If any of them fail, then just close the file and error out.
As for the lock file, perhaps you could use a non-world-writable directory such as /var/lock or /var/run/smb4k/, but make the files world-writable. That way the lock could only be created at install-time, etc. | 2006-Dec-07 21:11 | kees | If I understand correctly, the problem is, that some time elapses between the check and the moment the file is opened. So, would the following approach fix the issue?
if ( S_ISREG( file_stat.st_mode ) && !S_ISFIFO( file_stat.st_mode ) && !S_ISLNK( file_stat.st_mode ) && file.open( IO_ReadWrite ) )
{
...
}
else
{
if ( S_ISFIFO( file_stat.st_mode ) || S_ISLNK( file_stat.st_mode ) )
{
Smb4KError::error( ERROR_LOCK_FILE_IS_SYMLINK, file.name() );
return ok;
}
else
{
// Opening the file failed
Smb4KError::error( ERROR_OPENING_FILE, file.name() );
return ok;
}
}
Besides, I'm aware of the problem of a system-wide lock file in /tmp, but I have no better idea at the moment . If you have any idea... | 2006-Dec-07 09:45 | dustpuppy | That's great news, thanks! (Yes, with mkstemp, the file is now protected from reading.)
I'm not sure of a good way to deal with the lock file. Generally it's unsafe to have a system-wide lock file in /tmp. The race is mostly in code like this:
if ( S_ISREG( file_stat.st_mode ) && !S_ISFIFO( file_stat.st_mode ) && !S_ISLNK( file_stat.st_mode ) )
{
/* bad guy can replace file with symlink here */
if ( file.open( IO_ReadWrite ) )
| 2006-Dec-06 18:57 | kees | Thank you for the report.
For the upcoming version of Smb4K I rewrote Smb4KFileIO compeletely and writeFile() now indeed uses mkstemp instead of mktemp. Since mkstemp() is now used and because we restore the original permissions, I guess the second issue has also already been addressed. remove_lock_file() has been replaced with removeLockFile() which has some better file handling included, but I don't know whether it fixes the issue you reported.
May I ask you to also review our upcoming version? | 2006-Dec-06 07:47 | dustpuppy |
| |
|
|
No Other Bugs are Dependent on This Bug |
Bug Change History
| Field |
Old Value |
Date |
By |
| close_date | 1970-Jan-01 01:00 | 2006-Dec-21 19:09 | dustpuppy |
| resolution_id | None | 2006-Dec-21 19:09 | dustpuppy |
| status_id | Open | 2006-Dec-21 19:09 | dustpuppy |
|
|
|
