Subject: Re: [libssh2] Key Re-Exchange Patch

Re: [libssh2] Key Re-Exchange Patch

From: Peter Stuge <peter_at_stuge.se>
Date: Fri, 27 Jun 2008 14:16:12 +0200

On Thu, Jun 26, 2008 at 05:48:29PM -0400, Sean Peterson wrote:
> I've remade the patches in the 'diff -u' format.

Thanks!

> Index: kex.c
> ===================================================================
> RCS file: /cvsroot/libssh2/libssh2/src/kex.c,v
> retrieving revision 1.36
> diff -u -r1.36 kex.c
> --- kex.c 18 Nov 2007 20:57:13 -0000 1.36
> +++ kex.c 26 Jun 2008 21:32:50 -0000
> @@ -1680,6 +1680,8 @@
> int rc = 0;
> int retcode;
>
> + session->state |= LIBSSH2_STATE_KEX_ACTIVE;
> +
> if (key_state->state == libssh2_NB_state_idle) {
> /* Prevent loop in packet_add() */
> session->state |= LIBSSH2_STATE_EXCHANGING_KEYS;
> @@ -1711,11 +1713,14 @@
> if (key_state->state == libssh2_NB_state_sent) {
> retcode = libssh2_kexinit(session);
> if (retcode == PACKET_EAGAIN) {
> + session->state &= ~LIBSSH2_STATE_KEX_ACTIVE;
> return PACKET_EAGAIN;
> } else if (retcode) {
> session->local.kexinit = key_state->oldlocal;
> session->local.kexinit_len = key_state->oldlocal_len;
> key_state->state = libssh2_NB_state_idle;
> + session->state &= ~LIBSSH2_STATE_KEX_ACTIVE;
> + session->state &= ~LIBSSH2_STATE_EXCHANGING_KEYS;
> return -1;
> }
>
> @@ -1729,6 +1734,7 @@
> &key_state->data_len, 0, NULL, 0,
> &key_state->req_state);
> if (retcode == PACKET_EAGAIN) {
> + session->state &= ~LIBSSH2_STATE_KEX_ACTIVE;
> return PACKET_EAGAIN;
> } else if (retcode) {
> if (session->local.kexinit) {
> @@ -1737,6 +1743,8 @@
> session->local.kexinit = key_state->oldlocal;
> session->local.kexinit_len = key_state->oldlocal_len;
> key_state->state = libssh2_NB_state_idle;
> + session->state &= ~LIBSSH2_STATE_KEX_ACTIVE;
> + session->state &= ~LIBSSH2_STATE_EXCHANGING_KEYS;
> return -1;
> }
>
> @@ -1763,6 +1771,7 @@
> session->kex->exchange_keys(session,
> &key_state->key_state_low);
> if (retcode == PACKET_EAGAIN) {
> + session->state &= ~LIBSSH2_STATE_KEX_ACTIVE;
> return PACKET_EAGAIN;
> } else if (retcode) {
> libssh2_error(session, LIBSSH2_ERROR_KEY_EXCHANGE_FAILURE,
> @@ -1782,6 +1791,7 @@
> session->remote.kexinit = NULL;
> }
>
> + session->state &= ~LIBSSH2_STATE_KEX_ACTIVE;
> session->state &= ~LIBSSH2_STATE_EXCHANGING_KEYS;
>
> key_state->state = libssh2_NB_state_idle;

> Index: libssh2_priv.h
> ===================================================================
> RCS file: /cvsroot/libssh2/libssh2/src/libssh2_priv.h,v
> retrieving revision 1.35
> diff -u -r1.35 libssh2_priv.h
> --- libssh2_priv.h 6 Aug 2007 20:48:06 -0000 1.35
> +++ libssh2_priv.h 26 Jun 2008 21:32:05 -0000
> @@ -833,6 +833,7 @@
> #define LIBSSH2_STATE_EXCHANGING_KEYS 0x00000001
> #define LIBSSH2_STATE_NEWKEYS 0x00000002
> #define LIBSSH2_STATE_AUTHENTICATED 0x00000004
> +#define LIBSSH2_STATE_KEX_ACTIVE 0x00000008
>
> /* session.flag helpers */
> #ifdef MSG_NOSIGNAL

> Index: packet.c
> ===================================================================
> RCS file: /cvsroot/libssh2/libssh2/src/packet.c,v
> retrieving revision 1.55
> diff -u -r1.55 packet.c
> --- packet.c 6 Aug 2007 20:48:07 -0000 1.55
> +++ packet.c 26 Jun 2008 21:33:09 -0000
> @@ -914,11 +914,31 @@
>
> session->packAdd_state = libssh2_NB_state_sent2;
> }
> +
> + /*
> + * The KEXINIT message has been added to the queue.
> + * The packAdd and readPack states need to be reset
> + * because libssh2_kex_exchange (eventually) calls upon
> + * libssh2_packet_read to read the rest of the key exchange
> + * conversation.
> + */
> + session->readPack_state = libssh2_NB_state_idle;
> + session->packet.total_num = 0;
> + session->packAdd_state = libssh2_NB_state_idle;
> + session->fullpacket_state = libssh2_NB_state_idle;
> +
> + /*
> + * Also, don't use packAdd_key_state for key re-exchange,
> + * as it will be wiped out in the middle of the exchange.
> + * How about re-using the startup_key_state?
> + */
> + memset(&session->startup_key_state, 0, sizeof(key_exchange_state_t));
> +
> /*
> * If there was a key reexchange failure, let's just hope we didn't
> * send NEWKEYS yet, otherwise remote will drop us like a rock
> */
> - rc = libssh2_kex_exchange(session, 1, &session->packAdd_key_state);
> + rc = libssh2_kex_exchange(session, 1, &session->startup_key_state);
> if (rc == PACKET_EAGAIN) {
> return PACKET_EAGAIN;
> }

> Index: transport.c
> ===================================================================
> RCS file: /cvsroot/libssh2/libssh2/src/transport.c,v
> retrieving revision 1.13
> diff -u -r1.13 transport.c
> --- transport.c 8 Nov 2007 13:51:23 -0000 1.13
> +++ transport.c 26 Jun 2008 21:34:04 -0000
> @@ -269,6 +269,40 @@
> int blocksize;
> int encrypted = 1;
>
> + int status;
> +
> + /*
> + * All channels, systems, subsystems, etc eventually make it down here
> + * when looking for more incoming data. If a key exchange is going on
> + * (LIBSSH2_STATE_EXCHANGING_KEYS bit is set) then the remote end
> + * will ONLY send key exchange related traffic. In non-blocking mode,
> + * there is a chance to break out of the kex_exchange function with an
> + * EAGAIN status, and never come back to it. If LIBSSH2_STATE_EXCHANGING_KEYS
> + * is active, then we must redirect to the key exchange. However,
> + * if kex_exchange is active (as in it is the one that calls this execution
> + * of packet_read, then don't redirect, as that would be an infinite loop!
> + */
> +
> + if (session->state & LIBSSH2_STATE_EXCHANGING_KEYS &&
> + !(session->state & LIBSSH2_STATE_KEX_ACTIVE)) {
> +
> + /* Whoever wants a packet won't get anything until the key re-exchange
> + * is done!
> + */
> + _libssh2_debug(session, LIBSSH2_DBG_TRANS, "Redirecting into the"
> + " key re-exchange");
> + status = libssh2_kex_exchange(session, 1, &session->startup_key_state);
> + if (status == PACKET_EAGAIN) {
> + libssh2_error(session, LIBSSH2_ERROR_EAGAIN,
> + "Would block exchanging encryption keys", 0);
> + return PACKET_EAGAIN;
> + } else if (status) {
> + libssh2_error(session, LIBSSH2_ERROR_KEX_FAILURE,
> + "Unable to exchange encryption keys",0);
> + return LIBSSH2_ERROR_KEX_FAILURE;
> + }
> + }
> +
> /*
> * =============================== NOTE ===============================
> * I know this is very ugly and not a really good use of "goto", but
> @@ -527,8 +561,21 @@
> libssh2_packet_read_point1:
> rc = fullpacket(session, encrypted);
> if (rc == PACKET_EAGAIN) {
> - session->readPack_encrypted = encrypted;
> - session->readPack_state = libssh2_NB_state_jump1;
> +
> + if (session->packAdd_state != libssh2_NB_state_idle)
> + {
> + /* fullpacket only returns PACKET_EAGAIN if
> + * libssh2_packet_add returns PACKET_EAGAIN. If that
> + * returns PACKET_EAGAIN but the packAdd_state is idle,
> + * then the packet has been added to the brigade, but some
> + * immediate action that was taken based on the packet
> + * type (such as key re-exchange) is not yet complete.
> + * Clear the way for a new packet to be read in.
> + */
> + session->readPack_encrypted = encrypted;
> + session->readPack_state = libssh2_NB_state_jump1;
> + }
> +
> return PACKET_EAGAIN;
> }
>

Can I ack or vote for this? What is the process? Please commit it.

//Peter

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
libssh2-devel mailing list
libssh2-devel_at_lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libssh2-devel
Received on 2008-06-27