Possible security risk in mcserv.c

[metux]

Please have a look at: mcserv.c:1019.

The chroot() call's return value isn't handled - this may a security risk.

[slavazanko]

Created branch 1902_mcserv_possible_security_risk
changeset:b6d97698592e8dd2ba9ed5d247c3ad1e1190440d

Review, please.

[ossi]

the "errno = 0" and the "if (...

errno != 0)" make no sense whatsoever, they only serve to make the code less readable

[pavlinux]

Replying to ossi:

the "errno = 0" and the "if (...

errno != 0)" make no sense whatsoever,

they only serve to make the code less readable

And you can argue with probability 1, that chroot() can run for properly,
the system does not return error?!?!!

[ossi]

Replying to pavlinux:

And you can argue with probability 1, that chroot() can run for properly,
the system does not return error?!?!!

i prefer to trust the man page (which is based on several standards). one can contrive an arbitrary number of ways the system could be broken, and you can be almost sure that an actual breakage will still be an uncovered case. so littering the code with workarounds for invented bugs is totally pointless - even counterproductive, as more complicated code is more likely to be buggy and thus insecure.

[slavazanko]

Branch 1897_libc_return_values have commit:

Author: Enrico Weigelt, metux IT service <weigelt@metux.de>
Date:   Mon Dec 28 23:56:16 2009 +0100

    VFS: mcserv: denying access when chroot required but failed

diff --git a/vfs/mcserv.c b/vfs/mcserv.c
index 23f7ffe..68a4548 100644
--- a/vfs/mcserv.c
+++ b/vfs/mcserv.c
@@ -1016,7 +1016,8 @@ do_auth (const char *username, const char *password)
        return 0;

     if (strcmp (username, "ftp") == 0)
-       chroot (this->pw_dir);
+       if (chroot (this->pw_dir) == -1)
+           return 0;

     endpwent ();
     return auth;

Looks like this problem.

[slavazanko]

(In #1897) * e579dad69b30f2b6473ac9f5c7761e0727f8af5b: Fixed a little bit broken logic after return values fuxup.

Vote, please, for this my latest commit.

[metux]

yep, i encountered it on the unhandled-returns issue,
but wasnt really sure how to handle it.

pavlinux's patch looks good, IMHO. should we take it ?

[andrew_b]

Replying to metux:

pavlinux's patch looks good, IMHO. should we take it ?

Seems the portability problem.
Look at ​http://www.kernel.org/doc/man-pages/online/pages/man3/error.3.html

[pavlinux]

Heh...

сheck errno inseure, but

if (strcmp(username, "ftp") == 0) {

secure than

if (strncmp(username, "ftp", 3) == 0) {

:)

Slaves standards.

[pavlinux]

chroot

SUSv2 (marked LEGACY).
And this function is not part of POSIX.1-2001.

Какая в жопу портабельность?!

[slavazanko]

master: b3ea5f8ceb1fde19c120d6fc675cfd391afc7ee0
4.7.0-stable: 4b3c4b339ea48d734003ab1c512dcc3d5f5ef3eb

[slavazanko]

Need to respect these thinks of Oswald Buddenhagen :
​http://groups.google.com/group/mc-commits/browse_thread/thread/3573614c518ad4f8

[slavazanko]

Branch recreated.
1902_mcserv_possible_security_risk (parent: master)

Changeset is: dfdb32b6cfa96ce4439183b9a0c7592a7b1c7949

Review.

[slavazanko]

Master: 7ea4dfb4ca811202e796edf9b46edf0e93dbd2ba
4.7.0-stable: 532c186d7b99c02c5ed7b22157f4d03b42b88b05