Ticket #1902 (closed defect: fixed)

Opened 9 years ago

Last modified 9 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 9 years ago.

Change History

Changed 9 years ago by pavlinux

comment:1 Changed 9 years ago by slavazanko

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

comment:2 Changed 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 years ago by metux

  • Votes for changeset set to metux

comment:11 Changed 9 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 9 years ago by pavlinux

chroot

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

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

comment:13 Changed 9 years ago by iNode

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

comment:14 Changed 9 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 9 years ago by slavazanko

  • Status changed from testing to closed

comment:16 Changed 9 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 9 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 9 years ago by ossi

  • Votes for changeset set to ossi

comment:19 Changed 9 years ago by andrew_b

  • Votes for changeset changed from ossi to ossi andrew_b

comment:20 Changed 9 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 9 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 9 years ago by slavazanko

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

comment:23 Changed 9 years ago by slavazanko

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