From 8428b7703c2b5d510aed8f57f146d2717b5529bc Mon Sep 17 00:00:00 2001 From: Uros Majstorovic Date: Thu, 25 Apr 2024 19:48:06 +0200 Subject: fixed possible timer timeout/open reply message racing conditions; removed conn reset and reopen as unsafe --- ecp/src/ecp/core.c | 190 ++++++++++++++++++++++++++++++---------------------- ecp/src/ecp/core.h | 5 +- ecp/src/ecp/timer.c | 12 +++- 3 files changed, 119 insertions(+), 88 deletions(-) (limited to 'ecp/src') diff --git a/ecp/src/ecp/core.c b/ecp/src/ecp/core.c index bb3309c..d111a97 100644 --- a/ecp/src/ecp/core.c +++ b/ecp/src/ecp/core.c @@ -928,31 +928,11 @@ int ecp_conn_init_outb(ECPConnection *conn) { ECPDHKey key; int rv; - conn->key_curr = 0; - rv = ecp_dhkey_gen(&key); if (rv) return rv; - rv = conn_dhkey_new(conn, conn->key_curr, &key); - if (rv) return rv; - - return ECP_OK; -} - -int ecp_conn_reset(ECPConnection *conn) { - if (conn->flags) return ECP_ERR; - - conn->key_curr = ECP_ECDH_IDX_INV; - conn->key_next = ECP_ECDH_IDX_INV; - conn->rkey_curr = ECP_ECDH_IDX_INV; - memset(&conn->key, 0, sizeof(conn->key)); - memset(&conn->rkey, 0, sizeof(conn->rkey)); - memset(&conn->shkey, 0, sizeof(conn->shkey)); - arc4random_buf(&conn->nonce_out, sizeof(conn->nonce_out)); - conn->access_ts = 0; - conn->keyx_ts = 0; - conn->nonce_in = 0; - conn->nonce_map = 0; + conn->key_curr = 0; + conn->key[conn->key_curr] = key; return ECP_OK; } @@ -1066,30 +1046,6 @@ int ecp_conn_create_outb(ECPConnection *conn, ECPConnection *parent, ECPNode *no return ECP_OK; } -int ecp_conn_reset_outb(ECPConnection *conn) { - int rv = ECP_OK; - - ecp_conn_remove(conn); - -#ifdef ECP_WITH_PTHREAD - pthread_mutex_lock(&conn->mutex); -#endif - - if (_ecp_conn_is_open(conn)) rv = ECP_ERR; - if (!rv) rv = ecp_conn_reset(conn); - if (!rv) rv = ecp_conn_init_outb(conn); - -#ifdef ECP_WITH_PTHREAD - pthread_mutex_unlock(&conn->mutex); -#endif - if (rv) return rv; - - rv = ecp_conn_insert(conn); - if (rv) return rv; - - return ECP_OK; -} - void ecp_conn_destroy(ECPConnection *conn) { #ifdef ECP_WITH_VCONN if (conn->parent) { @@ -1171,6 +1127,30 @@ int ecp_conn_insert_gc(ECPConnection *conn) { return rv; } +int _ecp_conn_remove(ECPConnection *conn) { + ECPSocket *sock = conn->sock; + int rv = ECP_OK; + +#ifdef ECP_WITH_PTHREAD + pthread_mutex_lock(&sock->conn_table.mutex); + pthread_mutex_lock(&conn->mutex); +#endif + + if (!_ecp_conn_is_reg(conn)) rv = ECP_ERR_CLOSED; + if (!rv && _ecp_conn_is_open(conn)) rv = ECP_ERR_BUSY; + if (!rv) { + conn_table_remove(conn); + _ecp_conn_clr_reg(conn); + } + +#ifdef ECP_WITH_PTHREAD + pthread_mutex_unlock(&conn->mutex); + pthread_mutex_unlock(&sock->conn_table.mutex); +#endif + + return rv; +} + void ecp_conn_remove(ECPConnection *conn) { ECPSocket *sock = conn->sock; @@ -1268,15 +1248,6 @@ int _ecp_conn_open(ECPConnection *conn, ECPConnection *parent, ECPNode *node, in return rv; } -int _ecp_conn_reopen(ECPConnection *conn, int retry) { - int rv; - - rv = ecp_conn_reset_outb(conn); - if (rv) return rv; - - return ecp_send_init_req(conn, retry); -} - int ecp_conn_open(ECPConnection *conn, ECPNode *node) { int rv; @@ -1291,13 +1262,6 @@ int ecp_conn_try_open(ECPConnection *conn, ECPNode *node) { return rv; } -int ecp_conn_reopen(ECPConnection *conn) { - int rv; - - rv = _ecp_conn_reopen(conn, 1); - return rv; -} - void _ecp_conn_close(ECPConnection *conn) { if (_ecp_conn_is_open(conn)) { ecp_close_handler_t handler; @@ -1618,6 +1582,7 @@ void ecp_err_handle(ECPConnection *conn, unsigned char mtype, int err) { rv = ecp_ext_err_handle(conn, mtype, err); if (rv != ECP_PASS) return; + if (err == ECP_ERR_CLOSED) return; if (ctx->handle_err) ctx->handle_err(conn, mtype, err); } @@ -1643,12 +1608,52 @@ static ssize_t _send_ireq(ECPConnection *conn, ECPTimerItem *ti) { } static ssize_t _retry_ireq(ECPConnection *conn, ECPTimerItem *ti) { + ECPSocket *sock = conn->sock; + ECPDHKey key; + int is_open, refcount_ok; int rv; - rv = ecp_conn_reset_outb(conn); + rv = ecp_dhkey_gen(&key); + if (rv) return rv; + +#ifdef ECP_WITH_PTHREAD + pthread_mutex_lock(&sock->conn_table.mutex); + pthread_mutex_lock(&conn->mutex); +#endif + + rv = ECP_OK; + refcount_ok = 0; + + is_open = _ecp_conn_is_open(conn); + if (!is_open) { + /* caller (timer) holds one reference */ + refcount_ok = 1; + if (!ecp_conn_is_gc(conn)) refcount_ok++; + + refcount_ok = (conn->refcount == refcount_ok); + if (refcount_ok) { + rv = conn_dhkey_new(conn, conn->key_curr, &key); + } + } + +#ifdef ECP_WITH_PTHREAD + pthread_mutex_unlock(&conn->mutex); + pthread_mutex_unlock(&sock->conn_table.mutex); +#endif if (rv) return rv; - return _send_ireq(conn, ti); + if (is_open) { + /* already opened, that's OK */ + return 0; + } + + if (refcount_ok) { + return _send_ireq(conn, ti); + } else { + /* check later */ + rv = ecp_timer_push(ti); + return rv; + } } ssize_t ecp_send_init_req(ECPConnection *conn, int retry) { @@ -1911,7 +1916,7 @@ ssize_t ecp_send_open_rep(ECPConnection *conn) { ssize_t ecp_handle_open(ECPConnection *conn, unsigned char mtype, unsigned char *msg, size_t msg_size, ECP2Buffer *bufs) { ecp_open_handler_t handler; - int is_open, is_gc; + int is_reg, is_open, is_gc; int _rv = ECP_OK; ssize_t rv; @@ -1919,37 +1924,55 @@ ssize_t ecp_handle_open(ECPConnection *conn, unsigned char mtype, unsigned char pthread_mutex_lock(&conn->mutex); #endif + is_reg = _ecp_conn_is_reg(conn); is_open = _ecp_conn_is_open(conn); + if (!is_reg) _rv = ECP_ERR_CLOSED; + if (!_rv && is_open) _rv = ECP_ERR; + + /* set open if registered, avoids race condition with timeout handler */ + if (!_rv) _ecp_conn_set_open(conn); #ifdef ECP_WITH_PTHREAD pthread_mutex_unlock(&conn->mutex); #endif - if (is_open) return ECP_ERR; + if (_rv) return _rv; if (mtype == ECP_MTYPE_OPEN_REQ) { - if (ecp_conn_is_outb(conn)) return ECP_ERR; + if (ecp_conn_is_outb(conn)) { + _rv = ECP_ERR; + goto handle_open_fin; + } rv = 2; - if (msg_size < rv) return ECP_ERR_SIZE; + if (msg_size < rv) { + _rv = ECP_ERR_SIZE; + goto handle_open_fin; + } if (msg[1]) rv += ECP_SIZE_VBOX; - if (msg_size < rv) return ECP_ERR_SIZE; + if (msg_size < rv) { + _rv = ECP_ERR_SIZE; + goto handle_open_fin; + } } else { - if (ecp_conn_is_inb(conn)) return ECP_ERR; + if (ecp_conn_is_inb(conn)) { + _rv = ECP_ERR; + goto handle_open_fin; + } rv = 0; } is_gc = ecp_conn_is_gc(conn); if (is_gc) { _rv = ecp_conn_insert_gc(conn); - if (_rv) return _rv; + if (_rv) goto handle_open_fin; } _rv = ecp_ext_conn_open(conn); if (_rv) { if (is_gc) ecp_conn_remove_gc(conn); - return _rv; + goto handle_open_fin; } handler = ecp_get_open_handler(conn); @@ -1958,25 +1981,30 @@ ssize_t ecp_handle_open(ECPConnection *conn, unsigned char mtype, unsigned char if (_rv) { if (is_gc) ecp_conn_remove_gc(conn); ecp_ext_conn_close(conn); - return _rv; + goto handle_open_fin; } } + if (ecp_conn_is_inb(conn)) { + ecp_tr_release(bufs->packet, 1); + ecp_send_open_rep(conn); + } else if (ecp_conn_is_root(conn)) { + ecp_conn_remove_addr(conn); + } + +handle_open_fin: + if (_rv) { #ifdef ECP_WITH_PTHREAD - pthread_mutex_lock(&conn->mutex); + pthread_mutex_lock(&conn->mutex); #endif - _ecp_conn_set_open(conn); + _ecp_conn_clr_open(conn); #ifdef ECP_WITH_PTHREAD - pthread_mutex_unlock(&conn->mutex); + pthread_mutex_unlock(&conn->mutex); #endif - if (ecp_conn_is_inb(conn)) { - ecp_tr_release(bufs->packet, 1); - ecp_send_open_rep(conn); - } else if (ecp_conn_is_root(conn)) { - ecp_conn_remove_addr(conn); + return _rv; } return rv; diff --git a/ecp/src/ecp/core.h b/ecp/src/ecp/core.h index 30c00b3..6f1dbab 100644 --- a/ecp/src/ecp/core.h +++ b/ecp/src/ecp/core.h @@ -404,7 +404,6 @@ int ecp_cookie_verify(ECPSocket *sock, unsigned char *cookie, unsigned char *pub ECPConnection *ecp_conn_new_inb(ECPSocket *sock, unsigned char ctype); void ecp_conn_init(ECPConnection *conn, ECPSocket *sock, unsigned char ctype); int ecp_conn_init_outb(ECPConnection *conn); -int ecp_conn_reset(ECPConnection *conn); void ecp_conn_set_flags(ECPConnection *conn, unsigned char flags); void ecp_conn_clr_flags(ECPConnection *conn, unsigned char flags); void ecp_conn_set_remote_key(ECPConnection *conn, ECPDHPub *key); @@ -412,21 +411,19 @@ void ecp_conn_set_remote_addr(ECPConnection *conn, ecp_tr_addr_t *addr); int ecp_conn_create(ECPConnection *conn, ECPConnection *parent); int ecp_conn_create_inb(ECPConnection *conn, ECPConnection *parent, unsigned char s_idx, unsigned char c_idx, ecp_ecdh_public_t *public, ECPDHPub *rkey_perma, ecp_aead_key_t *shkey); int ecp_conn_create_outb(ECPConnection *conn, ECPConnection *parent, ECPNode *node); -int ecp_conn_reset_outb(ECPConnection *conn); void ecp_conn_destroy(ECPConnection *conn); void ecp_conn_free(ECPConnection *conn); int ecp_conn_insert(ECPConnection *conn); int ecp_conn_insert_gc(ECPConnection *conn); +int _ecp_conn_remove(ECPConnection *conn); void ecp_conn_remove(ECPConnection *conn); void ecp_conn_remove_addr(ECPConnection *conn); void ecp_conn_remove_gc(ECPConnection *conn); int _ecp_conn_open(ECPConnection *conn, ECPConnection *parent, ECPNode *node, int retry); -int _ecp_conn_reopen(ECPConnection *conn, int retry); int ecp_conn_open(ECPConnection *conn, ECPNode *node); int ecp_conn_try_open(ECPConnection *conn, ECPNode *node); -int ecp_conn_reopen(ECPConnection *conn); void _ecp_conn_close(ECPConnection *conn); void ecp_conn_close(ECPConnection *conn); diff --git a/ecp/src/ecp/timer.c b/ecp/src/ecp/timer.c index 6b58303..00478ed 100644 --- a/ecp/src/ecp/timer.c +++ b/ecp/src/ecp/timer.c @@ -181,14 +181,20 @@ ecp_sts_t ecp_timer_exe(ECPSocket *sock) { _rv = retry(conn, to_exec+i); if (_rv < 0) rv = _rv; } - if (rv && (rv != ECP_ERR_CLOSED)) { + if (rv) { ecp_err_handle(conn, mtype, rv); } } else { rv = ECP_ERR_TIMEOUT; - ecp_err_handle(conn, mtype, rv); if (mtype == ECP_MTYPE_OPEN_REP) { - ecp_conn_remove(conn); + int _rv; + + /* remove only if connection is not already open, avoids race condition with open reply message handler */ + _rv = _ecp_conn_remove(conn); + if (_rv) rv = ECP_OK; + } + if (rv) { + ecp_err_handle(conn, mtype, rv); } } -- cgit v1.2.3