• Bug#265709: vsftpd: Does not properly report errors

    From Aaron Isotton@1:229/2 to All on Sat Aug 14 17:00:12 2004
    From: [email protected]

    This is a multi-part MIME message sent by reportbug.

    Package: vsftpd
    Version: 2.0.1-1
    Severity: minor
    Tags: patch

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA1

    Vsftpd does not report the system error (as reported in errno) when some
    action cannot be done; in this session, the user "encoder" does not have the necessary permissions to create a directory in the ftp root, but it does have the permissions to do so in the directory "temp". To the user, there is no difference.

    Example session:

    aisotton@zarathustra:~$ ftp tiger
    Connected to tiger.internal.berg21.ch.
    220 (vsFTPd 2.0.1)
    Name (tiger:aisotton): encoder
    331 Please specify the password.
    Password:
    230 Login successful.
    Remote system type is UNIX.
    Using binary mode to transfer files.
    ftp> mkdir xxx
    550 Create directory operation failed.
    ftp> cd temp
    250 Directory successfully changed.
    ftp> mkdir xxx
    257 "/temp/xxx" created
    ftp> mkdir xxx
    550 Create directory operation failed.
    ftp> 221 Goodbye.

    Example session with my patch applied (different lines are marked with '*'):

    aisotton@zarathustra:~$ ftp tiger
    Connected to tiger.internal.berg21.ch.
    220 (vsFTPd 2.0.1)
    Name (tiger:aisotton): encoder
    331 Please specify the password.
    Password:
    230 Login successful.
    Remote system type is UNIX.
    Using binary mode to transfer files.
    ftp> mkdir xx
    * 550 Create directory operation failed: Permission denied.
    ftp> cd temp
    250 Directory successfully changed.
    ftp> mkdir xxx
    257 "/temp/xxx" created
    ftp> mkdir xxx
    * 550 Create directory operation failed: File exists.
    ftp> 221 Goodbye.

    This may not be important for human users, but it is important for programs; specifically, my program needs to create a directory with a specific name; if the directory cannot be created because it already exists, it must create a directory with another name; if it cannot be created for some other reason (such as 'permission denied'), it must fail.

    The new function 'vsf_cmdio_printf' could also be used for similar error
    output of other functions; as far as I can see, it should not introduce any
    new bugs or security holes.

    Greetings,
    Aaron

    - -- System Information:
    Debian Release: 3.1
    APT prefers unstable
    APT policy: (500, 'unstable')
    Architecture: i386 (i686)
    Kernel: Linux 2.6.7-zarathustra
    Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8

    Versions of packages vsftpd depends on:
    ii adduser 3.59 Add and remove users and groups
    ii libc6 2.3.2.ds1-16 GNU C Library: Shared libraries an ii libcap1 1:1.10-14 support for getting/setting POSIX. ii libpam-modules 0.76-22 Pluggable Authentication Modules f ii libpam-runtime 0.76-22 Runtime support for the PAM librar ii libpam0g 0.76-22 Pluggable Authentication Modules l ii libssl0.9.7 0.9.7d-5 SSL shared libraries
    ii libwrap0 7.6.dbs-5 Wietse Venema's TCP wrappers libra

    - -- no debconf information

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.2.5 (GNU/Linux)

    iD8DBQFBHiMxm2HPKfVbHyoRAlE1AKDoA2Lyaofcpy7vOWoGKsKsDSX+nwCg0mZG cXcuphkd0zh6PSztT9qXMxw=
    =snT6
    -----END PGP SIGNATURE-----

    diff -u vsftpd-2.0.1/ftpcmdio.c vsftpd-2.0.1-aisotton/ftpcmdio.c
    --- vsftpd-2.0.1/ftpcmdio.c 2004-07-02 13:23:02.000000000 +0200
    +++ vsftpd-2.0.1-aisotton/ftpcmdio.c 2004-08-14 16:18:07.000000000 +0200
    @@ -19,6 +19,8 @@
    #include "logging.h"
    #include "session.h"
    #include "readwrite.h"
    +#include <stdarg.h>
    +#include <stdio.h>

    /* Internal functions */
    static void control_getline(struct mystr* p_str, struct vsf_session* p_sess); @@ -52,6 +54,19 @@
    }

    void
    +vsf_cmdio_printf(struct vsf_session* p_sess, int status, const char* p_fmt,
    + ...)
    +{
    + char buf[1000];
    + va_list ap;
    + va_start(ap, p_fmt);
    + vsnprintf(buf, sizeof(buf), p_fmt, ap);
    + va_end(ap);
    + ftp_write_text_common(p_sess, status, buf, 0, ' ');
    +}
    +
    +
    +void
    vsf_cmdio_write_hyphen(struct vsf_session* p_sess, int status,
    const char* p_text)
    {
    diff -u vsftpd-2.0.1/ftpcmdio.h vsftpd-2.0.1-aisotton/ftpcmdio.h
    --- vsftpd-2.0.1/ftpcmdio.h 2003-09-10 01:11:01.000000000 +0200
    +++ vsftpd-2.0.1-aisotton/ftpcmdio.h 2004-08-14 16:15:35.000000000 +0200
    @@ -2
  • From Daniel Jacobowitz@1:229/2 to Aaron Isotton on Mon Aug 16 16:00:15 2004
    From: [email protected]

    On Sat, Aug 14, 2004 at 04:35:29PM +0200, Aaron Isotton wrote:
    Package: vsftpd
    Version: 2.0.1-1
    Severity: minor
    Tags: patch

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA1

    Vsftpd does not report the system error (as reported in errno) when some action cannot be done; in this session, the user "encoder" does not have the necessary permissions to create a directory in the ftp root, but it does have the permissions to do so in the directory "temp". To the user, there is no difference.

    I don't know if the current behavior was deliberate. Chris, what do
    you think of this patch?


    Example session:

    aisotton@zarathustra:~$ ftp tiger
    Connected to tiger.internal.berg21.ch.
    220 (vsFTPd 2.0.1)
    Name (tiger:aisotton): encoder
    331 Please specify the password.
    Password:
    230 Login successful.
    Remote system type is UNIX.
    Using binary mode to transfer files.
    ftp> mkdir xxx
    550 Create directory operation failed.
    ftp> cd temp
    250 Directory successfully changed.
    ftp> mkdir xxx
    257 "/temp/xxx" created
    ftp> mkdir xxx
    550 Create directory operation failed.
    ftp> 221 Goodbye.

    Example session with my patch applied (different lines are marked with '*'):

    aisotton@zarathustra:~$ ftp tiger
    Connected to tiger.internal.berg21.ch.
    220 (vsFTPd 2.0.1)
    Name (tiger:aisotton): encoder
    331 Please specify the password.
    Password:
    230 Login successful.
    Remote system type is UNIX.
    Using binary mode to transfer files.
    ftp> mkdir xx
    * 550 Create directory operation failed: Permission denied.
    ftp> cd temp
    250 Directory successfully changed.
    ftp> mkdir xxx
    257 "/temp/xxx" created
    ftp> mkdir xxx
    * 550 Create directory operation failed: File exists.
    ftp> 221 Goodbye.

    This may not be important for human users, but it is important for programs; specifically, my program needs to create a directory with a specific name; if the directory cannot be created because it already exists, it must create a directory with another name; if it cannot be created for some other reason (such as 'permission denied'), it must fail.

    Patch aside, it sounds like you should be getting a directory listing
    anyway.


    The new function 'vsf_cmdio_printf' could also be used for similar error output of other functions; as far as I can see, it should not introduce any new bugs or security holes.

    Greetings,
    Aaron

    - -- System Information:
    Debian Release: 3.1
    APT prefers unstable
    APT policy: (500, 'unstable')
    Architecture: i386 (i686)
    Kernel: Linux 2.6.7-zarathustra
    Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8

    Versions of packages vsftpd depends on:
    ii adduser 3.59 Add and remove users and groups ii libc6 2.3.2.ds1-16 GNU C Library: Shared libraries an
    ii libcap1 1:1.10-14 support for getting/setting POSIX.
    ii libpam-modules 0.76-22 Pluggable Authentication Modules f
    ii libpam-runtime 0.76-22 Runtime support for the PAM librar
    ii libpam0g 0.76-22 Pluggable Authentication Modules l
    ii libssl0.9.7 0.9.7d-5 SSL shared libraries
    ii libwrap0 7.6.dbs-5 Wietse Venema's TCP wrappers libra

    - -- no debconf information

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.2.5 (GNU/Linux)

    iD8DBQFBHiMxm2HPKfVbHyoRAlE1AKDoA2Lyaofcpy7vOWoGKsKsDSX+nwCg0mZG cXcuphkd0zh6PSztT9qXMxw=
    =snT6
    -----END PGP SIGNATURE-----

    diff -u vsftpd-2.0.1/ftpcmdio.c vsftpd-2.0.1-aisotton/ftpcmdio.c
    --- vsftpd-2.0.1/ftpcmdio.c 2004-07-02 13:23:02.000000000 +0200
    +++ vsftpd-2.0.1-aisotton/ftpcmdio.c 2004-08-14 16:18:07.000000000 +0200
    @@ -19,6 +19,8 @@
    #include "logging.h"
    #include "session.h"
    #include "readwrite.h"
    +#include <stdarg.h>
    +#include <stdio.h>

    /* Internal functions */
    static void control_getline(struct mystr* p_str, struct vsf_session* p_sess);
    @@ -52,6 +54,19 @@
    }

    void
    +vsf_cmdio_printf(struct vsf_session* p_sess, int status, const char* p_fmt, + ...)
    +{
    + char buf[1000];
    + va_list ap;
    + va_start(ap, p_fmt);
    + vsnprintf(buf, sizeof(buf), p_fmt, ap);
    + va_end(ap);
    + ftp_write_text_common(p_sess, status, buf, 0, ' ');
    +}
    +
    +
    +void
    vsf_cmdio_write_hyphen(struct vsf_session* p_sess, int status,
    const char* p_text)
    {
    diff -u vsftpd-2.0.1/ftpcmdio.h vsftpd-2.0.1-aisotton/ftpcmdio.h
    --- vsftpd-2.0.1/ftpcmdio.h 2003-09-10 01:11:01.000000000 +0200
    +++ vsftpd-2.0.1-aisotton/ftpcmdio.h 2004-08-14 16:15:35.000000000 +0200
    @@ -22,6 +22,20 @@
    void vsf_cmdio_write(struct vsf_session* p_sess, int status,
    const char* p_text);

    +/* vsf_cmdio_printf()
    + * PURPOSE
    + * Same as vsf_cmdio_write(), but with printf()-style formatting.
    + * PARAMETERS
    + * p_sess - the current session object
    + * status - the status code to report
    + * p_fmt - the printf-style formatting string
    + * ... - the arguments to convert and insert into the formatting
    + * string
    + */
    +
    +void vsf_cmdio_printf(struct vsf_session* p_sess, int status,
    + const char* p_fmt, ...);
    +
    /* vsf_cmdio_write_hyphen()
    * PURPOSE
    * Write a response to the FTP control connection, with a hyphen '-'
    Common subdirectories: vsftpd-2.0.1/port and vsftpd-2.0.1-aisotton/port
    diff -u vsftpd-2.0.1/postlogin.c vsftpd-2.0.1-aisotton/postlogin.c
    --- vsftpd-2.0.1/postlogin.c 2004-07-02 13:24:01.000000000 +0200
    +++ vsftpd-2.0.1-aisotton/postlogin.c 2004-08-14 16:24:22.000000000 +0200
    @@ -26,6 +26,8 @@
    #include "features.h"
    #include "ssl.h"
    #include "vsftpver.h"
    +#include <errno.h>
    +#include <string.h>

    /* Private local functions */
    static void handle_pwd(struct vsf_session* p_sess);
    @@ -1066,8 +1068,8 @@
    retval = str_mkdir(&p_sess->ftp_arg_str, 0777);
    if (retval != 0)
    {
    - vsf_cmdio_write(p_sess, FTP_FILEFAIL,

    [continued in next message]

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Daniel Jacobowitz@1:229/2 to [email protected] on Tue Aug 17 00:40:09 2004
    From: [email protected]

    tags 265709 + wontfix
    thanks

    On Mon, Aug 16, 2004 at 11:09:51PM +0100, [email protected] wrote:
    Hi-

    On Mon, 16 Aug 2004, Daniel Jacobowitz wrote:

    On Sat, Aug 14, 2004 at 04:35:29PM +0200, Aaron Isotton wrote:
    Package: vsftpd
    Version: 2.0.1-1
    Severity: minor
    Tags: patch

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA1

    Vsftpd does not report the system error (as reported in errno) when some action cannot be done; in this session, the user "encoder" does not have the
    necessary permissions to create a directory in the ftp root, but it does have
    the permissions to do so in the directory "temp". To the user, there is no
    difference.

    I don't know if the current behavior was deliberate. Chris, what do
    you think of this patch?

    Current behaviour is deliberate; generally, it's good practice to give as little information as possible when telling the user an access control
    check failed.

    That's what I figured. Thanks.

    (Aaron, an example: I don't think your patch handled the case of
    creating a directory in a directory which the user was allowed to write
    to but not list. This would be a file-identification technique, if a
    somewhat useless one.)

    --
    Daniel Jacobowitz


    --
    To UNSUBSCRIBE, email to [email protected]
    with a subject of "unsubscribe". Trouble? Contact [email protected]

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)