On 31.03.2019 14:23, Yuriy M. Kaminskiy wrote: > FTR, (some) problems that was addressed by this patch was (apparently > independently) rediscovered 3 years later, assigned CVE-2019-38{55...63} > and fixed (differently; I have not checked if fixed code covers all > cases was covered by my patch). > > BTW, _libssh2_check_length() that is extensively used by current code is > broken/incorrect; e.g. suppose > > buf->dataptr = buf->data, buf->len = 5, len = 0xfffffff7 > > then _libssh2_check_length(buf, len) will return 1; uh-doh. > > With obvious security implications. > > (No, I'm not going to compose patch to be ignored for another 3 years).
Ah, yeah, forgot to look at 1.8.x branch. No _check_length there, but other problematic code present instead: uint32_t len = _libssh2_ntohu32(data + 5); ... if((len + 9) < datalen) Broken when len > UINT32_MAX - 9. if(datalen >= 9) { message_len = _libssh2_ntohu32(data + 5); if(message_len < datalen-13) { Broken when datalen >= 9 && datalen < 13 (and there are more similar code). etc. > On 2016-03-27 22:28 , Yuriy M. Kaminskiy wrote: >> Ping? I'd like to stress out this issue has security imlications. At >> very least, DoS (and this is not a standalone application, so it is not >> a minor issue), and maybe host memory exposure too. (However, it is only >> heap over-reads, without heap/stack over-writes, so no risk of >> escalating to remote code execution). >> >> On 02/25/16 03:10 , Yuriy M. Kaminskiy wrote: >>> "George Garner (online)" <ggarner_onl...@gmgsystemsinc.com> writes: >>> [...] >>>> 3. Where is the p_len/group_order parameter validated? In >>>> kex_method_diffie_hellman_group_exchange_sha256_key_exchange it is >>>> converted from network byte order and accepted at face value. What >>>> happens if a malicious packet is received with a bogus value for >>>> p_len? >>> >>> Maybe I miss something, but it looks like this defect (blindly trust >>> various 32-bit length that was sent remote side and don't verify if it >>> fits buffer) is *everywhere* in libssh2. I've sent some patches for >>> kex.c via gh pull request, but quickly discovered it is much worse. Very >>> WIP (and incomplete) patch for *other* files is attached; unfortunately, >>> in most cases, I have no idea how such errors should be handled within >>> libssh2, >>> don't know libssh2 code base well enough, so I give up at this. >>> >>> Note that in early connection setup "malicious server" is not required, >>> "malicious MITM" can insert broken packets as well. >>> >>> In general, please re-review all `grep ntoh -r src/`, in many cases >>> surrounding code looks problematic in one way or other. >>> >>> >>> --- >>> Changelog: >>> v2: fixed obvious errors >>> Note: This is still NOT COMPLETE work, all XXX comment must be reviewed and >>> acted upon. >>> >>> src/agent.c | 32 ++++++++-------- >>> src/channel.c | 10 ++++- >>> src/hostkey.c | 19 +++++++-- >>> src/kex.c | 43 +++++++++++---------- >>> src/packet.c | 45 +++++++++++++++++----- >>> src/publickey.c | 117 >>> +++++++++++++++++++++++++++++++++++++++++++------------- >>> src/session.c | 2 + >>> src/sftp.c | 42 ++++++++++++++++---- >>> src/sftp.h | 1 + >>> src/userauth.c | 32 ++++++++++++++++ >>> 10 files changed, 260 insertions(+), 83 deletions(-) >>> >>> diff --git a/src/agent.c b/src/agent.c >>> index c2ba422..255b63d 100644 >>> --- a/src/agent.c >>> +++ b/src/agent.c >>> @@ -449,12 +449,12 @@ agent_sign(LIBSSH2_SESSION *session, unsigned char >>> **sig, size_t *sig_len, >>> goto error; >>> } >>> method_len = _libssh2_ntohu32(s); >>> - s += 4; >>> - len -= method_len; >>> - if (len < 0) { >>> + if (method_len < 0 || len < method_len) { >>> rc = LIBSSH2_ERROR_AGENT_PROTOCOL; >>> goto error; >>> } >>> + s += 4; >>> + len -= method_len; >>> s += method_len; >>> >>> /* Read the signature */ >>> @@ -464,12 +464,12 @@ agent_sign(LIBSSH2_SESSION *session, unsigned char >>> **sig, size_t *sig_len, >>> goto error; >>> } >>> *sig_len = _libssh2_ntohu32(s); >>> - s += 4; >>> - len -= *sig_len; >>> - if (len < 0) { >>> + if ((size_t)len < *sig_len) { >>> rc = LIBSSH2_ERROR_AGENT_PROTOCOL; >>> goto error; >>> } >>> + len -= *sig_len; >>> + s += 4; >>> >>> *sig = LIBSSH2_ALLOC(session, *sig_len); >>> if (!*sig) { >>> @@ -558,15 +558,15 @@ agent_list_identities(LIBSSH2_AGENT *agent) >>> goto error; >>> } >>> identity->external.blob_len = _libssh2_ntohu32(s); >>> - s += 4; >>> - >>> - /* Read the blob */ >>> - len -= identity->external.blob_len; >>> - if (len < 0) { >>> + if ((size_t)len < identity->external.blob_len) { >>> rc = LIBSSH2_ERROR_AGENT_PROTOCOL; >>> LIBSSH2_FREE(agent->session, identity); >>> goto error; >>> } >>> + s += 4; >>> + >>> + /* Read the blob */ >>> + len -= identity->external.blob_len; >>> >>> identity->external.blob = LIBSSH2_ALLOC(agent->session, >>> >>> identity->external.blob_len); >>> @@ -587,16 +587,16 @@ agent_list_identities(LIBSSH2_AGENT *agent) >>> goto error; >>> } >>> comment_len = _libssh2_ntohu32(s); >>> - s += 4; >>> - >>> - /* Read the comment */ >>> - len -= comment_len; >>> - if (len < 0) { >>> + if (comment_len < 0 || len < comment_len) { >>> rc = LIBSSH2_ERROR_AGENT_PROTOCOL; >>> LIBSSH2_FREE(agent->session, identity->external.blob); >>> LIBSSH2_FREE(agent->session, identity); >>> goto error; >>> } >>> + s += 4; >>> + >>> + /* Read the comment */ >>> + len -= comment_len; >>> >>> identity->external.comment = LIBSSH2_ALLOC(agent->session, >>> comment_len + 1); >>> diff --git a/src/channel.c b/src/channel.c >>> index 32d914d..38572be 100644 >>> --- a/src/channel.c >>> +++ b/src/channel.c >>> @@ -225,6 +225,7 @@ _libssh2_channel_open(LIBSSH2_SESSION * session, const >>> char *channel_type, >>> } >>> >>> if (session->open_state == libssh2_NB_state_sent) { >>> + unsigned char *end; >>> rc = _libssh2_packet_requirev(session, reply_codes, >>> &session->open_data, >>> &session->open_data_len, 1, >>> @@ -238,7 +239,11 @@ _libssh2_channel_open(LIBSSH2_SESSION * session, const >>> char *channel_type, >>> goto channel_error; >>> } >>> >>> + end = session->open_data + session->open_data_len; >>> + >>> if (session->open_data[0] == SSH_MSG_CHANNEL_OPEN_CONFIRMATION) { >>> + if (13+4 > (end - session->open_data)) >>> + goto channel_error; >>> session->open_channel->remote.id = >>> _libssh2_ntohu32(session->open_data + 5); >>> session->open_channel->local.window_size = >>> @@ -265,7 +270,8 @@ _libssh2_channel_open(LIBSSH2_SESSION * session, const >>> char *channel_type, >>> return session->open_channel; >>> } >>> >>> - if (session->open_data[0] == SSH_MSG_CHANNEL_OPEN_FAILURE) { >>> + if (session->open_data[0] == SSH_MSG_CHANNEL_OPEN_FAILURE && >>> + 4 <= (end - (session->open_data + 5))) { >>> unsigned int reason_code = _libssh2_ntohu32(session->open_data >>> + 5); >>> switch (reason_code) { >>> case SSH_OPEN_ADMINISTRATIVELY_PROHIBITED: >>> @@ -1399,6 +1405,7 @@ _libssh2_channel_flush(LIBSSH2_CHANNEL *channel, int >>> streamid) >>> >>> if (((packet_type == SSH_MSG_CHANNEL_DATA) >>> || (packet_type == SSH_MSG_CHANNEL_EXTENDED_DATA)) >>> + && packet->data_len >= 5 + (packet_type == >>> SSH_MSG_CHANNEL_EXTENDED_DATA ? 4 : 0) >>> && (_libssh2_ntohu32(packet->data + 1) == >>> channel->local.id)) { >>> /* It's our channel at least */ >>> long packet_stream_id = >>> @@ -1418,6 +1425,7 @@ _libssh2_channel_flush(LIBSSH2_CHANNEL *channel, int >>> streamid) >>> bytes_to_flush, packet_stream_id, >>> channel->local.id, channel->remote.id); >>> >>> + /* XXX assert(packet->data_len >= 13); XXX */ >>> /* It's one of the streams we wanted to flush */ >>> channel->flush_refund_bytes += packet->data_len - 13; >>> channel->flush_flush_bytes += bytes_to_flush; >>> diff --git a/src/hostkey.c b/src/hostkey.c >>> index 2a0a8f9..7b780e2 100644 >>> --- a/src/hostkey.c >>> +++ b/src/hostkey.c >>> @@ -66,31 +66,42 @@ hostkey_method_ssh_rsa_init(LIBSSH2_SESSION * session, >>> libssh2_rsa_ctx *rsactx; >>> const unsigned char *s, *e, *n; >>> unsigned long len, e_len, n_len; >>> + const unsigned char *end = hostkey_data + hostkey_data_len; >>> int ret; >>> >>> - (void) hostkey_data_len; >>> - >>> if (*abstract) { >>> hostkey_method_ssh_rsa_dtor(session, abstract); >>> *abstract = NULL; >>> } >>> >>> s = hostkey_data; >>> + if (4 > end - s) >>> + return -1; >>> len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (len > (size_t)(end - s)) >>> + return -1; >>> >>> if (len != 7 || strncmp((char *) s, "ssh-rsa", 7) != 0) { >>> return -1; >>> } >>> - s += 7; >>> + s += len; >>> >>> + if (4 > end - s) >>> + return -1; >>> e_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (e_len > (size_t)(end - s)) >>> + return -1; >>> >>> e = s; >>> s += e_len; >>> + if (4 > end - s) >>> + return -1; >>> n_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (n_len > (size_t)(end - s)) >>> + return -1; >>> n = s; >>> >>> ret = _libssh2_rsa_new(&rsactx, e, e_len, n, n_len, NULL, 0, >>> @@ -181,6 +192,8 @@ hostkey_method_ssh_rsa_sig_verify(LIBSSH2_SESSION * >>> session, >>> (void) session; >>> >>> /* Skip past keyname_len(4) + keyname(7){"ssh-rsa"} + signature_len(4) >>> */ >>> + if (15 > sig_len) >>> + return -1; >>> sig += 15; >>> sig_len -= 15; >>> return _libssh2_rsa_sha1_verify(rsactx, sig, sig_len, m, m_len); >>> diff --git a/src/kex.c b/src/kex.c >>> index 40dbeab..2381d52 100644 >>> --- a/src/kex.c >>> +++ b/src/kex.c >>> @@ -2463,21 +2463,20 @@ static int kex_agree_comp(LIBSSH2_SESSION *session, >>> * within the given packet. >>> */ >>> static int kex_string_pair(unsigned char **sp, /* parsing position */ >>> - unsigned char *data, /* start pointer to >>> packet */ >>> - size_t data_len, /* size of total packet */ >>> + unsigned char *end, /* end of packet */ >>> size_t *lenp, /* length of the string */ >>> unsigned char **strp) /* pointer to string >>> start */ >>> { >>> unsigned char *s = *sp; >>> - *lenp = _libssh2_ntohu32(s); >>> >>> - /* the length of the string must fit within the current pointer and the >>> - end of the packet */ >>> - if (*lenp > (data_len - (s - data) -4)) >>> + if (4 > end - s) >>> return 1; >>> - *strp = s + 4; >>> - s += 4 + *lenp; >>> - >>> + *lenp = _libssh2_ntohu32(s); >>> + s += 4; >>> + if (*lenp > (size_t)(end - s)) >>> + return 1; >>> + *strp = s; >>> + s += *lenp; >>> *sp = s; >>> return 0; >>> } >>> @@ -2493,6 +2492,10 @@ static int kex_agree_methods(LIBSSH2_SESSION * >>> session, unsigned char *data, >>> size_t kex_len, hostkey_len, crypt_cs_len, crypt_sc_len, comp_cs_len; >>> size_t comp_sc_len, mac_cs_len, mac_sc_len; >>> unsigned char *s = data; >>> + unsigned char *end = data + data_len; >>> + >>> + if (1 + 16 > end - s) >>> + return -1; >>> >>> /* Skip packet_type, we know it already */ >>> s++; >>> @@ -2501,21 +2504,24 @@ static int kex_agree_methods(LIBSSH2_SESSION * >>> session, unsigned char *data, >>> s += 16; >>> >>> /* Locate each string */ >>> - if(kex_string_pair(&s, data, data_len, &kex_len, &kex)) >>> + if(kex_string_pair(&s, end, &kex_len, &kex)) >>> + return -1; >>> + if(kex_string_pair(&s, end, &hostkey_len, &hostkey)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &hostkey_len, &hostkey)) >>> + if(kex_string_pair(&s, end, &crypt_cs_len, &crypt_cs)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &crypt_cs_len, &crypt_cs)) >>> + if(kex_string_pair(&s, end, &crypt_sc_len, &crypt_sc)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &crypt_sc_len, &crypt_sc)) >>> + if(kex_string_pair(&s, end, &mac_cs_len, &mac_cs)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &mac_cs_len, &mac_cs)) >>> + if(kex_string_pair(&s, end, &mac_sc_len, &mac_sc)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &mac_sc_len, &mac_sc)) >>> + if(kex_string_pair(&s, end, &comp_cs_len, &comp_cs)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &comp_cs_len, &comp_cs)) >>> + if(kex_string_pair(&s, end, &comp_sc_len, &comp_sc)) >>> return -1; >>> - if(kex_string_pair(&s, data, data_len, &comp_sc_len, &comp_sc)) >>> + >>> + if (1 > end - s) >>> return -1; >>> >>> /* If the server sent an optimistic packet, assume that it guessed >>> wrong. >>> @@ -2524,9 +2530,6 @@ static int kex_agree_methods(LIBSSH2_SESSION * >>> session, unsigned char *data, >>> session->burn_optimistic_kexinit = *(s++); >>> /* Next uint32 in packet is all zeros (reserved) */ >>> >>> - if (data_len < (unsigned) (s - data)) >>> - return -1; /* short packet */ >>> - >>> if (kex_agree_kex_hostkey(session, kex, kex_len, hostkey, >>> hostkey_len)) { >>> return -1; >>> } >>> diff --git a/src/packet.c b/src/packet.c >>> index 5f1feb8..3659daa 100644 >>> --- a/src/packet.c >>> +++ b/src/packet.c >>> @@ -85,10 +85,12 @@ packet_queue_listener(LIBSSH2_SESSION * session, >>> unsigned char *data, >>> char failure_code = SSH_OPEN_ADMINISTRATIVELY_PROHIBITED; >>> int rc; >>> >>> - (void) datalen; >>> - >>> if (listen_state->state == libssh2_NB_state_idle) { >>> unsigned char *s = data + (sizeof("forwarded-tcpip") - 1) + 5; >>> + unsigned char *end = data + datalen; >>> + if (4*4 > (end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> listen_state->sender_channel = _libssh2_ntohu32(s); >>> s += 4; >>> >>> @@ -99,15 +101,27 @@ packet_queue_listener(LIBSSH2_SESSION * session, >>> unsigned char *data, >>> >>> listen_state->host_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (listen_state->host_len > (size_t)(end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> listen_state->host = s; >>> s += listen_state->host_len; >>> + if (4*2 > (end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> listen_state->port = _libssh2_ntohu32(s); >>> s += 4; >>> >>> listen_state->shost_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (listen_state->shost_len > (size_t)(end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> listen_state->shost = s; >>> s += listen_state->shost_len; >>> + if (4 > (end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> listen_state->sport = _libssh2_ntohu32(s); >>> >>> _libssh2_debug(session, LIBSSH2_TRACE_CONN, >>> @@ -271,10 +285,12 @@ packet_x11_open(LIBSSH2_SESSION * session, unsigned >>> char *data, >>> LIBSSH2_CHANNEL *channel = x11open_state->channel; >>> int rc; >>> >>> - (void) datalen; >>> - >>> if (x11open_state->state == libssh2_NB_state_idle) { >>> unsigned char *s = data + (sizeof("x11") - 1) + 5; >>> + unsigned char *end = data + datalen; >>> + if (4*4 > (end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> x11open_state->sender_channel = _libssh2_ntohu32(s); >>> s += 4; >>> x11open_state->initial_window_size = _libssh2_ntohu32(s); >>> @@ -283,8 +299,14 @@ packet_x11_open(LIBSSH2_SESSION * session, unsigned >>> char *data, >>> s += 4; >>> x11open_state->shost_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (x11open_state->shost_len > (size_t)(end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> x11open_state->shost = s; >>> s += x11open_state->shost_len; >>> + if (4 > (end - s)) { >>> + return 0; /* XXX ??? XXX */ >>> + } >>> x11open_state->sport = _libssh2_ntohu32(s); >>> >>> _libssh2_debug(session, LIBSSH2_TRACE_CONN, >>> @@ -807,22 +829,26 @@ _libssh2_packet_add(LIBSSH2_SESSION * session, >>> unsigned char *data, >>> else if (len == sizeof("exit-signal") - 1 >>> && !memcmp("exit-signal", data + 9, >>> sizeof("exit-signal") - 1)) { >>> + unsigned char *end = data + datalen; >>> + unsigned char *s = data + 9 + sizeof("exit-signal"); >>> /* command terminated due to signal */ >>> if(datalen >= 20) >>> channelp = _libssh2_channel_locate(session, >>> channel); >>> >>> - if (channelp) { >>> + if (channelp && end - s >= 4) { >>> /* set signal name (without SIG prefix) */ >>> - uint32_t namelen = >>> - _libssh2_ntohu32(data + 9 + >>> sizeof("exit-signal")); >>> + uint32_t namelen = _libssh2_ntohu32(s); >>> + s += 4; >>> + if (namelen > (size_t)(end - s)) >>> + /* XXX ??? XXX */; >>> + else { >>> channelp->exit_signal = >>> LIBSSH2_ALLOC(session, namelen + 1); >>> if (!channelp->exit_signal) >>> rc = _libssh2_error(session, >>> LIBSSH2_ERROR_ALLOC, >>> "memory for signal name"); >>> else { >>> - memcpy(channelp->exit_signal, >>> - data + 13 + sizeof("exit_signal"), >>> namelen); >>> + memcpy(channelp->exit_signal, s, namelen); >>> channelp->exit_signal[namelen] = '\0'; >>> /* TODO: save error message and language tag */ >>> _libssh2_debug(session, LIBSSH2_TRACE_CONN, >>> @@ -832,6 +858,7 @@ _libssh2_packet_add(LIBSSH2_SESSION * session, unsigned >>> char *data, >>> channelp->local.id, >>> channelp->remote.id); >>> } >>> + } >>> } >>> } >>> >>> diff --git a/src/publickey.c b/src/publickey.c >>> index bfee0a8..d19efb7 100644 >>> --- a/src/publickey.c >>> +++ b/src/publickey.c >>> @@ -247,6 +247,7 @@ publickey_response_success(LIBSSH2_PUBLICKEY * pkey) >>> switch (response) { >>> case LIBSSH2_PUBLICKEY_RESPONSE_STATUS: >>> /* Error, or processing complete */ >>> + if (data_len >= 4) >>> { >>> unsigned long status = _libssh2_ntohu32(s); >>> >>> @@ -258,6 +259,7 @@ publickey_response_success(LIBSSH2_PUBLICKEY * pkey) >>> publickey_status_error(pkey, session, status); >>> return -1; >>> } >>> + /* fallthru */ >>> default: >>> LIBSSH2_FREE(session, data); >>> if (response < 0) { >>> @@ -403,6 +405,7 @@ static LIBSSH2_PUBLICKEY >>> *publickey_init(LIBSSH2_SESSION *session) >>> if (session->pkeyInit_state == libssh2_NB_state_sent3) { >>> while (1) { >>> unsigned char *s; >>> + unsigned char *end; >>> rc = publickey_packet_receive(session->pkeyInit_pkey, >>> &session->pkeyInit_data, >>> &session->pkeyInit_data_len); >>> @@ -419,6 +422,7 @@ static LIBSSH2_PUBLICKEY >>> *publickey_init(LIBSSH2_SESSION *session) >>> } >>> >>> s = session->pkeyInit_data; >>> + end = session->pkeyInit_data + session->pkeyInit_data_len; >>> if ((response = >>> publickey_response_id(&s, session->pkeyInit_data_len)) < >>> 0) { >>> _libssh2_error(session, LIBSSH2_ERROR_PUBLICKEY_PROTOCOL, >>> @@ -432,19 +436,33 @@ static LIBSSH2_PUBLICKEY >>> *publickey_init(LIBSSH2_SESSION *session) >>> { >>> unsigned long status, descr_len, lang_len; >>> >>> - status = _libssh2_ntohu32(s); >>> - s += 4; >>> - descr_len = _libssh2_ntohu32(s); >>> - s += 4; >>> - /* description starts here */ >>> - s += descr_len; >>> - lang_len = _libssh2_ntohu32(s); >>> - s += 4; >>> - /* lang starts here */ >>> - s += lang_len; >>> - >>> - if (s > >>> - session->pkeyInit_data + session->pkeyInit_data_len) { >>> + if (4*2 > end - s) >>> + s = NULL; >>> + else { >>> + status = _libssh2_ntohu32(s); >>> + s += 4; >>> + descr_len = _libssh2_ntohu32(s); >>> + s += 4; >>> + /* description starts here */ >>> + if (descr_len > (size_t)(end - s)) >>> + s = NULL; >>> + else { >>> + s += descr_len; >>> + if (4 > end - s) >>> + s = NULL; >>> + else { >>> + lang_len = _libssh2_ntohu32(s); >>> + s += 4; >>> + /* lang starts here */ >>> + if (lang_len > (size_t)(end - s)) >>> + s = NULL; >>> + else >>> + s += lang_len; >>> + } >>> + } >>> + } >>> + >>> + if (s == NULL) { >>> _libssh2_error(session, >>> LIBSSH2_ERROR_PUBLICKEY_PROTOCOL, >>> "Malformed publickey subsystem packet"); >>> @@ -810,6 +828,7 @@ libssh2_publickey_list_fetch(LIBSSH2_PUBLICKEY * pkey, >>> unsigned long *num_keys, >>> } >>> >>> while (1) { >>> + unsigned char *end; >>> rc = publickey_packet_receive(pkey, &pkey->listFetch_data, >>> &pkey->listFetch_data_len); >>> if (rc == LIBSSH2_ERROR_EAGAIN) { >>> @@ -822,6 +841,7 @@ libssh2_publickey_list_fetch(LIBSSH2_PUBLICKEY * pkey, >>> unsigned long *num_keys, >>> } >>> >>> pkey->listFetch_s = pkey->listFetch_data; >>> + end = pkey->listFetch_data + pkey->listFetch_data_len; >>> if ((response = >>> publickey_response_id(&pkey->listFetch_s, >>> pkey->listFetch_data_len)) < 0) { >>> @@ -836,19 +856,34 @@ libssh2_publickey_list_fetch(LIBSSH2_PUBLICKEY * >>> pkey, unsigned long *num_keys, >>> { >>> unsigned long status, descr_len, lang_len; >>> >>> - status = _libssh2_ntohu32(pkey->listFetch_s); >>> - pkey->listFetch_s += 4; >>> - descr_len = _libssh2_ntohu32(pkey->listFetch_s); >>> - pkey->listFetch_s += 4; >>> - /* description starts at pkey->listFetch_s */ >>> - pkey->listFetch_s += descr_len; >>> - lang_len = _libssh2_ntohu32(pkey->listFetch_s); >>> - pkey->listFetch_s += 4; >>> - /* lang starts at pkey->listFetch_s */ >>> - pkey->listFetch_s += lang_len; >>> - >>> - if (pkey->listFetch_s > >>> - pkey->listFetch_data + pkey->listFetch_data_len) { >>> + if (4*2 > end - pkey->listFetch_s) >>> + pkey->listFetch_s = NULL; >>> + else { >>> + status = _libssh2_ntohu32(pkey->listFetch_s); >>> + pkey->listFetch_s += 4; >>> + descr_len = _libssh2_ntohu32(pkey->listFetch_s); >>> + pkey->listFetch_s += 4; >>> + if (descr_len > (size_t)(end - pkey->listFetch_s)) >>> + pkey->listFetch_s = NULL; >>> + else { >>> + /* description starts at pkey->listFetch_s */ >>> + pkey->listFetch_s += descr_len; >>> + if (4 > end - pkey->listFetch_s) >>> + pkey->listFetch_s = NULL; >>> + else { >>> + lang_len = _libssh2_ntohu32(pkey->listFetch_s); >>> + pkey->listFetch_s += 4; >>> + if (lang_len > (size_t)(end - pkey->listFetch_s)) >>> + pkey->listFetch_s = NULL; >>> + else { >>> + /* lang starts at pkey->listFetch_s */ >>> + pkey->listFetch_s += lang_len; >>> + } >>> + } >>> + } >>> + } >>> + >>> + if (pkey->listFetch_s == NULL) { >>> _libssh2_error(session, LIBSSH2_ERROR_PUBLICKEY_PROTOCOL, >>> "Malformed publickey subsystem packet"); >>> goto err_exit; >>> @@ -887,8 +922,12 @@ libssh2_publickey_list_fetch(LIBSSH2_PUBLICKEY * pkey, >>> unsigned long *num_keys, >>> if (pkey->version == 1) { >>> unsigned long comment_len; >>> >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> comment_len = _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (comment_len > (size_t)(end - pkey->listFetch_s)) >>> + goto err_exit; >>> if (comment_len) { >>> list[keys].num_attrs = 1; >>> list[keys].attrs = >>> @@ -911,24 +950,42 @@ libssh2_publickey_list_fetch(LIBSSH2_PUBLICKEY * >>> pkey, unsigned long *num_keys, >>> list[keys].num_attrs = 0; >>> list[keys].attrs = NULL; >>> } >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].name_len = _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (list[keys].name_len > (size_t)(end - >>> pkey->listFetch_s)) >>> + goto err_exit; >>> list[keys].name = pkey->listFetch_s; >>> pkey->listFetch_s += list[keys].name_len; >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].blob_len = _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (list[keys].blob_len > (size_t)(end - >>> pkey->listFetch_s)) >>> + goto err_exit; >>> list[keys].blob = pkey->listFetch_s; >>> pkey->listFetch_s += list[keys].blob_len; >>> } else { >>> /* Version == 2 */ >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].name_len = _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (list[keys].name_len > (size_t)(end - >>> pkey->listFetch_s)) >>> + goto err_exit; >>> list[keys].name = pkey->listFetch_s; >>> pkey->listFetch_s += list[keys].name_len; >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].blob_len = _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (list[keys].blob_len > (size_t)(end - >>> pkey->listFetch_s)) >>> + goto err_exit; >>> list[keys].blob = pkey->listFetch_s; >>> pkey->listFetch_s += list[keys].blob_len; >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].num_attrs = _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> if (list[keys].num_attrs) { >>> @@ -943,14 +1000,22 @@ libssh2_publickey_list_fetch(LIBSSH2_PUBLICKEY * >>> pkey, unsigned long *num_keys, >>> goto err_exit; >>> } >>> for(i = 0; i < list[keys].num_attrs; i++) { >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].attrs[i].name_len = >>> _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (list[keys].attrs[i].name_len > (size_t)(end - >>> pkey->listFetch_s)) >>> + goto err_exit; >>> list[keys].attrs[i].name = (char *) >>> pkey->listFetch_s; >>> pkey->listFetch_s += list[keys].attrs[i].name_len; >>> + if (4 > end - pkey->listFetch_s) >>> + goto err_exit; >>> list[keys].attrs[i].value_len = >>> _libssh2_ntohu32(pkey->listFetch_s); >>> pkey->listFetch_s += 4; >>> + if (list[keys].attrs[i].value_len > (size_t)(end - >>> pkey->listFetch_s)) >>> + goto err_exit; >>> list[keys].attrs[i].value = (char *) >>> pkey->listFetch_s; >>> pkey->listFetch_s += list[keys].attrs[i].value_len; >>> >>> diff --git a/src/session.c b/src/session.c >>> index 06e61dd..ba1bad5 100644 >>> --- a/src/session.c >>> +++ b/src/session.c >>> @@ -763,6 +763,7 @@ session_startup(LIBSSH2_SESSION *session, >>> libssh2_socket_t sock) >>> return rc; >>> >>> session->startup_service_length = >>> + (5 > session->startup_data_len) ? 0 : >>> _libssh2_ntohu32(session->startup_data + 1); >>> >>> if ((session->startup_service_length != (sizeof("ssh-userauth") - >>> 1)) >>> @@ -1410,6 +1411,7 @@ libssh2_poll_channel_read(LIBSSH2_CHANNEL *channel, >>> int extended) >>> packet = _libssh2_list_first(&session->packets); >>> >>> while (packet) { >>> + /* XXX assert(packet->data_len >= 5) XXX */ >>> if ( channel->local.id == _libssh2_ntohu32(packet->data + 1)) { >>> if ( extended == 1 && >>> (packet->data[0] == SSH_MSG_CHANNEL_EXTENDED_DATA >>> diff --git a/src/sftp.c b/src/sftp.c >>> index c142713..ad38638 100644 >>> --- a/src/sftp.c >>> +++ b/src/sftp.c >>> @@ -249,6 +249,7 @@ sftp_packet_add(LIBSSH2_SFTP *sftp, unsigned char *data, >>> "Out of sync with the world"); >>> } >>> >>> + /* XXX ??? assert(data_len >= 5); XXX */ >>> request_id = _libssh2_ntohu32(&data[1]); >>> >>> _libssh2_debug(session, LIBSSH2_TRACE_SFTP, "Received packet id %d", >>> @@ -635,21 +636,25 @@ sftp_attr2bin(unsigned char *p, const >>> LIBSSH2_SFTP_ATTRIBUTES * attrs) >>> >>> /* sftp_bin2attr >>> */ >>> -static int >>> -sftp_bin2attr(LIBSSH2_SFTP_ATTRIBUTES * attrs, const unsigned char *p) >>> +static const unsigned char * >>> +sftp_bin2attr(LIBSSH2_SFTP_ATTRIBUTES * attrs, const unsigned char *s, >>> const unsigned char *end) >>> { >>> - const unsigned char *s = p; >>> - >>> memset(attrs, 0, sizeof(LIBSSH2_SFTP_ATTRIBUTES)); >>> + if (4 < end - p) >>> + return NULL; >>> attrs->flags = _libssh2_ntohu32(s); >>> s += 4; >>> >>> if (attrs->flags & LIBSSH2_SFTP_ATTR_SIZE) { >>> + if (8 < end - p) >>> + return NULL; >>> attrs->filesize = _libssh2_ntohu64(s); >>> s += 8; >>> } >>> >>> if (attrs->flags & LIBSSH2_SFTP_ATTR_UIDGID) { >>> + if (4*2 < end - p) >>> + return NULL; >>> attrs->uid = _libssh2_ntohu32(s); >>> s += 4; >>> attrs->gid = _libssh2_ntohu32(s); >>> @@ -657,18 +662,22 @@ sftp_bin2attr(LIBSSH2_SFTP_ATTRIBUTES * attrs, const >>> unsigned char *p) >>> } >>> >>> if (attrs->flags & LIBSSH2_SFTP_ATTR_PERMISSIONS) { >>> + if (4 < end - p) >>> + return NULL; >>> attrs->permissions = _libssh2_ntohu32(s); >>> s += 4; >>> } >>> >>> if (attrs->flags & LIBSSH2_SFTP_ATTR_ACMODTIME) { >>> + if (4*2 < end - p) >>> + return NULL; >>> attrs->atime = _libssh2_ntohu32(s); >>> s += 4; >>> attrs->mtime = _libssh2_ntohu32(s); >>> s += 4; >>> } >>> >>> - return (s - p); >>> + return s; >>> } >>> >>> /* ************ >>> @@ -1698,7 +1707,7 @@ static ssize_t sftp_readdir(LIBSSH2_SFTP_HANDLE >>> *handle, char *buffer, >>> if (attrs) >>> memset(attrs, 0, sizeof(LIBSSH2_SFTP_ATTRIBUTES)); >>> >>> - s += sftp_bin2attr(attrs ? attrs : &attrs_dummy, s); >>> + s = sftp_bin2attr(attrs ? attrs : &attrs_dummy, s, >>> handle->u.dir.names_end); >>> >>> handle->u.dir.next_name = (char *) s; >>> end: >>> @@ -1789,6 +1798,7 @@ static ssize_t sftp_readdir(LIBSSH2_SFTP_HANDLE >>> *handle, char *buffer, >>> >>> handle->u.dir.names_left = num_names; >>> handle->u.dir.names_packet = data; >>> + handle->u.dir.names_end = data + data_len; >>> handle->u.dir.next_name = (char *) data + 9; >>> >>> /* use the name popping mechanism from the start of the function */ >>> @@ -2252,7 +2262,7 @@ static int sftp_fstat(LIBSSH2_SFTP_HANDLE *handle, >>> } >>> } >>> >>> - sftp_bin2attr(attrs, data + 5); >>> + sftp_bin2attr(attrs, data + 5, data + data_len); >>> LIBSSH2_FREE(session, data); >>> >>> return 0; >>> @@ -2559,6 +2569,7 @@ static int sftp_unlink(LIBSSH2_SFTP *sftp, const char >>> *filename, >>> >>> sftp->unlink_state = libssh2_NB_state_idle; >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> retcode = _libssh2_ntohu32(data + 5); >>> LIBSSH2_FREE(session, data); >>> >>> @@ -2669,6 +2680,7 @@ static int sftp_rename(LIBSSH2_SFTP *sftp, const char >>> *source_filename, >>> >>> sftp->rename_state = libssh2_NB_state_idle; >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> retcode = _libssh2_ntohu32(data + 5); >>> LIBSSH2_FREE(session, data); >>> >>> @@ -2793,6 +2805,7 @@ static int sftp_fstatvfs(LIBSSH2_SFTP_HANDLE *handle, >>> LIBSSH2_SFTP_STATVFS *st) >>> "Error waiting for FXP EXTENDED REPLY"); >>> } >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> if (data[0] == SSH_FXP_STATUS) { >>> int retcode = _libssh2_ntohu32(data + 5); >>> sftp->fstatvfs_state = libssh2_NB_state_idle; >>> @@ -2919,6 +2932,7 @@ static int sftp_statvfs(LIBSSH2_SFTP *sftp, const >>> char *path, >>> "Error waiting for FXP EXTENDED REPLY"); >>> } >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> if (data[0] == SSH_FXP_STATUS) { >>> int retcode = _libssh2_ntohu32(data + 5); >>> sftp->statvfs_state = libssh2_NB_state_idle; >>> @@ -3051,6 +3065,7 @@ static int sftp_mkdir(LIBSSH2_SFTP *sftp, const char >>> *path, >>> >>> sftp->mkdir_state = libssh2_NB_state_idle; >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> retcode = _libssh2_ntohu32(data + 5); >>> LIBSSH2_FREE(session, data); >>> >>> @@ -3145,6 +3160,7 @@ static int sftp_rmdir(LIBSSH2_SFTP *sftp, const char >>> *path, >>> >>> sftp->rmdir_state = libssh2_NB_state_idle; >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> retcode = _libssh2_ntohu32(data + 5); >>> LIBSSH2_FREE(session, data); >>> >>> @@ -3188,6 +3204,7 @@ static int sftp_stat(LIBSSH2_SFTP *sftp, const char >>> *path, >>> ((stat_type == >>> LIBSSH2_SFTP_SETSTAT) ? sftp_attrsize(attrs->flags) : 0); >>> unsigned char *s, *data; >>> + unsigned char *data_end; >>> static const unsigned char stat_responses[2] = >>> { SSH_FXP_ATTRS, SSH_FXP_STATUS }; >>> int rc; >>> @@ -3258,6 +3275,8 @@ static int sftp_stat(LIBSSH2_SFTP *sftp, const char >>> *path, >>> >>> sftp->stat_state = libssh2_NB_state_idle; >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> + >>> if (data[0] == SSH_FXP_STATUS) { >>> int retcode; >>> >>> @@ -3273,7 +3292,7 @@ static int sftp_stat(LIBSSH2_SFTP *sftp, const char >>> *path, >>> } >>> >>> memset(attrs, 0, sizeof(LIBSSH2_SFTP_ATTRIBUTES)); >>> - sftp_bin2attr(attrs, data + 5); >>> + sftp_bin2attr(attrs, data + 5, data + data_len); >>> LIBSSH2_FREE(session, data); >>> >>> return 0; >>> @@ -3389,6 +3408,8 @@ static int sftp_symlink(LIBSSH2_SFTP *sftp, const >>> char *path, >>> >>> sftp->symlink_state = libssh2_NB_state_idle; >>> >>> + /* XXX ??? assert(data_len >= 5+4); XXX */ >>> + >>> if (data[0] == SSH_FXP_STATUS) { >>> int retcode; >>> >>> @@ -3410,8 +3431,13 @@ static int sftp_symlink(LIBSSH2_SFTP *sftp, const >>> char *path, >>> "no name entries"); >>> } >>> >>> + /* XXX ??? assert(data_len >= 5+4*2); XXX */ >>> + >>> /* this reads a u32 and stores it into a signed 32bit value */ >>> link_len = _libssh2_ntohu32(data + 9); >>> + >>> + /* XXX ??? assert(data_len-(5+4*2) >= link_len); XXX */ >>> + >>> if (link_len < target_len) { >>> memcpy(target, data + 13, link_len); >>> target[link_len] = 0; >>> diff --git a/src/sftp.h b/src/sftp.h >>> index 2ed32ce..91fc0a7 100644 >>> --- a/src/sftp.h >>> +++ b/src/sftp.h >>> @@ -122,6 +122,7 @@ struct _LIBSSH2_SFTP_HANDLE >>> uint32_t names_left; >>> void *names_packet; >>> char *next_name; >>> + char *names_end; >>> } dir; >>> } u; >>> >>> diff --git a/src/userauth.c b/src/userauth.c >>> index cdfa25e..c799a40 100644 >>> --- a/src/userauth.c >>> +++ b/src/userauth.c >>> @@ -69,6 +69,7 @@ static char *userauth_list(LIBSSH2_SESSION *session, >>> const char *username, >>> service(14)"ssh-connection" + method_len(4) = 27 */ >>> unsigned long methods_len; >>> unsigned char *s; >>> + unsigned char *end; >>> int rc; >>> >>> if (session->userauth_list_state == libssh2_NB_state_idle) { >>> @@ -143,7 +144,18 @@ static char *userauth_list(LIBSSH2_SESSION *session, >>> const char *username, >>> return NULL; >>> } >>> >>> + if (5 > session->userauth_list_data_len) { >>> + /* XXX ??? XXX */ >>> +userauth_packet_overrun: >>> + LIBSSH2_FREE(session, session->userauth_list_data); >>> + session->userauth_list_data = NULL; >>> + session->userauth_list_state = libssh2_NB_state_idle; >>> + return NULL; >>> + } >>> methods_len = _libssh2_ntohu32(session->userauth_list_data + 1); >>> + if (methods_len > session->userauth_list_data_len - 5) { >>> + goto userauth_packet_overrun; >>> + } >>> >>> /* Do note that the memory areas overlap! */ >>> memmove(session->userauth_list_data, session->userauth_list_data + >>> 5, >>> @@ -1561,6 +1573,7 @@ userauth_keyboard_interactive(LIBSSH2_SESSION * >>> session, >>> >>> LIBSSH2_USERAUTH_KBDINT_RESPONSE_FUNC((*response_callback))) >>> { >>> unsigned char *s; >>> + unsigned char *end; >>> int rc; >>> >>> static const unsigned char reply_codes[4] = { >>> @@ -1685,10 +1698,15 @@ userauth_keyboard_interactive(LIBSSH2_SESSION * >>> session, >>> >>> /* server requested PAM-like conversation */ >>> s = session->userauth_kybd_data + 1; >>> + end = session->userauth_kybd_data + >>> session->userauth_kybd_data_len; >>> >>> /* string name (ISO-10646 UTF-8) */ >>> + if (4 > end - s) >>> + goto cleanup; /* XXX ??? XXX */ >>> session->userauth_kybd_auth_name_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (session->userauth_kybd_auth_name_len > (size_t)(end - s)) >>> + goto cleanup; /* XXX ??? XXX */ >>> if(session->userauth_kybd_auth_name_len) { >>> session->userauth_kybd_auth_name = >>> LIBSSH2_ALLOC(session, >>> @@ -1706,8 +1724,12 @@ userauth_keyboard_interactive(LIBSSH2_SESSION * >>> session, >>> } >>> >>> /* string instruction (ISO-10646 UTF-8) */ >>> + if (4 > end - s) >>> + goto cleanup; /* XXX ??? XXX */ >>> session->userauth_kybd_auth_instruction_len = >>> _libssh2_ntohu32(s); >>> s += 4; >>> + if (session->userauth_kybd_auth_instruction_len > (size_t)(end >>> - s)) >>> + goto cleanup; /* XXX ??? XXX */ >>> if(session->userauth_kybd_auth_instruction_len) { >>> session->userauth_kybd_auth_instruction = >>> LIBSSH2_ALLOC(session, >>> @@ -1725,13 +1747,19 @@ userauth_keyboard_interactive(LIBSSH2_SESSION * >>> session, >>> } >>> >>> /* string language tag (as defined in [RFC-3066]) */ >>> + if (4 > end - s) >>> + goto cleanup; /* XXX ??? XXX */ >>> language_tag_len = _libssh2_ntohu32(s); >>> s += 4; >>> + if (language_tag_len > (size_t)(end - s)) >>> + goto cleanup; /* XXX ??? XXX */ >>> >>> /* ignoring this field as deprecated */ >>> s += language_tag_len; >>> >>> /* int num-prompts */ >>> + if (4 > end - s) >>> + goto cleanup; /* XXX ??? XXX */ >>> session->userauth_kybd_num_prompts = _libssh2_ntohu32(s); >>> s += 4; >>> >>> @@ -1760,9 +1788,13 @@ userauth_keyboard_interactive(LIBSSH2_SESSION * >>> session, >>> >>> for(i = 0; i < session->userauth_kybd_num_prompts; i++) { >>> /* string prompt[1] (ISO-10646 UTF-8) */ >>> + if (4 > end - s) >>> + goto cleanup; /* XXX ??? XXX */ >>> session->userauth_kybd_prompts[i].length = >>> _libssh2_ntohu32(s); >>> s += 4; >>> + if (session->userauth_kybd_prompts[i].length > >>> (size_t)(end - s)) >>> + goto cleanup; /* XXX ??? XXX */ >>> session->userauth_kybd_prompts[i].text = >>> LIBSSH2_CALLOC(session, >>> >>> session->userauth_kybd_prompts[i].length); _______________________________________________ libssh2-devel https://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel