From 0b47e8f9cc4aed0aad3579c89617bd55f7b270b4 Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:15:08 +0100 Subject: [PATCH 01/18] io: handle potential read/write errors --- src/io.c | 91 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/src/io.c b/src/io.c index 29908b1e..26b00fdc 100644 --- a/src/io.c +++ b/src/io.c @@ -64,7 +64,7 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti { uint32_t readd = 0; ssize_t r = -1; - int fd, interrupted; + int fd, interrupted, res; struct timespec ts_inact_timeout; assert(session); @@ -109,22 +109,28 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti session->status = NC_STATUS_INVALID; session->term_reason = NC_SESSION_TERM_DROPPED; return -1; + } else if (r > (count - readd)) { + ERR(session, "Invalid number of bytes read (%ld > %u)", r, (count - readd)); + session->status = NC_STATUS_INVALID; + session->term_reason = NC_SESSION_TERM_DROPPED; + return -1; } break; #ifdef NC_ENABLED_SSH_TLS case NC_TI_SSH: /* read via libssh */ - r = ssh_channel_read(session->ti.libssh.channel, buf + readd, count - readd, 0); - if (r == SSH_AGAIN) { + res = ssh_channel_read(session->ti.libssh.channel, buf + readd, count - readd, 0); + if (res == SSH_AGAIN) { r = 0; break; - } else if (r == SSH_ERROR) { + } else if (res == SSH_ERROR) { ERR(session, "Reading from the SSH channel failed (%s).", ssh_get_error(session->ti.libssh.session)); session->status = NC_STATUS_INVALID; session->term_reason = NC_SESSION_TERM_OTHER; return -1; - } else if (r == 0) { + } else if (res == 0) { + r = 0; if (ssh_channel_is_eof(session->ti.libssh.channel)) { ERR(session, "SSH channel unexpected EOF."); session->status = NC_STATUS_INVALID; @@ -132,14 +138,22 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti return -1; } break; + } else if ((uint32_t) res > (count - readd)) { + return -1; + } else { + r = (ssize_t) res; } break; case NC_TI_TLS: - r = nc_tls_read_wrap(session, (unsigned char *)buf + readd, count - readd); - if (r < 0) { + res = nc_tls_read_wrap(session, (unsigned char *)buf + readd, count - readd); + if (res < 0) { /* non-recoverable error */ - return r; + return -1; + } else if ((uint32_t) res > (count - readd)) { + return -1; + } else { + r = (ssize_t) res; } break; #endif /* NC_ENABLED_SSH_TLS */ @@ -160,9 +174,13 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti session->term_reason = NC_SESSION_TERM_OTHER; return -1; } + } else if ((r < 0) || (r > (UINT32_MAX - readd))) { + session->status = NC_STATUS_INVALID; + session->term_reason = NC_SESSION_TERM_OTHER; + return -1; } else { /* something read */ - readd += r; + readd += (uint32_t) r; /* reset inactive timeout */ nc_timeouttime_get(&ts_inact_timeout, inact_timeout); @@ -581,7 +599,8 @@ struct nc_wclb_arg { static int nc_write(struct nc_session *session, const void *buf, uint32_t count) { - int c, fd, interrupted; + ssize_t c; + int fd, interrupted, res; uint32_t written = 0; if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { @@ -605,14 +624,17 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) case NC_TI_UNIX: fd = session->ti_type == NC_TI_FD ? session->ti.fd.out : session->ti.unixsock.sock; c = write(fd, (char *)(buf + written), count - written); - if ((c < 0) && (errno == EAGAIN)) { + if ((c == -1) && (errno == EAGAIN)) { c = 0; - } else if ((c < 0) && (errno == EINTR)) { + } else if ((c == -1) && (errno == EINTR)) { c = 0; interrupted = 1; } else if (c < 0) { ERR(session, "Socket error (%s).", strerror(errno)); return -1; + } else if (c > (count - written)) { + ERR(session, "invalid number of bytes written (%ld > %u).", c, (count - written)); + return -1; } break; @@ -628,18 +650,20 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) session->term_reason = NC_SESSION_TERM_DROPPED; return -1; } - c = ssh_channel_write(session->ti.libssh.channel, (char *)(buf + written), count - written); - if ((c == SSH_ERROR) || (c == -1)) { + res = ssh_channel_write(session->ti.libssh.channel, (char *)(buf + written), count - written); + if ((res == SSH_ERROR) || (res == -1) || ((uint32_t) res > (count - written))) { ERR(session, "SSH channel write failed."); return -1; } + c = (ssize_t) res; break; case NC_TI_TLS: - c = nc_tls_write_wrap(session, (const unsigned char *)(buf + written), count - written); - if (c < 0) { + res = nc_tls_write_wrap(session, (const unsigned char *)(buf + written), count - written); + if ((res < 0) || ((uint32_t) res > (count - written))) { /* possible client dc, or some socket/TLS communication error */ return -1; } + c = (ssize_t) res; break; #endif /* NC_ENABLED_SSH_TLS */ default: @@ -652,10 +676,10 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) usleep(NC_TIMEOUT_STEP); } - written += c; + written += (uint32_t) c; } while (written < count); - return written; + return (written > INT_MAX) ? -1 : (int) written; } /** @@ -670,21 +694,21 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) static int nc_write_starttag_and_msg(struct nc_session *session, const void *buf, uint32_t count) { - int ret = 0, r; + int ret = 0, r, bufsize; char chunksize[24]; if (session->version == NC_VERSION_11) { - r = sprintf(chunksize, "\n#%" PRIu32 "\n", count); + bufsize = sprintf(chunksize, "\n#%" PRIu32 "\n", count); - r = nc_write(session, chunksize, r); - if (r == -1) { + r = nc_write(session, chunksize, bufsize); + if ((r < 0) || (r > bufsize)) { return -1; } ret += r; } r = nc_write(session, buf, count); - if (r == -1) { + if ((r < 0) || ((uint32_t) r > (UINT32_MAX - ret))) { return -1; } ret += r; @@ -747,23 +771,24 @@ nc_write_clb_flush(struct nc_wclb_arg *warg) static ssize_t nc_write_clb(void *arg, const void *buf, uint32_t count, int xmlcontent) { - ssize_t ret = 0, c; + ssize_t ret = 0; + int c; uint32_t l; struct nc_wclb_arg *warg = arg; if (!buf) { c = nc_write_clb_flush(warg); - if (c == -1) { + if (c < 0) { return -1; } - ret += c; + ret += (ssize_t) c; /* endtag */ c = nc_write_endtag(warg->session); - if (c == -1) { + if ((c < 0) || ((ssize_t) c > (SSIZE_MAX - ret))) { return -1; } - ret += c; + ret += (ssize_t) c; return ret; } @@ -771,19 +796,19 @@ nc_write_clb(void *arg, const void *buf, uint32_t count, int xmlcontent) if (warg->len && (warg->len + count > WRITE_BUFSIZE)) { /* dump current buffer */ c = nc_write_clb_flush(warg); - if (c == -1) { + if (c < 0) { return -1; } - ret += c; + ret += (ssize_t) c; } if (!xmlcontent && (count > WRITE_BUFSIZE)) { /* write directly */ c = nc_write_starttag_and_msg(warg->session, buf, count); - if (c == -1) { + if (c < 0) { return -1; } - ret += c; + ret += (ssize_t) c; } else { /* keep in buffer and write later */ if (xmlcontent) { @@ -791,7 +816,7 @@ nc_write_clb(void *arg, const void *buf, uint32_t count, int xmlcontent) if (warg->len + 5 >= WRITE_BUFSIZE) { /* buffer is full */ c = nc_write_clb_flush(warg); - if (c == -1) { + if (c < 0) { return -1; } } From 685c5dc127f63363b100d506cc3df176f50d5b21 Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:16:04 +0100 Subject: [PATCH 02/18] io: avoid NULL pointer access --- src/io.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/io.c b/src/io.c index 26b00fdc..a5157c13 100644 --- a/src/io.c +++ b/src/io.c @@ -968,8 +968,10 @@ nc_write_msg_io(struct nc_session *session, int io_timeout, int type, ...) switch (reply->type) { case NC_RPL_OK: - if (lyd_new_opaq2(reply_envp, NULL, "ok", NULL, rpc_envp->name.prefix, rpc_envp->name.module_ns, NULL)) { - lyd_free_tree(reply_envp); + if ((reply_envp == NULL) || (rpc_envp == NULL) || lyd_new_opaq2(reply_envp, NULL, "ok", NULL, rpc_envp->name.prefix, rpc_envp->name.module_ns, NULL)) { + if (reply_envp != NULL) { + lyd_free_tree(reply_envp); + } ERRINT; ret = NC_MSG_ERROR; From bde58f951c1f59dc76dd2dce9cd9e5a5718a285c Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:16:26 +0100 Subject: [PATCH 03/18] log: free allocated variable argument list --- src/log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/log.c b/src/log.c index e61a4383..52dc58f4 100644 --- a/src/log.c +++ b/src/log.c @@ -140,6 +140,7 @@ nc_log_vprintf(const struct nc_session *session, NC_VERB_LEVEL level, const char cleanup: free(msg); + va_end(args2); } void From 28b6b22d0b64f38457e0306d605bd1f9a90f305e Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:23:32 +0100 Subject: [PATCH 04/18] server_config: only free buffer if it was allocated before --- src/server_config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index a5c85a1a..a1b9aaa8 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -2841,7 +2841,7 @@ nc_server_config_endpoint_reference(const struct lyd_node *node, enum nc_operati /* listen */ free(endpt->referenced_endpt_name); endpt->referenced_endpt_name = NULL; - } else { + } else if (ch_endpt) { /* call home */ free(ch_endpt->referenced_endpt_name); ch_endpt->referenced_endpt_name = NULL; @@ -2869,7 +2869,7 @@ nc_server_config_endpoint_reference(const struct lyd_node *node, enum nc_operati free(endpt->referenced_endpt_name); endpt->referenced_endpt_name = strdup(lyd_get_value(node)); NC_CHECK_ERRMEM_GOTO(!endpt->referenced_endpt_name, ret = 1, cleanup); - } else { + } else if (ch_endpt) { free(ch_endpt->referenced_endpt_name); ch_endpt->referenced_endpt_name = strdup(lyd_get_value(node)); NC_CHECK_ERRMEM_GOTO(!ch_endpt->referenced_endpt_name, ret = 1, cleanup); From 58985a896a402b4de3b5237f38f8487d0663a46f Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:34:15 +0100 Subject: [PATCH 05/18] server_config: evaluate return value of lyd_find_path --- src/server_config.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/server_config.c b/src/server_config.c index a1b9aaa8..3d16775c 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -3233,7 +3233,11 @@ nc_server_config_create_cert_to_name(const struct lyd_node *node, struct nc_serv assert(!strcmp(LYD_NAME(node), "cert-to-name")); /* find the list's key */ - lyd_find_path(node, "id", 0, &n); + if (lyd_find_path(node, "id", 0, &n)) { + ERR(NULL, "Missing CTN id."); + ret = 1; + goto cleanup; + } assert(n); id = ((struct lyd_node_term *)n)->value.uint32; From 5f5e1c6ec36a41a4f6f2a0196ec1148c8c73734b Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:37:19 +0100 Subject: [PATCH 06/18] server_config_util_ssh: make hash buffer static to avoid large call stack --- src/server_config_util_ssh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/server_config_util_ssh.c b/src/server_config_util_ssh.c index 58e96781..7e356026 100644 --- a/src/server_config_util_ssh.c +++ b/src/server_config_util_ssh.c @@ -498,10 +498,12 @@ _nc_server_config_add_ssh_user_password(const struct ly_ctx *ctx, const char *tr int ret = 0; char *hashed_pw = NULL; const char *salt = "$6$idsizuippipk$"; - struct crypt_data cdata = {0}; + static struct crypt_data cdata; NC_CHECK_ARG_RET(NULL, ctx, tree_path, password, config, 1); + memset(&cdata, 0, sizeof(struct crypt_data)); + hashed_pw = crypt_r(password, salt, &cdata); if (!hashed_pw) { ERR(NULL, "Hashing password failed (%s).", strerror(errno)); From e1217b931ca41ad86b6adc23da479e080818f3a9 Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:41:06 +0100 Subject: [PATCH 07/18] session: protect session status against concurrent access --- src/session.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/session.c b/src/session.c index 647119de..c20a926c 100644 --- a/src/session.c +++ b/src/session.c @@ -881,8 +881,25 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) struct ly_in *msg; struct timespec ts; void *p; + NC_STATUS status; - if (!session || (session->status == NC_STATUS_CLOSING)) { + if (!session) { + return; + } + + if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { + /* CH LOCK */ + pthread_mutex_lock(&session->opts.server.ch_lock); + } + + status = session->status; + + if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { + /* CH UNLOCK */ + pthread_mutex_unlock(&session->opts.server.ch_lock); + } + + if (status == NC_STATUS_CLOSING) { return; } From f53a09d5a752ba1e1035aec250a5be4dc713571e Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 10:56:01 +0100 Subject: [PATCH 08/18] session_server: only free attr_p if it was allocated before --- src/session_server.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index 32964080..1d2a54fe 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -854,9 +854,11 @@ nc_server_init(void) goto error; } +#ifdef HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP if (attr_p) { pthread_rwlockattr_destroy(attr_p); } +#endif #ifdef NC_ENABLED_SSH_TLS if (curl_global_init(CURL_GLOBAL_SSL | CURL_GLOBAL_ACK_EINTR)) { @@ -898,9 +900,11 @@ nc_server_init(void) return 0; error: +#ifdef HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP if (attr_p) { pthread_rwlockattr_destroy(attr_p); } +#endif return -1; } @@ -2766,11 +2770,14 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, nc_server_ch_session_acquire_ctx_ const struct ly_ctx *ctx = NULL; int sock, ret; struct timespec ts_cur; - char *ip_host; + char *ip_host = NULL; sock = nc_sock_connect(endpt->src_addr, endpt->src_port, endpt->dst_addr, endpt->dst_port, NC_CH_CONNECT_TIMEOUT, &endpt->ka, &endpt->sock_pending, &ip_host); if (sock < 0) { + if (ip_host) { + free(ip_host); + } return NC_MSG_ERROR; } @@ -3990,6 +3997,7 @@ nc_server_notif_cert_exp_thread(void *arg) /* config changed, reload the certificates and intervals */ nc_server_notif_cert_exp_dates_destroy(exp_dates, exp_date_count); + exp_dates = NULL; nc_server_notif_cert_exp_intervals_get(default_intervals, 3, &intervals, &interval_count); @@ -4030,7 +4038,9 @@ nc_server_notif_cert_exp_thread(void *arg) if (targ->clb_free_data) { targ->clb_free_data(targ->clb_data); } - nc_server_notif_cert_exp_dates_destroy(exp_dates, exp_date_count); + if (exp_dates) { + nc_server_notif_cert_exp_dates_destroy(exp_dates, exp_date_count); + } free(targ); return NULL; } From 354d94e160846385b83b6bd317729752bca1efc4 Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Mon, 3 Mar 2025 11:01:20 +0100 Subject: [PATCH 09/18] session_server_ssh: make hash buffer static to avoid large call stack --- src/session_server_ssh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index b98c5fdc..bad7dbe3 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -625,10 +625,12 @@ static int nc_server_ssh_compare_password(const char *stored_pw, const char *received_pw) { char *received_pw_hash = NULL; - struct crypt_data cdata = {0}; + static struct crypt_data cdata; NC_CHECK_ARG_RET(NULL, stored_pw, received_pw, 1); + memset(&cdata, 0, sizeof(struct crypt_data)); + if (!stored_pw[0]) { if (!received_pw[0]) { WRN(NULL, "User authentication successful with an empty password!"); From 6c72c0dfe24c6a85b72213936b965fc23f490bdc Mon Sep 17 00:00:00 2001 From: Michal Struwe Date: Fri, 7 Mar 2025 14:22:45 +0100 Subject: [PATCH 10/18] session_server_tls: remove unneeded pointer resets --- src/session_server_tls.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/session_server_tls.c b/src/session_server_tls.c index d23c0e7e..175d0ac7 100644 --- a/src/session_server_tls.c +++ b/src/session_server_tls.c @@ -372,7 +372,6 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username fingerprint_match = 1; } free(digest_md5); - digest_md5 = NULL; /* SHA-1 */ } else if (!strncmp(ctn->fingerprint, "02", 2)) { @@ -388,7 +387,6 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username fingerprint_match = 1; } free(digest_sha1); - digest_sha1 = NULL; /* SHA-224 */ } else if (!strncmp(ctn->fingerprint, "03", 2)) { @@ -404,7 +402,6 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username fingerprint_match = 1; } free(digest_sha224); - digest_sha224 = NULL; /* SHA-256 */ } else if (!strncmp(ctn->fingerprint, "04", 2)) { @@ -420,7 +417,6 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username fingerprint_match = 1; } free(digest_sha256); - digest_sha256 = NULL; /* SHA-384 */ } else if (!strncmp(ctn->fingerprint, "05", 2)) { @@ -436,7 +432,6 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username fingerprint_match = 1; } free(digest_sha384); - digest_sha384 = NULL; /* SHA-512 */ } else if (!strncmp(ctn->fingerprint, "06", 2)) { @@ -452,7 +447,6 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username fingerprint_match = 1; } free(digest_sha512); - digest_sha512 = NULL; /* unknown */ } else { From 51857757f796422af2da69b16f199716bc293e62 Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 13:05:36 +0200 Subject: [PATCH 11/18] io: reverted some changes due to PR review --- src/io.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/io.c b/src/io.c index a5157c13..0e327890 100644 --- a/src/io.c +++ b/src/io.c @@ -109,11 +109,6 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti session->status = NC_STATUS_INVALID; session->term_reason = NC_SESSION_TERM_DROPPED; return -1; - } else if (r > (count - readd)) { - ERR(session, "Invalid number of bytes read (%ld > %u)", r, (count - readd)); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; - return -1; } break; @@ -138,8 +133,6 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti return -1; } break; - } else if ((uint32_t) res > (count - readd)) { - return -1; } else { r = (ssize_t) res; } @@ -632,9 +625,6 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) } else if (c < 0) { ERR(session, "Socket error (%s).", strerror(errno)); return -1; - } else if (c > (count - written)) { - ERR(session, "invalid number of bytes written (%ld > %u).", c, (count - written)); - return -1; } break; @@ -968,10 +958,9 @@ nc_write_msg_io(struct nc_session *session, int io_timeout, int type, ...) switch (reply->type) { case NC_RPL_OK: - if ((reply_envp == NULL) || (rpc_envp == NULL) || lyd_new_opaq2(reply_envp, NULL, "ok", NULL, rpc_envp->name.prefix, rpc_envp->name.module_ns, NULL)) { - if (reply_envp != NULL) { - lyd_free_tree(reply_envp); - } + assert(rpc_envp != NULL); + if (lyd_new_opaq2(reply_envp, NULL, "ok", NULL, rpc_envp->name.prefix, rpc_envp->name.module_ns, NULL)) { + lyd_free_tree(reply_envp); ERRINT; ret = NC_MSG_ERROR; From 311820f91a2c23c487fe94acd485efba145df7d5 Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 13:28:15 +0200 Subject: [PATCH 12/18] server_config: reverted some changes due to PR review --- src/server_config.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 3d16775c..9da1092a 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -2841,7 +2841,7 @@ nc_server_config_endpoint_reference(const struct lyd_node *node, enum nc_operati /* listen */ free(endpt->referenced_endpt_name); endpt->referenced_endpt_name = NULL; - } else if (ch_endpt) { + } else { /* call home */ free(ch_endpt->referenced_endpt_name); ch_endpt->referenced_endpt_name = NULL; @@ -2869,7 +2869,7 @@ nc_server_config_endpoint_reference(const struct lyd_node *node, enum nc_operati free(endpt->referenced_endpt_name); endpt->referenced_endpt_name = strdup(lyd_get_value(node)); NC_CHECK_ERRMEM_GOTO(!endpt->referenced_endpt_name, ret = 1, cleanup); - } else if (ch_endpt) { + } else { free(ch_endpt->referenced_endpt_name); ch_endpt->referenced_endpt_name = strdup(lyd_get_value(node)); NC_CHECK_ERRMEM_GOTO(!ch_endpt->referenced_endpt_name, ret = 1, cleanup); @@ -3232,12 +3232,8 @@ nc_server_config_create_cert_to_name(const struct lyd_node *node, struct nc_serv assert(!strcmp(LYD_NAME(node), "cert-to-name")); - /* find the list's key */ - if (lyd_find_path(node, "id", 0, &n)) { - ERR(NULL, "Missing CTN id."); - ret = 1; - goto cleanup; - } + /* find the list's key - ignore result using assert of reference argument instead */ + lyd_find_path(node, "id", 0, &n); assert(n); id = ((struct lyd_node_term *)n)->value.uint32; From ed2cc2d68c452622304afc0c9b5fae3e089dbe8a Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 13:46:10 +0200 Subject: [PATCH 13/18] server_config_util_ssh: changes due to PR review --- src/server_config_util_ssh.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/server_config_util_ssh.c b/src/server_config_util_ssh.c index 7e356026..7f8f1383 100644 --- a/src/server_config_util_ssh.c +++ b/src/server_config_util_ssh.c @@ -498,13 +498,18 @@ _nc_server_config_add_ssh_user_password(const struct ly_ctx *ctx, const char *tr int ret = 0; char *hashed_pw = NULL; const char *salt = "$6$idsizuippipk$"; - static struct crypt_data cdata; + struct crypt_data *cdata = NULL; NC_CHECK_ARG_RET(NULL, ctx, tree_path, password, config, 1); - memset(&cdata, 0, sizeof(struct crypt_data)); + cdata = (struct crypt_data *) calloc(sizeof(struct crypt_data), 1); + if (cdata == NULL) { + ERR(NULL, "Allocation of crypt_data struct failed."); + ret = 1; + goto cleanup; + } - hashed_pw = crypt_r(password, salt, &cdata); + hashed_pw = crypt_r(password, salt, cdata); if (!hashed_pw) { ERR(NULL, "Hashing password failed (%s).", strerror(errno)); ret = 1; @@ -517,6 +522,7 @@ _nc_server_config_add_ssh_user_password(const struct ly_ctx *ctx, const char *tr } cleanup: + free(cdata); return ret; } From 27841f5503ffd01e6a9246032b505b5d7947329e Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 14:25:17 +0200 Subject: [PATCH 14/18] session_server: reverted some changes due to PR review --- src/session_server.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index 1d2a54fe..c5d46c78 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -854,11 +854,9 @@ nc_server_init(void) goto error; } -#ifdef HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP if (attr_p) { pthread_rwlockattr_destroy(attr_p); } -#endif #ifdef NC_ENABLED_SSH_TLS if (curl_global_init(CURL_GLOBAL_SSL | CURL_GLOBAL_ACK_EINTR)) { @@ -900,11 +898,9 @@ nc_server_init(void) return 0; error: -#ifdef HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP if (attr_p) { pthread_rwlockattr_destroy(attr_p); } -#endif return -1; } @@ -2775,9 +2771,6 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, nc_server_ch_session_acquire_ctx_ sock = nc_sock_connect(endpt->src_addr, endpt->src_port, endpt->dst_addr, endpt->dst_port, NC_CH_CONNECT_TIMEOUT, &endpt->ka, &endpt->sock_pending, &ip_host); if (sock < 0) { - if (ip_host) { - free(ip_host); - } return NC_MSG_ERROR; } @@ -3997,7 +3990,6 @@ nc_server_notif_cert_exp_thread(void *arg) /* config changed, reload the certificates and intervals */ nc_server_notif_cert_exp_dates_destroy(exp_dates, exp_date_count); - exp_dates = NULL; nc_server_notif_cert_exp_intervals_get(default_intervals, 3, &intervals, &interval_count); From 5d2e24420edc864455a0dfc04b399205f0e012aa Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 14:30:53 +0200 Subject: [PATCH 15/18] session_server_ssh: changes due to PR review --- src/session_server_ssh.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index bad7dbe3..d8c3ad4a 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -625,12 +625,11 @@ static int nc_server_ssh_compare_password(const char *stored_pw, const char *received_pw) { char *received_pw_hash = NULL; - static struct crypt_data cdata; + struct crypt_data *cdata; + int ret; NC_CHECK_ARG_RET(NULL, stored_pw, received_pw, 1); - memset(&cdata, 0, sizeof(struct crypt_data)); - if (!stored_pw[0]) { if (!received_pw[0]) { WRN(NULL, "User authentication successful with an empty password!"); @@ -647,13 +646,23 @@ nc_server_ssh_compare_password(const char *stored_pw, const char *received_pw) return strcmp(stored_pw + 3, received_pw); } - received_pw_hash = crypt_r(received_pw, stored_pw, &cdata); + cdata = (struct crypt_data *) calloc(sizeof(struct crypt_data), 1); + if (cdata == NULL) { + ERR(NULL, "Allocation of crypt_data struct failed."); + return 1; + } + + received_pw_hash = crypt_r(received_pw, stored_pw, cdata); if (!received_pw_hash) { ERR(NULL, "Hashing the password failed (%s).", strerror(errno)); + free(cdata); return 1; } - return strcmp(received_pw_hash, stored_pw); + ret = strcmp(received_pw_hash, stored_pw); + free(cdata); + + return ret; } API int From 698ca7f1172ec26eb8846d77a6b142dfd518975d Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 14:37:48 +0200 Subject: [PATCH 16/18] session_server_tls: changes due to PR review --- src/session_server_tls.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/session_server_tls.c b/src/session_server_tls.c index 175d0ac7..5e7fe3fc 100644 --- a/src/session_server_tls.c +++ b/src/session_server_tls.c @@ -331,8 +331,8 @@ static int nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username) { int ret = 1, i, cert_count, fingerprint_match; - char *digest_md5 = NULL, *digest_sha1 = NULL, *digest_sha224 = NULL; - char *digest_sha256 = NULL, *digest_sha384 = NULL, *digest_sha512 = NULL; + char *digest_md5, *digest_sha1, *digest_sha224; + char *digest_sha256, *digest_sha384, *digest_sha512; void *cert; /* first make sure the entry is valid */ From aee4b9df64405f957c72e852e350709ff7af9c98 Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 16:11:09 +0200 Subject: [PATCH 17/18] session_server: changes due to PR review --- src/session_server.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index c5d46c78..9ac8f185 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -4030,9 +4030,7 @@ nc_server_notif_cert_exp_thread(void *arg) if (targ->clb_free_data) { targ->clb_free_data(targ->clb_data); } - if (exp_dates) { - nc_server_notif_cert_exp_dates_destroy(exp_dates, exp_date_count); - } + nc_server_notif_cert_exp_dates_destroy(exp_dates, exp_date_count); free(targ); return NULL; } From 79399c1f14bcfdc7d6fbc5f7c9437458a403381d Mon Sep 17 00:00:00 2001 From: Martin Herberg Date: Tue, 22 Apr 2025 16:12:48 +0200 Subject: [PATCH 18/18] io: reverted some changes due to PR review --- src/io.c | 81 +++++++++++++++++++++++--------------------------------- 1 file changed, 33 insertions(+), 48 deletions(-) diff --git a/src/io.c b/src/io.c index 0e327890..e7c851d2 100644 --- a/src/io.c +++ b/src/io.c @@ -64,7 +64,7 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti { uint32_t readd = 0; ssize_t r = -1; - int fd, interrupted, res; + int fd, interrupted; struct timespec ts_inact_timeout; assert(session); @@ -115,17 +115,16 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti #ifdef NC_ENABLED_SSH_TLS case NC_TI_SSH: /* read via libssh */ - res = ssh_channel_read(session->ti.libssh.channel, buf + readd, count - readd, 0); - if (res == SSH_AGAIN) { + r = ssh_channel_read(session->ti.libssh.channel, buf + readd, count - readd, 0); + if (r == SSH_AGAIN) { r = 0; break; - } else if (res == SSH_ERROR) { + } else if (r == SSH_ERROR) { ERR(session, "Reading from the SSH channel failed (%s).", ssh_get_error(session->ti.libssh.session)); session->status = NC_STATUS_INVALID; session->term_reason = NC_SESSION_TERM_OTHER; return -1; - } else if (res == 0) { - r = 0; + } else if (r == 0) { if (ssh_channel_is_eof(session->ti.libssh.channel)) { ERR(session, "SSH channel unexpected EOF."); session->status = NC_STATUS_INVALID; @@ -133,20 +132,14 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti return -1; } break; - } else { - r = (ssize_t) res; } break; case NC_TI_TLS: - res = nc_tls_read_wrap(session, (unsigned char *)buf + readd, count - readd); - if (res < 0) { + r = nc_tls_read_wrap(session, (unsigned char *)buf + readd, count - readd); + if (r < 0) { /* non-recoverable error */ - return -1; - } else if ((uint32_t) res > (count - readd)) { - return -1; - } else { - r = (ssize_t) res; + return r; } break; #endif /* NC_ENABLED_SSH_TLS */ @@ -167,13 +160,9 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti session->term_reason = NC_SESSION_TERM_OTHER; return -1; } - } else if ((r < 0) || (r > (UINT32_MAX - readd))) { - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; - return -1; } else { /* something read */ - readd += (uint32_t) r; + readd += r; /* reset inactive timeout */ nc_timeouttime_get(&ts_inact_timeout, inact_timeout); @@ -592,8 +581,7 @@ struct nc_wclb_arg { static int nc_write(struct nc_session *session, const void *buf, uint32_t count) { - ssize_t c; - int fd, interrupted, res; + int c, fd, interrupted; uint32_t written = 0; if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { @@ -617,9 +605,9 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) case NC_TI_UNIX: fd = session->ti_type == NC_TI_FD ? session->ti.fd.out : session->ti.unixsock.sock; c = write(fd, (char *)(buf + written), count - written); - if ((c == -1) && (errno == EAGAIN)) { + if ((c < 0) && (errno == EAGAIN)) { c = 0; - } else if ((c == -1) && (errno == EINTR)) { + } else if ((c < 0) && (errno == EINTR)) { c = 0; interrupted = 1; } else if (c < 0) { @@ -640,20 +628,18 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) session->term_reason = NC_SESSION_TERM_DROPPED; return -1; } - res = ssh_channel_write(session->ti.libssh.channel, (char *)(buf + written), count - written); - if ((res == SSH_ERROR) || (res == -1) || ((uint32_t) res > (count - written))) { + c = ssh_channel_write(session->ti.libssh.channel, (char *)(buf + written), count - written); + if ((c == SSH_ERROR) || (c == -1)) { ERR(session, "SSH channel write failed."); return -1; } - c = (ssize_t) res; break; case NC_TI_TLS: - res = nc_tls_write_wrap(session, (const unsigned char *)(buf + written), count - written); - if ((res < 0) || ((uint32_t) res > (count - written))) { + c = nc_tls_write_wrap(session, (const unsigned char *)(buf + written), count - written); + if (c < 0) { /* possible client dc, or some socket/TLS communication error */ return -1; } - c = (ssize_t) res; break; #endif /* NC_ENABLED_SSH_TLS */ default: @@ -666,10 +652,10 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) usleep(NC_TIMEOUT_STEP); } - written += (uint32_t) c; + written += c; } while (written < count); - return (written > INT_MAX) ? -1 : (int) written; + return written; } /** @@ -684,21 +670,21 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) static int nc_write_starttag_and_msg(struct nc_session *session, const void *buf, uint32_t count) { - int ret = 0, r, bufsize; + int ret = 0, r; char chunksize[24]; if (session->version == NC_VERSION_11) { - bufsize = sprintf(chunksize, "\n#%" PRIu32 "\n", count); + r = sprintf(chunksize, "\n#%" PRIu32 "\n", count); - r = nc_write(session, chunksize, bufsize); - if ((r < 0) || (r > bufsize)) { + r = nc_write(session, chunksize, r); + if (r == -1) { return -1; } ret += r; } r = nc_write(session, buf, count); - if ((r < 0) || ((uint32_t) r > (UINT32_MAX - ret))) { + if (r == -1) { return -1; } ret += r; @@ -761,24 +747,23 @@ nc_write_clb_flush(struct nc_wclb_arg *warg) static ssize_t nc_write_clb(void *arg, const void *buf, uint32_t count, int xmlcontent) { - ssize_t ret = 0; - int c; + ssize_t ret = 0, c; uint32_t l; struct nc_wclb_arg *warg = arg; if (!buf) { c = nc_write_clb_flush(warg); - if (c < 0) { + if (c == -1) { return -1; } - ret += (ssize_t) c; + ret += c; /* endtag */ c = nc_write_endtag(warg->session); - if ((c < 0) || ((ssize_t) c > (SSIZE_MAX - ret))) { + if (c == -1) { return -1; } - ret += (ssize_t) c; + ret += c; return ret; } @@ -786,19 +771,19 @@ nc_write_clb(void *arg, const void *buf, uint32_t count, int xmlcontent) if (warg->len && (warg->len + count > WRITE_BUFSIZE)) { /* dump current buffer */ c = nc_write_clb_flush(warg); - if (c < 0) { + if (c == -1) { return -1; } - ret += (ssize_t) c; + ret += c; } if (!xmlcontent && (count > WRITE_BUFSIZE)) { /* write directly */ c = nc_write_starttag_and_msg(warg->session, buf, count); - if (c < 0) { + if (c == -1) { return -1; } - ret += (ssize_t) c; + ret += c; } else { /* keep in buffer and write later */ if (xmlcontent) { @@ -806,7 +791,7 @@ nc_write_clb(void *arg, const void *buf, uint32_t count, int xmlcontent) if (warg->len + 5 >= WRITE_BUFSIZE) { /* buffer is full */ c = nc_write_clb_flush(warg); - if (c < 0) { + if (c == -1) { return -1; } }