search

marcepunk / ganymed-ssh-2

Automatically exported from code.google.com/p/ganymed-ssh-2
Other
0 stars 0 forks source link

Bad buffer length check may result in memory access violation #22

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
The problem can be seen here:

    if (payload_length >= len)
        throw new IOException("Receive buffer too small (" + len + ", need " + payload_length + ")");

    cis.read(buffer, off, payload_length);

http://code.google.com/p/ganymed-ssh-2/source/browse/trunk/src/main/java/ch/ethz
/ssh2/transport/TransportConnection.java#253

The `buffer` length check is done against `len`, but then writing is done in 
`cis.read` starting at offset `off`. If `off+payload_length` is greater than 
`len`, then there will be an attempt to write beyond the end of `buffer`.

The fix is to change the check to:
    if ((off + payload_length) >= len)

(One way I could be wrong, that I'm not 100% sure of: If there's some guarantee 
that `len` is "buffer size minus offset". But I don't see that that's the case.)

Original issue reported on code.google.com by a.pritch...@psiphon.ca on 10 Apr 2013 at 5:43

GoogleCodeExporter commented 9 years ago 
Yes, we do not check whether the caller is intelligent enough to pass a correct 
"len" parameter =) You are completely right.

However...

* The method will never be called by third-party code
* At the only place where our own code uses it, we pass an offset of "0" and 
length == buffer.length ("tc.receiveMessage(msg, 0, msg.length)")

Original comment by cleondris on 10 Jun 2013 at 3:46