Bug 896 - Improper Input buffer handling
Summary: Improper Input buffer handling
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: -current
Hardware: All All
: P2 normal
Assignee: OpenSSH Bugzilla mailing list
URL:
Keywords: openbsd, patch
Depends on:
Blocks: 994
  Show dependency treegraph
 
Reported: 2004-07-20 04:21 AEST by Michael Stevens
Modified: 2006-10-07 11:36 AEST (History)
0 users

See Also:


Attachments
Proposed patch to fix this problem by limiting input buffer to 0x10000 (808 bytes, patch)
2004-07-20 04:23 AEST, Michael Stevens
no flags Details | Diff
Hack client to reproduce problem in sshd. (994 bytes, patch)
2005-02-11 12:05 AEDT, Darren Tucker
no flags Details | Diff
allow maximum utilization of input buffer (3.91 KB, patch)
2005-02-11 13:43 AEDT, Darren Tucker
no flags Details | Diff
Buffer only a reasonable number of packets' worth of input data (614 bytes, patch)
2005-02-11 13:55 AEDT, Darren Tucker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Stevens 2004-07-20 04:21:08 AEST
There are a set of circumstances that can cause sshd to incorrectly fatal()

1) Client advertises a large window > 0x10000 increment of 0xa0000 total
2) Server is sending more than above listed data to client
3) Client does not consume data as fast as server buffers it into channel->input
4) Server's channel->input buffer grows above limits specified in buffer.c
Comment 1 Michael Stevens 2004-07-20 04:23:18 AEST
Created attachment 685 [details]
Proposed patch to fix this problem by limiting input buffer to 0x10000
Comment 2 Darren Tucker 2005-02-11 12:05:14 AEDT
Created attachment 817 [details]
Hack client to reproduce problem in sshd.

After some discussion off-line with Chris Rapier, I am able to reproduce the
problem by in the server with a hacked client.	Apply the patch, rebuild, then
point it at a server running in debug mode and hit CTRL-C as soon as the
transfer starts:

$ ssh yourserver dd if=/dev/zero >/dev/null
[hit ctrl-c]

The server will eventually die with something like:
debug2: channel 0: read 328 from efd 12
debug2: channel 0: rwin 13107200 elen 328 euse 1
debug2: channel 0: sent ext data 328
buffer_append_space: alloc 10522624 not supported

The limit it hits is the maximum allocated size in buffer_append_space
(0xa00000) not the maximum buffer increment (0x100000).  It will never hit the
increment limit because the amount that will be added in any one increment is
limited by the read buffers (16k).

The patch in attachment #685 [details] will prevent the problem but does not look like
the correct fix, though, since it limits the buffer to a relatively small size.
 Simply changing the patch to have either of the above isn't strictly correct
either.

For maximum buffer you need to know how much you could safely add to the
buffer, however this is not a simple function of buffer_len() since the buffer
is consumed from the front and appended at the end, so up to half the buffer
may be unused before it's compacted to make room.

This can be fixed allowing maximum buffer utilization by adding a function to
the buffer code that returns the maximum amount that may be safely appended to
the buffer.

Alternatively, the buffer limit could be set to something arbitrary but under
the level where it could possibly exceed the maximum buffer->alloc limit (ie
less than 0xa00000 / 2 - 0x100000).
Comment 3 Darren Tucker 2005-02-11 13:43:57 AEDT
Created attachment 818 [details]
allow maximum utilization of input buffer

I've changed my mind: allowing input buffer to be used to its absolute maximum
(eg attached patch) doesn't make much sense.  10MB+ of buffered input is a bit
much...
Comment 4 Darren Tucker 2005-02-11 13:55:04 AEDT
Created attachment 819 [details]
Buffer only a reasonable number of packets' worth of input data

This patch allows buffering of up to 1MB of input data (equal to 32 default
sized or 64 max sized SSH packets).

Setting the limit too low may reduce the effectiveness of compression on highly
compressible data, and setting it high may result in excessive memory usage. 
Compressing 1MB of zeros with "gzip -6" reduces to ~1kB so this seems a
reasonable compromise.
Comment 5 Darren Tucker 2005-03-09 11:12:23 AEDT
Unfortunately this won't be in 4.0.
Comment 6 Darren Tucker 2005-03-14 23:34:19 AEDT
This has now been fixed in OpenBSD, Portable HEAD and the 4.0 branch, although
not in the same way as the patches here; it will allow approximately 10MB of
buffer usage before halting the read()s, and the buffer code is now a bit more
aggressive about compacting consumed buffers.  It tests OK with a pathological
hacked client (ie one that sets a window size of 2^32-1).

Thanks for the report.
Comment 7 Darren Tucker 2006-10-07 11:36:32 AEST
Change all RESOLVED bug to CLOSED with the exception of the ones fixed post-4.4.