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

[jelly]

To: openssh-unix-dev at mindrot.org
Subject: [PATCH] sftp: check the argument to -R for validity


[Resend: my previous copy of this message was reformatted in transit -- the
inline diff arrived with its tab indentation replaced by other characters and
with HTML entity escapes in place of ">", "<" and "&", so it could not be
applied.  Same content, sent as plain text this time; the patch is attached
as 0001-sftp-check-the-argument-to-R-for-validity.patch.]


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".


Patch attached: 0001-sftp-check-the-argument-to-R-for-validity.patch