[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v2 1/2] packet: Do not decrypt zero length rest of buffer
[Thread Prev] | [Thread Next]
- Subject: [PATCH v2 1/2] packet: Do not decrypt zero length rest of buffer
- From: Alan Dunn <amdunn@xxxxxxxxx>
- Reply-to: libssh@xxxxxxxxxx
- Date: Thu, 6 Feb 2014 08:12:46 -0600
- To: libssh@xxxxxxxxxx
- Cc: Alan Dunn <amdunn@xxxxxxxxx>
If we receive a packet of length exactly blocksize, then
packet_decrypt gets called on a buffer of size 0. The check at the
beginning of packet_decrypt indicates that the function should be
called on buffers of at least one blocksize, though the check allows
through zero length. As is packet_decrypt can return -1 when len is 0
because malloc can return NULL in this case: according to the ISO C
standard, malloc is free to return NULL or a pointer that can be freed
when size == 0, and uclibc by default will return NULL here (in
"non-glibc-compatible" mode). The net result is that when using
uclibc connections with libssh can anomalously fail.
Alternatively, packet_decrypt (and probably packet_encrypt for
consistency) could be made to always succeed on len == 0 without
depending on the behavior of malloc.
Thanks to Josh Berlin for bringing conneciton failures with uclibc to
my attention.
Signed-off-by: Alan Dunn <amdunn@xxxxxxxxx>
---
src/packet.c | 19 ++++++++++++-------
src/packet1.c | 19 +++++++++++++------
2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/packet.c b/src/packet.c
index 87b9100..e57e67e 100644
--- a/src/packet.c
+++ b/src/packet.c
@@ -152,7 +152,7 @@ int ssh_packet_socket_callback(const void *data, size_t receivedlen, void *user)
const uint8_t *packet;
int to_be_read;
int rc;
- uint32_t len, compsize, payloadsize;
+ uint32_t len, compsize, payloadsize, buffer_len;
uint8_t padding;
size_t processed = 0; /* number of byte processed from the callback */
@@ -251,12 +251,17 @@ int ssh_packet_socket_callback(const void *data, size_t receivedlen, void *user)
* Decrypt the rest of the packet (blocksize bytes already
* have been decrypted)
*/
- rc = packet_decrypt(session,
- ((uint8_t*)buffer_get_rest(session->in_buffer) + blocksize),
- buffer_get_rest_len(session->in_buffer) - blocksize);
- if (rc < 0) {
- ssh_set_error(session, SSH_FATAL, "Decrypt error");
- goto error;
+
+ /* The following check avoids decrypting zero bytes */
+ buffer_len = buffer_get_rest_len(session->in_buffer);
+ if (buffer_len != blocksize) {
+ rc = packet_decrypt(session,
+ ((uint8_t*)buffer_get_rest(session->in_buffer) + blocksize),
+ buffer_len - blocksize);
+ if (rc < 0) {
+ ssh_set_error(session, SSH_FATAL, "Decrypt error");
+ goto error;
+ }
}
/* copy the last part from the incoming buffer */
diff --git a/src/packet1.c b/src/packet1.c
index ec72f16..3284f17 100644
--- a/src/packet1.c
+++ b/src/packet1.c
@@ -106,7 +106,7 @@ int ssh_packet_socket_callback1(const void *data, size_t receivedlen, void *user
size_t processed=0;
uint32_t padding;
uint32_t crc;
- uint32_t len;
+ uint32_t len, buffer_len;
ssh_session session=(ssh_session)user;
switch (session->packet_state){
@@ -168,11 +168,16 @@ int ssh_packet_socket_callback1(const void *data, size_t receivedlen, void *user
* We decrypt everything, missing the lenght part (which was
* previously read, unencrypted, and is not part of the buffer
*/
- if (packet_decrypt(session,
- ssh_buffer_get_begin(session->in_buffer),
- ssh_buffer_get_len(session->in_buffer)) < 0) {
- ssh_set_error(session, SSH_FATAL, "Packet decrypt error");
- goto error;
+ buffer_len = ssh_buffer_get_len(session->in_buffer);
+ if (buffer_len > 0) {
+ int rc;
+ rc = packet_decrypt(session,
+ ssh_buffer_get_begin(session->in_buffer),
+ buffer_len);
+ if (rc < 0) {
+ ssh_set_error(session, SSH_FATAL, "Packet decrypt error");
+ goto error;
+ }
}
}
#ifdef DEBUG_CRYPTO
@@ -300,6 +305,8 @@ int packet_send1(ssh_session session) {
ssh_buffer_get_len(session->out_buffer));
#endif
+ /* session->out_buffer should have more than sizeof(uint32_t) bytes
+ in it as required for packet_encrypt */
packet_encrypt(session, (unsigned char *)ssh_buffer_get_begin(session->out_buffer) + sizeof(uint32_t),
ssh_buffer_get_len(session->out_buffer) - sizeof(uint32_t));
--
1.7.9.5
| [PATCH v2 0/2] Fix connection success dependency on malloc behavior | Alan Dunn <amdunn@xxxxxxxxx> |