Bug 1624949

Created attachment 1480585 [details]
Proposed patch for makeargv()

+++ This bug was initially created as a clone of Bug #1624938 +++

Description of problem:
The ftp client crashes on "Segmentation fault" when e.g. an "mput" command is used with many arguments (e.g. more than 40 files).

Version-Release number of selected component (if applicable):
ftp-0.17-54.el6.x86_64

How reproducible:
Always

Steps to Reproduce:
1. Open an FTP connection using the netkit ftp client:

    # ftp -i localhost
    Trying ::1...
    ftp: connect to address ::1Connection refused
    Trying 127.0.0.1...
    Connected to localhost (127.0.0.1).
    220 (vsFTPd 2.2.2)
    Name (localhost:root): foobar
    331 Please specify the password.
    Password:
    230 Login successful.
    Remote system type is UNIX.
    Using binary mode to transfer files.


2. Use the "mput" or "mget" FTP commands to transfer >40 files. It's the number of arguments that matter, the files need not to exist locally or on the FTP server to trigger the fault:

    ftp> mput a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a 
    local: a remote: a
    Segmentation fault (core dumped)

- or -

    ftp> mput
    (local-files) a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a 
    local: a remote: a
    Segmentation fault (core dumped)

Actual results:

    Core was generated by `ftp -i localhost'.
    Program terminated with signal 11, Segmentation fault.
    #0  0x00007fd010caf367 in ___vfprintf_chk (fp=0x622615, flag=1, format=0x40ec94 "TYPE %s", ap=0x7fff79909920) at vfprintf_chk.c:31
31	  _IO_acquire_lock_clear_flags2 (fp);

    (gdb) bt
    #0  0x00007fd010caf367 in ___vfprintf_chk (fp=0x622615, flag=1, format=0x40ec94 "TYPE %s", ap=0x7fff79909920) at vfprintf_chk.c:31
    #1  0x0000000000408a27 in vfprintf (fmt=<value optimized out>) at /usr/include/bits/stdio2.h:128
    #2  command (fmt=<value optimized out>) at ftp.c:424
    #3  0x0000000000402a3e in changetype (newtype=3, show=<value optimized out>) at cmds.c:387
    #4  0x000000000040aefa in sendrequest (cmd=0x40f22f "STOR", local=0xfb7d60 "a", remote=0xfb7d60 "a", printnames=<value optimized out>) at ftp.c:609
    #5  0x0000000000406cad in mput (argc=73, argv=0x61d0a0) at cmds.c:611
    #6  0x000000000040e06e in cmdscanner (argc=1, argv=0x7fff7990dd48) at main.c:353
    #7  main (argc=1, argv=0x7fff7990dd48) at main.c:226

    (gdb) up
    #1  0x0000000000408a27 in vfprintf (fmt=<value optimized out>) at /usr/include/bits/stdio2.h:128
    128	  return __vfprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt, __ap);

    (gdb) up
    #2  command (fmt=<value optimized out>) at ftp.c:424
    424		vfprintf(cout, fmt, ap);

    (gdb) print *cout
    $2 = {_flags = 6357089, _IO_read_ptr = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_read_end = 0x61006100610061 <Address 0x61006100610061 out of bounds>, 
  _IO_read_base = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_write_base = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_write_ptr = 0x61006100610061 <Address 0x61006100610061 out of bounds>, 
  _IO_write_end = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_buf_base = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_buf_end = 0x0, _IO_save_base = 0x0, _IO_backup_base = 0x0, _IO_save_end = 0x0, 
  _markers = 0x0, _chain = 0x0, _fileno = 0, _flags2 = 0, _old_offset = 0, _cur_column = 0, _vtable_offset = 0 '\000', _shortbuf = "", _lock = 0x0, _offset = 0, __pad1 = 0x0, __pad2 = 0x0, __pad3 = 0x0, __pad4 = 0x0, __pad5 = 0, _mode = 0, 
  _unused2 = '\000' <repeats 19 times>}

    (gdb) print *cin
    $1 = {_flags = 6357089, _IO_read_ptr = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_read_end = 0x61006100610061 <Address 0x61006100610061 out of bounds>, 
  _IO_read_base = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_write_base = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_write_ptr = 0x61006100610061 <Address 0x61006100610061 out of bounds>, 
  _IO_write_end = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_buf_base = 0x61006100610061 <Address 0x61006100610061 out of bounds>, _IO_buf_end = 0x61 <Address 0x61 out of bounds>, _IO_save_base = 0x0, _IO_backup_base = 0x0, 
  _IO_save_end = 0x0, _markers = 0x0, _chain = 0x0, _fileno = 0, _flags2 = 0, _old_offset = 0, _cur_column = 0, _vtable_offset = 0 '\000', _shortbuf = "", _lock = 0x0, _offset = 0, __pad1 = 0x0, __pad2 = 0x0, __pad3 = 0x0, __pad4 = 0x0, 
  __pad5 = 0, _mode = 0, _unused2 = '\000' <repeats 19 times>}

    (gdb) x/s &*cout
    0x622615 <argbuf+85>:	 "a"
    (gdb) x/s &*cin
    0x622613 <argbuf+83>:	 "a"

    (gdb) info var cout
    All variables matching regular expression "cout":
    
    File ftp.c:
    FILE *cout;
    (gdb) info var cin
    All variables matching regular expression "cin":

    File ftp.c:
    FILE *cin;

    The variables appear to be overwritten by the argument list somehow.

Expected results:
    All files uploaded/downloaded if they exist.

Additional info:
    The issue is reproducible on RHEL-7.5, but harder to trigger. Due to the different location of variables on the BSS segment, one needs to use cca 3000 arguments (fill the line limit, short or single-char file names) to trigger the fault.
    The fault on RHEL-7.5 may not happen immediately after the "mput" command is given, but e.g. on closing the client using the "bye" command.

    I have tracked the issue down to a buffer overflow in main.c:410 in makeargv():

    399 char **
    400 makeargv(int *pargc, char **parg)
    401 {
    402         static char *rargv[20];
    403         int rargc = 0;
    404         char **argp;
    405 
    406         argp = rargv;
    407         stringbase = line;              /* scan from first of buffer */
    408         argbase = argbuf;               /* store from first of buffer */
    409         slrflag = 0;
    410         while ((*argp++ = slurpstring())!=NULL)
    411                 rargc++;
    412 
    413         *pargc = rargc;
    414         if (parg) *parg = altarg;
    415         return rargv;
    416 }

    There appears to be a argument list limit of 20 arguments on line 402, which is, however, not checked at all anywhere in the code. Therefore using more than 20 arguments to any FTP command will cause buffer overflow that will overwrite variables present in the BSS segment. When a critical variable is hit, the program will crash.

    I have a working patch, which I will upload below.

--- Additional comment from Red Hat Bugzilla Rules Engine on 2018-09-03 12:13:33 EDT ---

Since this bug report was entered in Red Hat Bugzilla, the release flag has been set to ? to ensure that it is properly evaluated for this release.

--- Additional comment from Stepan Broz on 2018-09-03 12:24 EDT ---

I am proposing this patch which increases the max argument number to LINELEN, and adding a check for the maximum number.

The current maximum number of arguments (20) is not documented anywhere, and does not even have its own error message. Increasing the limit to LINELEN may seem as an overkill, but appears safe.

I believe the maximum number of arguments might in fact be LINELEN/2 as you need 2 chars to make an argument, but I'm not sure the several kB of BSS size is worth it.

Please share your thoughts.