updated: [b3ea5f8] Ticket #1902: Possible security risk in mcserv.c

[Slava Zanko]

The following commit has been merged in the master branch:
commit b3ea5f8ceb1fde19c120d6fc675cfd391afc7ee0
Author: Pavel Vasilyev <pa...@pavlinux.ru>
Date: Tue Jan 12 17:36:01 2010 +0200

Ticket #1902: Possible security risk in mcserv.c

Look at mcserv.c near 1019
The chroot() call's return value isn't handled - this may a security risk.

Signed-off-by: Slava Zanko <slava...@gmail.com>

diff --git a/lib/vfs/mc-vfs/mcserv.c b/lib/vfs/mc-vfs/mcserv.c
index 18b79f2..c0cd7bf 100644
--- a/lib/vfs/mc-vfs/mcserv.c
+++ b/lib/vfs/mc-vfs/mcserv.c
@@ -56,6 +56,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/wait.h>
+#include <error.h>
#include <errno.h>
#include <signal.h>
#ifdef HAVE_GETOPT_H
@@ -1015,8 +1016,14 @@ do_auth (const char *username, const char *password)
if (getuid () != this->pw_uid)
return 0;

- if (strcmp (username, "ftp") == 0)
- chroot (this->pw_dir);
+ if (strncmp(username, "ftp", 3) == 0) {
+ errno = 0;
+ if (chroot(this->pw_dir) != 0 || errno != 0) {
+ auth = errno;
+ error(0, errno, strerror(errno));
+ return (-auth);
+ }
+ }

endpwent ();
return auth;

--
Midnight Commander Development

[Oswald Buddenhagen]

On Thu, Feb 04, 2010 at 10:18:24AM +0100, Slava Zanko wrote:
> diff --git a/lib/vfs/mc-vfs/mcserv.c b/lib/vfs/mc-vfs/mcserv.c
> index 18b79f2..c0cd7bf 100644
> --- a/lib/vfs/mc-vfs/mcserv.c
> +++ b/lib/vfs/mc-vfs/mcserv.c

> @@ -1015,8 +1016,14 @@ do_auth (const char *username, const char *password)
> if (getuid () != this->pw_uid)
> return 0;
>
> - if (strcmp (username, "ftp") == 0)
> - chroot (this->pw_dir);
> + if (strncmp(username, "ftp", 3) == 0) {
> + errno = 0;
> + if (chroot(this->pw_dir) != 0 || errno != 0) {
> + auth = errno;
> + error(0, errno, strerror(errno));
> + return (-auth);
> + }
> + }
>

this is just great. first, you decided to ignore my advice about not
obfuscating the code with nonsense-checks, and on top of that you
broke the string comparison (just see what happens when you try a user
named "ftpmaster"). how about you listen to someone with a clue instead
of a guy who doesn't seem to be capable of formulating a coherent thought?