[PATCH] sftp: check the argument to -R for validity

[jelly]

Hi,


the -R option of sftp(1) is parsed with strtol(3) and the result is
assigned to a size_t without any range check beyond rejecting zero and
trailing garbage. Two problems follow from that:


  - a negative value wraps around when it is truncated to the u_int
    num_requests member of struct sftp_conn, e.g. "-R -5" becomes
    4294967291;
  - a value larger than the range accepted by the equivalent
    "-X nrequests=" option (1..262144) is silently truncated.


num_requests is the client's in-flight request limit, so a negative
value effectively disables flow control: every request for the file is
sent without waiting for replies. Measured against a local sftp-server,
a 400MB download with "-R 64" peaks at 64 outstanding requests while
"-R -5" peaks at 767, i.e. every request for the whole file, with the
server having to absorb them all at once.


Fix it by parsing the argument with strtonum(3), the same call already
used for "-X nrequests=", and report the reason for rejection. Document
the maximum in sftp.1.


Tested on current master (4001f0a):
  - -R 1 / -R 64 / -R 262144 are still accepted;
  - -R 0 / -R -1 / -R 262145 / -R 4000000000 are now rejected;
  - "abc" / "12x" are still rejected; "make unit" passes.


The same change is pushed to the immortal-jelly/openssh-portable branch
"sftp-R-request-count".


---
diff --git a/sftp.1 b/sftp.1
index a30338b..c220059 100644
--- a/sftp.1
+++ b/sftp.1
@@ -339,6 +339,7 @@ Specify how many requests may be outstanding at any one time.
 Increasing this may slightly improve file transfer speed
 but will increase memory usage.
 The default is 64 outstanding requests.
+The maximum supported value is 262144.
 .It Fl r
 Recursively copy entire directories when uploading and downloading.
 Note that
diff --git a/sftp.c b/sftp.c
index fdc5167..5350ca4 100644
--- a/sftp.c
+++ b/sftp.c
@@ -2642,10 +2642,16 @@ main(int argc, char **argv)
                  global_rflag = 1;
                  break;
            case 'R':
-                 num_requests = strtol(optarg, &cp, 10);
-                 if (num_requests == 0 || *cp != '\0')
-                       fatal("Invalid number of requests \"%s\"",
-                           optarg);
+                 /*
+                  * Bound the value so that a negative argument cannot
+                  * wrap into an enormous size_t; -X nrequests= uses
+                  * the same range.
+                  */
+                 llv = strtonum(optarg, 1, 256 * 1024, &errstr);
+                 if (errstr != NULL)
+                       fatal("Invalid number of requests \"%s\": %s",
+                           optarg, errstr);
+                 num_requests = (size_t)llv;
                  break;
            case 's':
                  sftp_server = optarg;