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 #9631 ] security: weaknesses in utilities/smb4k_*.cpp
Date:
2006-Dec-05 21:21 |
Submitted By:
kees |
Assigned To:
none |
Category:
None |
Priority:
5 |
Bug Group:
None |
Resolution:
Fixed |
Summary:
security: weaknesses in utilities/smb4k_*.cpp |
Original Submission:
Hi, another report from my audit of smb4k. There is a design issue with smb4k_kill. Anyone in the sudoers list for smb4k can kill any process on the system. That could lead to all sorts of problems, but I'm not sure what the "right" solution might be.
Additionally, when used along with the "sudo" configuration, all of the tools have stack overflows with args variable, as well as other strcpy uses that could be a problem in the future. This could lead to full priv escalation for anyone in the smb4k sudoers list. I'd recommend checking the position in the args list (25 is the current size), and using strncpy.
Thanks! |
Followups
| Comment |
Date |
By | | The code of smb4k_mount and smb4k_umount has been rewritten and committed to BRANCH_08 CVS branch. FreeBSD support has to be added, though. | 2007-Mar-05 07:37 | dustpuppy | | Reopened. | 2007-Feb-19 13:28 | dustpuppy | The result of not escaping hyphens is that I can evade the restriction to mounting as SMBfs/CIFS:
ln -s -- /dev/$target -t
mkdir smbfs
sudo smb4k_mount --suid -- -t smbfs
and now my target device is mounted under the directory "smbfs".
These programs really must not be made available to untrusted users unless you're prepared to do far more thorough sanitisation. You are showing no sign of sufficient understanding of the language, let alone the problem, to do this. | 2007-Feb-18 22:49 | benh | In smb4k_mount, replace_special_characters() doesn't.
The characters it checks for include many letters, but don't include the hyphen (useful for adding options to the mount command!).
It returns an invalid pointer, consistent with findprog(). | 2007-Feb-18 22:22 | benh | Neither part of this bug has not been fixed. Please reopen it.
It's no good checking for array bounds overflow *after* the fact. The damage has already been done.
Also, findprog() returns a pointer to memory that is freed when the function exits. | 2007-Feb-18 21:11 | benh | | Fixed in 0.8.0 and security patches. | 2006-Dec-21 19:09 | dustpuppy | | Great! Sounds right. Thanks again for looking into it. :) | 2006-Dec-07 21:13 | kees | I implemented the use of strncpy you proposed where applicable. And "k" is now being check that it is not equal or greater than the size of the args array. For example:
if ( k >= 25 )
{
cerr << "smb4k_mount: There are too many arguments" << endl;
exit( EXIT_FAILURE );
} | 2006-Dec-07 08:16 | dustpuppy | Hi! Thanks for checking into this. That's good news that you're using strncpy now, however I'd like to suggest a safer approach.
Current you have this:
char *p = new char[100];
const char *prg = findprog( "kill" );
args[k] = strncpy( p, prg, strlen( prg ) + 1 );
While presently "findprog" won't produce anything unsafe, you probably want to get into the habit of using strncpy more safely. The above code is functionally the same as "strcpy" (since it limits the copy to the size of the source, not the destination). You want to be using something like this:
args[k] = strncpy( p, prg, 99 );
p[99]='\0';
That way, the string will be ever be longer than the allocated size (p is 100 bytes), and the final byte will always terminate the string.
The current CVS still doesn't check that "k" is less than 25, so that is still an issue. Imagine running 'sudo smb4k_kill 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26'. That 26th argument will be written beyond the allocated space for "args".
| 2006-Dec-06 18:44 | kees | I'm aware of the smb4k_kill problem. At the moment, however, I don't have an idea either how to fix it. I'll make up my mind.
The use of the strncpy is already implemented in version 0.8.0 of Smb4K. | 2006-Dec-06 07:52 | dustpuppy |
| |
|
|
No Other Bugs are Dependent on This Bug |
Bug Change History
| Field |
Old Value |
Date |
By |
| close_date | 2006-Dec-21 19:09 | 2007-Apr-08 12:27 | dustpuppy |
| resolution_id | None | 2007-Apr-08 12:27 | dustpuppy |
| priority | 9 | 2007-Apr-08 12:27 | dustpuppy |
| status_id | Open | 2007-Apr-08 12:27 | dustpuppy |
| priority | 5 | 2007-Feb-19 13:28 | dustpuppy |
| resolution_id | Fixed | 2007-Feb-19 13:28 | dustpuppy |
| status_id | Closed | 2007-Feb-19 13:28 | dustpuppy |
| 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 |
|
|
|
