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