1 http://bugs.proftpd.org/show_bug.cgi?id=4014

     2 

     3 I addition, this patch backports the pr_str_get_nbytes() function.

     4 

     5 diff --git a/include/options.h b/include/options.h

     6 index 96fa35d..0b29bf7 100644

     7 --- a/include/options.h

     8 +++ b/include/options.h

     9 @@ -110,6 +110,13 @@

    10  # define PR_TUNABLE_XFER_BUFFER_SIZE	PR_TUNABLE_BUFFER_SIZE

    11  #endif

    12  

    13 +/* Maximum FTP command size.  For details on this size of 512KB, see

    14 + * the Bug#4014 discussion.

    15 + */

    16 +#ifndef PR_TUNABLE_CMD_BUFFER_SIZE

    17 +# define PR_TUNABLE_CMD_BUFFER_SIZE    (512 * 1024)

    18 +#endif

    19 +

    20  /* Maximum path length.  GNU HURD (and some others) do not define

    21   * MAXPATHLEN.  POSIX' PATH_MAX is mandated to be at least 256 

    22   * (according to some), so 1K, in the absense of MAXPATHLEN, should be

    23 diff --git a/include/str.h b/include/str.h

    24 index b4fed7c..1cf8724 100644

    25 --- a/include/str.h

    26 +++ b/include/str.h

    27 @@ -39,6 +39,7 @@ char *pstrndup(pool *, const char *, size_t);

    28  

    29  char *pr_str_strip(pool *, char *);

    30  char *pr_str_strip_end(char *, char *);

    31 +int pr_str_get_nbytes(const char *, const char *, off_t *);

    32  char *pr_str_get_word(char **, int);

    33  

    34  #define PR_STR_FL_PRESERVE_COMMENTS		0x0001

    35 diff --git a/modules/mod_core.c b/modules/mod_core.c

    36 index 18a47c2..922f4d1 100644

    37 --- a/modules/mod_core.c

    38 +++ b/modules/mod_core.c

    39 @@ -2240,18 +2240,44 @@ MODRET set_allowforeignaddress(cmd_rec *cmd) {

    40  }

    41  

    42  MODRET set_commandbuffersize(cmd_rec *cmd) {

    43 -  int size = 0;

    44 +  int res;

    45 +  size_t size = 0;

    46 +  off_t nbytes = 0;

    47    config_rec *c = NULL;

    48 +  const char *units = NULL;

    49 +

    50 +  if (cmd->argc < 2 || cmd->argc > 3) {

    51 +    CONF_ERROR(cmd, "wrong number of parameters")

    52 +  }

    53  

    54 -  CHECK_ARGS(cmd, 1);

    55    CHECK_CONF(cmd, CONF_ROOT|CONF_VIRTUAL|CONF_GLOBAL);

    56  

    57 -  /* NOTE: need to add checks for maximum possible sizes, negative sizes. */

    58 -  size = atoi(cmd->argv[1]);

    59 +  if (cmd->argc == 3) {

    60 +    units = cmd->argv[2];

    61 +  }

    62 +

    63 +  if (pr_str_get_nbytes(cmd->argv[1], units, &nbytes) < 0) {

    64 +    CONF_ERROR(cmd, pstrcat(cmd->tmp_pool, "unable to parse: ",

    65 +      cmd->argv[1], " ", units, ": ", strerror(errno), NULL));

    66 +  }

    67 +

    68 +  if (nbytes > PR_TUNABLE_CMD_BUFFER_SIZE) {

    69 +    char max[1024];

    70 +

    71 +    snprintf(max, sizeof(max)-1, "%lu", (unsigned long)

    72 +      PR_TUNABLE_CMD_BUFFER_SIZE);

    73 +    max[sizeof(max)-1] = '\0';

    74 +

    75 +    CONF_ERROR(cmd, pstrcat(cmd->tmp_pool, "size ", cmd->argv[1], units,

    76 +      "exceeds max size ", max, NULL));

    77 +  }

    78 +

    79 +  /* Possible truncation here, but only for an absurdly large size. */

    80 +  size = (size_t) nbytes;

    81  

    82    c = add_config_param(cmd->argv[0], 1, NULL);

    83 -  c->argv[0] = pcalloc(c->pool, sizeof(int));

    84 -  *((int *) c->argv[0]) = size;

    85 +  c->argv[0] = pcalloc(c->pool, sizeof(size_t));

    86 +  *((size_t *) c->argv[0]) = size;

    87  

    88    return PR_HANDLED(cmd);

    89  }

    90 diff --git a/src/main.c b/src/main.c

    91 index 3e6d637..660e14b 100644

    92 --- a/src/main.c

    93 +++ b/src/main.c

    94 @@ -466,42 +466,21 @@ static int _dispatch(cmd_rec *cmd, int cmd_type, int validate, char *match) {

    95    return success;

    96  }

    97  

    98 -/* Returns the appropriate maximum buffer length to use for FTP commands

    99 - * from the client, taking the CommandBufferSize directive into account.

   100 +/* Returns the appropriate maximum buffer size to use for FTP commands

   101 + * from the client.

   102   */

   103 -static long get_max_cmd_len(size_t buflen) {

   104 -  long res;

   105 -  int *bufsz = NULL;

   106 -  size_t default_cmd_bufsz;

   107 -

   108 -  /* It's possible for the admin to select a PR_TUNABLE_BUFFER_SIZE which

   109 -   * is smaller than PR_DEFAULT_CMD_BUFSZ.  We need to handle such cases

   110 -   * properly.

   111 -   */

   112 -  default_cmd_bufsz = PR_DEFAULT_CMD_BUFSZ;

   113 -  if (default_cmd_bufsz > buflen) {

   114 -    default_cmd_bufsz = buflen;

   115 -  }

   116 - 

   117 +static size_t get_max_cmd_sz(void) {

   118 +  size_t res;

   119 +  size_t *bufsz = NULL;

   120 +

   121    bufsz = get_param_ptr(main_server->conf, "CommandBufferSize", FALSE);

   122    if (bufsz == NULL) {

   123 -    res = default_cmd_bufsz;

   124 -

   125 -  } else if (*bufsz <= 0) {

   126 -    pr_log_pri(PR_LOG_WARNING, "invalid CommandBufferSize size (%d) given, "

   127 -      "using default buffer size (%lu) instead", *bufsz,

   128 -      (unsigned long) default_cmd_bufsz);

   129 -    res = default_cmd_bufsz;

   130 -

   131 -  } else if (*bufsz + 1 > buflen) {

   132 -    pr_log_pri(PR_LOG_WARNING, "invalid CommandBufferSize size (%d) given, "

   133 -      "using default buffer size (%lu) instead", *bufsz,

   134 -      (unsigned long) default_cmd_bufsz);

   135 -    res = default_cmd_bufsz;

   136 +    res = PR_DEFAULT_CMD_BUFSZ;

   137  

   138    } else {

   139 -    pr_log_debug(DEBUG1, "setting CommandBufferSize to %d", *bufsz);

   140 -    res = (long) *bufsz;

   141 +    pr_log_debug(DEBUG1, "setting CommandBufferSize to %lu",

   142 +      (unsigned long) *bufsz);

   143 +    res = *bufsz;

   144    }

   145  

   146    return res;

   147 @@ -509,21 +488,29 @@ static long get_max_cmd_len(size_t buflen) {

   148  

   149  int pr_cmd_read(cmd_rec **res) {

   150    static long cmd_bufsz = -1;

   151 -  char buf[PR_DEFAULT_CMD_BUFSZ+1] = {'\0'};

   152 +  static char *cmd_buf = NULL;

   153    char *cp;

   154 -  size_t buflen;

   155 +  size_t cmd_buflen;

   156  

   157    if (res == NULL) {

   158      errno = EINVAL;

   159      return -1;

   160    }

   161  

   162 +  if (cmd_bufsz == -1) {

   163 +    cmd_bufsz = get_max_cmd_sz();

   164 +  }

   165 +

   166 +  if (cmd_buf == NULL) {

   167 +    cmd_buf = pcalloc(session.pool, cmd_bufsz + 1);

   168 +  }

   169 +

   170    while (TRUE) {

   171      pr_signals_handle();

   172  

   173 -    memset(buf, '\0', sizeof(buf));

   174 +    memset(cmd_buf, '\0', cmd_bufsz);

   175  

   176 -    if (pr_netio_telnet_gets(buf, sizeof(buf)-1, session.c->instrm,

   177 +    if (pr_netio_telnet_gets(cmd_buf, cmd_bufsz, session.c->instrm,

   178          session.c->outstrm) == NULL) {

   179  

   180        if (errno == E2BIG) {

   181 @@ -544,9 +531,6 @@ int pr_cmd_read(cmd_rec **res) {

   182      break;

   183    }

   184  

   185 -  if (cmd_bufsz == -1)

   186 -    cmd_bufsz = get_max_cmd_len(sizeof(buf));

   187 -

   188    /* This strlen(3) is guaranteed to terminate; the last byte of buf is

   189     * always NUL, since pr_netio_telnet_gets() is told that the buf size is

   190     * one byte less than it really is.

   191 @@ -554,26 +538,28 @@ int pr_cmd_read(cmd_rec **res) {

   192     * If the strlen(3) says that the length is less than the cmd_bufsz, then

   193     * there is no need to truncate the buffer by inserting a NUL.

   194     */

   195 -  buflen = strlen(buf);

   196 -  if (buflen > (cmd_bufsz - 1)) {

   197 +  cmd_buflen = strlen(cmd_buf);

   198 +  if (cmd_buflen > cmd_bufsz) {

   199      pr_log_debug(DEBUG0, "truncating incoming command length (%lu bytes) to "

   200        "CommandBufferSize %lu; use the CommandBufferSize directive to increase "

   201 -      "the allowed command length", (unsigned long) buflen,

   202 +      "the allowed command length", (unsigned long) cmd_buflen,

   203        (unsigned long) cmd_bufsz);

   204 -    buf[cmd_bufsz - 1] = '\0';

   205 +    cmd_buf[cmd_bufsz-1] = '\0';

   206    }

   207  

   208 -  if (buflen &&

   209 -      (buf[buflen-1] == '\n' || buf[buflen-1] == '\r')) {

   210 -    buf[buflen-1] = '\0';

   211 -    buflen--;

   212 +  if (cmd_buflen &&

   213 +      (cmd_buf[cmd_buflen-1] == '\n' || cmd_buf[cmd_buflen-1] == '\r')) {

   214 +    cmd_buf[cmd_buflen-1] = '\0';

   215 +    cmd_buflen--;

   216  

   217 -    if (buflen &&

   218 -        (buf[buflen-1] == '\n' || buf[buflen-1] =='\r'))

   219 -      buf[buflen-1] = '\0';

   220 +    if (cmd_buflen &&

   221 +        (cmd_buf[cmd_buflen-1] == '\n' || cmd_buf[cmd_buflen-1] =='\r')) {

   222 +      cmd_buf[cmd_buflen-1] = '\0';

   223 +      cmd_buflen--;

   224 +    }

   225    }

   226  

   227 -  cp = buf;

   228 +  cp = cmd_buf;

   229    if (*cp == '\r')

   230      cp++;

   231  

   232 @@ -587,11 +573,11 @@ int pr_cmd_read(cmd_rec **res) {

   233       * command handlers themselves, via cmd->arg.  This small hack

   234       * reduces the burden on SITE module developers, however.

   235       */

   236 -    if (strncasecmp(cp, C_SITE, 4) == 0)

   237 +    if (strncasecmp(cp, C_SITE, 4) == 0) {

   238        flags |= PR_STR_FL_PRESERVE_WHITESPACE;

   239 +    }

   240  

   241      cmd = make_ftp_cmd(session.pool, cp, flags);

   242 -

   243      if (cmd) {

   244        *res = cmd;

   245      } 

   246 diff --git a/src/str.c b/src/str.c

   247 index d243a17..4f327bf 100644

   248 --- a/src/str.c

   249 +++ b/src/str.c

   250 @@ -367,6 +367,81 @@ char *pr_str_strip_end(char *s, char *ch) {

   251    return s;

   252  }

   253  

   254 +int pr_str_get_nbytes(const char *str, const char *units, off_t *nbytes) {

   255 +  off_t sz;

   256 +  char *ptr = NULL;

   257 +  float factor = 0.0;

   258 +

   259 +  if (str == NULL) {

   260 +    errno = EINVAL;

   261 +    return -1;

   262 +  }

   263 +

   264 +  /* No negative numbers. */

   265 +  if (*str == '-') {

   266 +    errno = EINVAL;

   267 +    return -1;

   268 +  }

   269 +

   270 +  if (units == NULL ||

   271 +      *units == '\0') {

   272 +    factor = 1.0;

   273 +

   274 +  } else if (strncasecmp(units, "KB", 3) == 0) {

   275 +    factor = 1024.0;

   276 +

   277 +  } else if (strncasecmp(units, "MB", 3) == 0) {

   278 +    factor = 1024.0 * 1024.0;

   279 +

   280 +  } else if (strncasecmp(units, "GB", 3) == 0) {

   281 +    factor = 1024.0 * 1024.0 * 1024.0;

   282 +

   283 +  } else if (strncasecmp(units, "TB", 3) == 0) {

   284 +    factor = 1024.0 * 1024.0 * 1024.0 * 1024.0;

   285 +

   286 +  } else if (strncasecmp(units, "B", 2) == 0) {

   287 +    factor = 1.0;

   288 +

   289 +  } else {

   290 +    errno = EINVAL;

   291 +    return -1;

   292 +  }

   293 +

   294 +  errno = 0;

   295 +

   296 +#ifdef HAVE_STRTOULL

   297 +  sz = strtoull(str, &ptr, 10);

   298 +#else

   299 +  sz = strtoul(str, &ptr, 10);

   300 +#endif /* !HAVE_STRTOULL */

   301 +

   302 +  if (errno == ERANGE) {

   303 +    return -1;

   304 +  }

   305 +

   306 +  if (ptr != NULL && *ptr) {

   307 +    /* Error parsing the given string */

   308 +    errno = EINVAL;

   309 +    return -1;

   310 +  }

   311 +

   312 +  /* Don't bother applying the factor if the result will overflow the result. */

   313 +#ifdef ULLONG_MAX

   314 +  if (sz > (ULLONG_MAX / factor)) {

   315 +#else

   316 +  if (sz > (ULONG_MAX / factor)) {

   317 +#endif /* !ULLONG_MAX */

   318 +    errno = ERANGE;

   319 +    return -1;

   320 +  }

   321 +

   322 +  if (nbytes != NULL) {

   323 +    *nbytes = (off_t) (sz * factor);

   324 +  }

   325 +

   326 +  return 0;

   327 +}

   328 +

   329  char *pr_str_get_word(char **cp, int flags) {

   330    char *res, *dst;

   331    char quote_mode = 0;

   332 diff --git a/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm b/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm

   333 index ed4672a..a57c898 100644

   334 --- a/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm

   335 +++ b/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm

   336 @@ -94,6 +94,8 @@ sub cmdbuffersz_small {

   337      die("Can't open $test_path: $!");

   338    }

   339  

   340 +  my $idle_timeout = 3;

   341 +

   342    my $config = {

   343      PidFile => $pid_file,

   344      ScoreboardFile => $scoreboard_file,

   345 @@ -103,6 +105,7 @@ sub cmdbuffersz_small {

   346      AuthGroupFile => $auth_group_file,

   347  

   348      CommandBufferSize => $cmdbufsz,

   349 +    TimeoutIdle => $idle_timeout,

   350  

   351      IfModules => {

   352        'mod_delay.c' => {

   353 @@ -128,44 +131,16 @@ sub cmdbuffersz_small {

   354    defined(my $pid = fork()) or die("Can't fork: $!");

   355    if ($pid) {

   356      eval {

   357 -      my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port);

   358 +      my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port, 0, 1);

   359        $client->login($user, $passwd);

   360  

   361 -      my $conn = $client->list_raw($test_file);

   362 -      unless ($conn) {

   363 -        die("Failed to LIST $test_file: " . $client->response_code() . " " .

   364 -          $client->response_msg());

   365 +      # Since our filename is longer than the CommandBufferSize, proftpd

   366 +      # should simply ignore this.  It will fail because of the idle timeout

   367 +      # first.

   368 +      eval { $client->stat($test_file) };

   369 +      unless ($@) {

   370 +        die("STAT command succeeded unexpectedly");

   371        }

   372 -

   373 -      my $buf;

   374 -      $conn->read($buf, 8192, 30);

   375 -      eval { $conn->close() };

   376 -

   377 -      my $resp_code = $client->response_code();

   378 -      my $resp_msg = $client->response_msg();

   379 -

   380 -      # CommandBufferSize works by truncating any input longer than the

   381 -      # configured length.  (It should arguably reject such longer input,

   382 -      # but that is a different consideration.)

   383 -      #

   384 -      # Since our file name is longer than the CommandBufferSize, it means

   385 -      # the path will be truncated, and the LIST should return 450.

   386 -

   387 -      my $expected;

   388 -

   389 -      $expected = 450;

   390 -      $self->assert($expected == $resp_code,

   391 -        test_msg("Expected $expected, got $resp_code"));

   392 -

   393 -      # This length is CommandBufferSize - "LIST"(4) - " "(1) - 1 for

   394 -      # the NUL reserved in the code.  Thus CommandBufferSize - 6.

   395 -      my $truncated_name = ("A" x ($cmdbufsz - 6));

   396 -

   397 -      $expected = "$truncated_name: No such file or directory";

   398 -      $self->assert($expected eq $resp_msg,

   399 -        test_msg("Expected '$expected', got '$resp_msg'"));

   400 -

   401 -      $client->quit();

   402      };

   403  

   404      if ($@) {

   405 @@ -176,7 +151,7 @@ sub cmdbuffersz_small {

   406      $wfh->flush();

   407  

   408    } else {

   409 -    eval { server_wait($config_file, $rfh) };

   410 +    eval { server_wait($config_file, $rfh, 15) };

   411      if ($@) {

   412        warn($@);

   413        exit 1;

   414