Ticket #1902 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

Possible security risk in mcserv.c

Reported by: metux Owned by: slavazanko
Priority: major Milestone: 4.7.1
Component: mc-vfs Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: Votes for changeset: committed-master committed-stable

Description

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

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

Attachments

mcserv.diff (760 bytes) - added by pavlinux 10 years ago.

Change History

Changed 10 years ago by pavlinux

comment:1 Changed 10 years ago by slavazanko

  • Owner set to slavazanko
  • Status changed from new to accepted

comment:2 Changed 10 years ago by slavazanko

  • Version changed from version not selected to master
  • severity changed from no branch to on review
  • Milestone changed from 4.7 to 4.7.1

Created branch 1902_mcserv_possible_security_risk
changeset:b6d97698592e8dd2ba9ed5d247c3ad1e1190440d

Review, please.

comment:3 follow-up: ↓ 4 Changed 10 years ago by ossi

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

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 10 years ago by 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?!?!!

comment:5 in reply to: ↑ 4 Changed 10 years ago by 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.

comment:6 Changed 10 years ago by slavazanko

  • Blocked By 1897 added

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.

comment:7 Changed 10 years ago by slavazanko

  • Blocked By 1897 removed

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

Vote, please, for this my latest commit.

comment:8 follow-up: ↓ 9 Changed 10 years ago by 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 ?

comment:9 in reply to: ↑ 8 Changed 10 years ago by 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

comment:10 Changed 10 years ago by metux

  • Votes for changeset set to metux

comment:11 Changed 10 years ago by pavlinux

Heh...

сheck errno inseure, but

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


secure than

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


:)

Slaves standards.

comment:12 Changed 10 years ago by pavlinux

chroot

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

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

comment:13 Changed 10 years ago by iNode

  • Votes for changeset changed from metux to metux, iNode
  • severity changed from on review to approved

comment:14 Changed 10 years ago by slavazanko

  • Status changed from accepted to testing
  • Votes for changeset changed from metux, iNode to commited-master commited-stable
  • Resolution set to fixed
  • severity changed from approved to merged

comment:15 Changed 10 years ago by slavazanko

  • Status changed from testing to closed

comment:16 Changed 10 years ago by slavazanko

  • Status changed from closed to reopened
  • Resolution fixed deleted

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

comment:17 Changed 10 years ago by slavazanko

  • Status changed from reopened to accepted
  • Votes for changeset commited-master commited-stable deleted
  • severity changed from merged to on review

Branch recreated.
1902_mcserv_possible_security_risk (parent: master)

Changeset is: dfdb32b6cfa96ce4439183b9a0c7592a7b1c7949

Review.

comment:18 Changed 10 years ago by ossi

  • Votes for changeset set to ossi

comment:19 Changed 10 years ago by andrew_b

  • Votes for changeset changed from ossi to ossi andrew_b

comment:20 Changed 10 years ago by angel_il

  • Owner changed from slavazanko to angel_il
  • Votes for changeset changed from ossi andrew_b to ossi andrew_b angel_il
  • severity changed from on review to approved

comment:21 Changed 10 years ago by slavazanko

  • Owner changed from angel_il to slavazanko
  • Votes for changeset changed from ossi andrew_b angel_il to commited-master commited-stable
  • severity changed from approved to merged

comment:22 Changed 10 years ago by slavazanko

  • Status changed from accepted to testing
  • Resolution set to fixed

comment:23 Changed 10 years ago by slavazanko

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.