From 8a410f9690f5c4cc35e19083093a57732a7ba4a9 Mon Sep 17 00:00:00 2001 From: Heikki Hannikainen Date: Tue, 1 Nov 2022 22:29:07 +0200 Subject: [PATCH 1/5] coverity: Fix format string bugs in logging Mostly long integers printed with %d. --- src/accept.c | 6 +++--- src/aprsc.c | 8 ++++---- src/config.c | 2 +- src/dupecheck.c | 2 +- src/http.c | 2 +- src/login.c | 4 ++-- src/outgoing.c | 12 ++++++------ src/status.c | 4 ++-- src/tls.c | 2 +- src/uplink.c | 2 +- src/worker.c | 4 ++-- 11 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/accept.c b/src/accept.c index 73734d0..ac5c836 100644 --- a/src/accept.c +++ b/src/accept.c @@ -779,7 +779,7 @@ struct client_t *accept_client_for_listener(struct listen_t *l, int fd, char *ad /* text format of servers' connected IP address + port */ if (getsockname(fd, &sa_loc.sa, &addr_len_loc) == 0) { /* Fails very rarely.. */ if (addr_len_loc > sizeof(sa_loc)) - hlog(LOG_ERR, "accept_client_for_listener: getsockname for client %s truncated local address of %d to %d bytes", c->addr_rem, addr_len_loc, sizeof(sa_loc)); + hlog(LOG_ERR, "accept_client_for_listener: getsockname for client %s truncated local address of %d to %ld bytes", c->addr_rem, addr_len_loc, sizeof(sa_loc)); /* present my socket end address as a malloced string... */ s = strsockaddr( &sa_loc.sa, addr_len_loc ); } else { @@ -913,14 +913,14 @@ static void do_accept(struct listen_t *l) */ if (l->portaccount->gauge >= l->clients_max || inbound_connects.gauge >= maxclients) { if (inbound_connects.gauge >= maxclients) { - hlog(LOG_INFO, "%s - Denied client on fd %d from %s: MaxClients reached (%d)", l->addr_s, fd, s, inbound_connects.gauge); + hlog(LOG_INFO, "%s - Denied client on fd %d from %s: MaxClients reached (%ld)", l->addr_s, fd, s, inbound_connects.gauge); /* The "if" is here only to silence a compiler warning * about ignoring the result value. We're really * disconnecting the client right now, so we don't care. */ if (write(fd, "# Server full\r\n", 15)) {}; } else { - hlog(LOG_INFO, "%s - Denied client on fd %d from %s: Too many clients on Listener (%d)", l->addr_s, fd, s, l->portaccount->gauge); + hlog(LOG_INFO, "%s - Denied client on fd %d from %s: Too many clients on Listener (%ld)", l->addr_s, fd, s, l->portaccount->gauge); if (write(fd, "# Port full\r\n", 13)) {}; } close(fd); diff --git a/src/aprsc.c b/src/aprsc.c index 2fa594f..da0eaa6 100644 --- a/src/aprsc.c +++ b/src/aprsc.c @@ -567,7 +567,7 @@ static void generate_instance_id(void) if (fd >= 0) { l = read(fd, &seed, sizeof(seed)); if (l != sizeof(seed)) { - hlog(LOG_ERR, "read(/dev/urandom, %d) for seed failed: %s", sizeof(seed), strerror(errno)); + hlog(LOG_ERR, "read(/dev/urandom, %lu) for seed failed: %s", sizeof(seed), strerror(errno)); close(fd); fd = -1; } @@ -801,14 +801,14 @@ void time_thread(void *asdf) double slept = timeval_diff(sleep_start, sleep_end); if (slept > 0.90) - hlog(LOG_WARNING, "time keeping: sleep of %d ms took %.6f s!", sleep_req.tv_nsec / 1000 / 1000, slept); + hlog(LOG_WARNING, "time keeping: sleep of %ld ms took %.6f s!", sleep_req.tv_nsec / 1000 / 1000, slept); /* catch some oddities with time keeping */ if (tick != previous_tick) { if (previous_tick > tick) { - hlog(LOG_WARNING, "time keeping: Time jumped backward by %d seconds!", previous_tick - tick); + hlog(LOG_WARNING, "time keeping: Time jumped backward by %ld seconds!", previous_tick - tick); } else if (previous_tick < tick-1) { - hlog(LOG_WARNING, "time keeping: Time jumped forward by %d seconds!", tick - previous_tick); + hlog(LOG_WARNING, "time keeping: Time jumped forward by %ld seconds!", tick - previous_tick); } previous_tick = tick; diff --git a/src/config.c b/src/config.c index 13260e2..5c114af 100644 --- a/src/config.c +++ b/src/config.c @@ -1100,7 +1100,7 @@ int do_listen(struct listen_config_t **lq, int argc, char **argv) /* SSL requires both a cert and a key */ if ((l->certfile && !l->keyfile) || (l->keyfile && !l->certfile)) { - hlog(LOG_ERR, "Listen: Only one of tlskey and tlscert defined for '%' - both needed for TLS", argv[1]); + hlog(LOG_ERR, "Listen: Only one of tlskey and tlscert defined for '%s' - both needed for TLS", argv[1]); free_listen_config(&l); return -2; } diff --git a/src/dupecheck.c b/src/dupecheck.c index 94a12e6..a972803 100644 --- a/src/dupecheck.c +++ b/src/dupecheck.c @@ -658,7 +658,7 @@ static int dupecheck_drain_worker(struct worker_t *w, // check that the first packet isn't very old, it indicates we're not doing well if ((pb_list) && tick - pb_list->t > 10) { - hlog(LOG_ERR, "dupecheck: drain got packet %d aged %d sec from worker %d\n%*s", + hlog(LOG_ERR, "dupecheck: drain got packet %d aged %ld sec from worker %d\n%*s", pb_list->seqnum, tick - pb_list->t, w->id, pb_list->packet_len-2, pb_list->data); } diff --git a/src/http.c b/src/http.c index eec1b9e..ed99790 100644 --- a/src/http.c +++ b/src/http.c @@ -855,7 +855,7 @@ void lang_scan(void) if (ret == 0) { int i; - hlog(LOG_DEBUG, "%d language files found", globbuf.gl_pathc); + hlog(LOG_DEBUG, "%ld language files found", globbuf.gl_pathc); new_language_files = hmalloc(sizeof(*new_language_files) * globbuf.gl_pathc); memset(new_language_files, 0, sizeof(*new_language_files) * globbuf.gl_pathc); diff --git a/src/login.c b/src/login.c index 13e8d9e..2e25653 100644 --- a/src/login.c +++ b/src/login.c @@ -104,7 +104,7 @@ int http_udp_upload_login(const char *addr_rem, char *s, char **username, const for (i = 2; i < argc; i++) { if (strcasecmp(argv[i], "pass") == 0) { if (++i >= argc) { - hlog(LOG_WARNING, "%s (%s): %s: No passcode after pass command", addr_rem, log_source, username); + hlog(LOG_WARNING, "%s (%s): %s: No passcode after pass command", addr_rem, log_source, *username); break; } @@ -114,7 +114,7 @@ int http_udp_upload_login(const char *addr_rem, char *s, char **username, const validated = 1; } else if (strcasecmp(argv[i], "vers") == 0) { if (i+2 >= argc) { - hlog(LOG_DEBUG, "%s (%s): %s: No application name and version after vers command", addr_rem, username, log_source); + hlog(LOG_DEBUG, "%s (%s): %s: No application name and version after vers command", addr_rem, *username, log_source); break; } diff --git a/src/outgoing.c b/src/outgoing.c index 94006d4..7b268e9 100644 --- a/src/outgoing.c +++ b/src/outgoing.c @@ -144,21 +144,21 @@ void process_outgoing(struct worker_t *self) //__sync_synchronize(); /* Some safety checks against bugs and overload conditions */ if (pb->is_free) { - hlog(LOG_ERR, "worker %d: process_outgoing got pbuf %d marked free, age %d (now %d t %d)\n%.*s", + hlog(LOG_ERR, "worker %d: process_outgoing got pbuf %d marked free, age %ld (now %ld t %ld)\n%.*s", self->id, pb->seqnum, tick - pb->t, tick, pb->t, pb->packet_len-2, pb->data); abort(); /* this would be pretty bad, so we crash immediately */ } else if (pb->t > tick + 2) { /* 2-second offset is normal in case of one thread updating tick earlier than another * and a little thread scheduling luck */ - hlog(LOG_ERR, "worker %d: process_outgoing got packet %d from future with t %d > tick %d!\n%.*s", + hlog(LOG_ERR, "worker %d: process_outgoing got packet %d from future with t %ld > tick %ld!\n%.*s", self->id, pb->seqnum, pb->t, tick, pb->packet_len-2, pb->data); self->internal_packet_drops++; if (self->internal_packet_drops > 10) status_error(86400, "packet_drop_future"); } else if (tick - pb->t > 5) { /* this is a bit too old, are we stuck? */ - hlog(LOG_ERR, "worker %d: process_outgoing got packet %d aged %d sec (now %d t %d)\n%.*s", + hlog(LOG_ERR, "worker %d: process_outgoing got packet %d aged %ld sec (now %ld t %ld)\n%.*s", self->id, pb->seqnum, tick - pb->t, tick, pb->t, pb->packet_len-2, pb->data); self->internal_packet_drops++; if (self->internal_packet_drops > 10) @@ -172,14 +172,14 @@ void process_outgoing(struct worker_t *self) while ((pb = *self->pbuf_global_dupe_prevp)) { if (pb->is_free) { - hlog(LOG_ERR, "worker %d: process_outgoing got dupe %d marked free, age %d (now %d t %d)\n%.*s", + hlog(LOG_ERR, "worker %d: process_outgoing got dupe %d marked free, age %ld (now %ld t %ld)\n%.*s", self->id, pb->seqnum, tick - pb->t, tick, pb->t, pb->packet_len-2, pb->data); abort(); } else if (pb->t > tick + 2) { - hlog(LOG_ERR, "worker: process_outgoing got dupe from future %d with t %d > tick %d!\n%.*s", + hlog(LOG_ERR, "worker: process_outgoing got dupe from future %d with t %ld > tick %ld!\n%.*s", pb->seqnum, pb->t, tick, pb->packet_len-2, pb->data); } else if (tick - pb->t > 5) { - hlog(LOG_ERR, "worker: process_outgoing got dupe %d aged %d sec\n%.*s", + hlog(LOG_ERR, "worker: process_outgoing got dupe %d aged %ld sec\n%.*s", pb->seqnum, tick - pb->t, pb->packet_len-2, pb->data); } else { process_outgoing_single(self, pb); diff --git a/src/status.c b/src/status.c index 6ae0c7b..245ee28 100644 --- a/src/status.c +++ b/src/status.c @@ -511,7 +511,7 @@ int json_write_file(char *basename, const char *s) /* check if we're having I/O delays */ time(&end_t); if (end_t - start_t > 2) { - hlog(LOG_ERR, "json file update took %d seconds", end_t - start_t); + hlog(LOG_ERR, "json file update took %ld seconds", end_t - start_t); } return 0; @@ -593,7 +593,7 @@ int status_dump_file(void) /* check if we're having delays */ time(&end_t); if (end_t - start_t > 2) { - hlog(LOG_ERR, "status counters update took %d seconds", end_t - start_t); + hlog(LOG_ERR, "status counters update took %ld seconds", end_t - start_t); } return 0; diff --git a/src/tls.c b/src/tls.c index 95f06d9..5495e61 100644 --- a/src/tls.c +++ b/src/tls.c @@ -197,7 +197,7 @@ static void ssl_error(int level, const char *msg) ERR_error_string_n(n, errstr, sizeof(errstr)); errstr[sizeof(errstr)-1] = 0; - hlog(level, "%s (%d): %s", msg, n, errstr); + hlog(level, "%s (%ld): %s", msg, n, errstr); } } diff --git a/src/uplink.c b/src/uplink.c index 781beee..2f52e6d 100644 --- a/src/uplink.c +++ b/src/uplink.c @@ -332,7 +332,7 @@ int uplink_login_handler(struct worker_t *self, struct client_t *c, int l4proto, // TODO: The uplink login command here could maybe be improved to send a filter command. len = sprintf(buf, "user %s pass %s vers %s\r\n", serverid, passcode, verstr_aprsis); - hlog(LOG_DEBUG, "%s: my login string: \"%.*s\"", c->addr_rem, len-2, buf, len); + hlog(LOG_DEBUG, "%s: my login string: \"%.*s\"", c->addr_rem, len-2, buf); rc = c->write(self, c, buf, len); if (rc < -2) return rc; // the client was destroyed by client_write, don't touch it diff --git a/src/worker.c b/src/worker.c index 4384777..d6e5987 100644 --- a/src/worker.c +++ b/src/worker.c @@ -824,7 +824,7 @@ void client_close(struct worker_t *self, struct client_t *c, int errnum) // TODO: log validation status, ssl status, ssl cert info, tcp/sctp - hlog( LOG_INFO, "%s %s %s (%s) closed after %d s: %s, tx/rx %lld/%lld bytes %lld/%lld pkts, dropped %lld, fd %d, worker %d%s%s%s%s", + hlog( LOG_INFO, "%s %s %s (%s) closed after %ld s: %s, tx/rx %lld/%lld bytes %lld/%lld pkts, dropped %lld, fd %d, worker %d%s%s%s%s", ( (c->flags & CLFLAGS_UPLINKPORT) ? ((c->state == CSTATE_COREPEER) ? "Peer" : "Uplink") : "Client" ), protocol_str(c), @@ -1856,7 +1856,7 @@ void worker_thread(struct worker_t *self) #if 1 if (t7-t1 > 1 || t7-t1 < 0) // only report if the delay is over 1 seconds. they are a LOT rarer - hlog( LOG_DEBUG, "Worker thread %d loop step delays: dt2: %d dt3: %d dt4: %d dt5: %d dt6: %d dt7: %d", + hlog( LOG_DEBUG, "Worker thread %d loop step delays: dt2: %ld dt3: %ld dt4: %ld dt5: %ld dt6: %ld dt7: %ld", self->id, t2-t1, t3-t1, t4-t1, t5-t1, t6-t1, t7-t1 ); #endif } From 18505a1c4a8f8c525065afb77a75734e8b73db35 Mon Sep 17 00:00:00 2001 From: Heikki Hannikainen Date: Tue, 1 Nov 2022 23:06:44 +0200 Subject: [PATCH 2/5] dupecheck: Remove unused variables --- src/dupecheck.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/dupecheck.c b/src/dupecheck.c index a972803..99839d8 100644 --- a/src/dupecheck.c +++ b/src/dupecheck.c @@ -711,9 +711,8 @@ static void dupecheck_thread(void) struct worker_t *w; struct pbuf_t *pb_out, **pb_out_prevp, *pb_out_last; struct pbuf_t *pb_out_dupe, **pb_out_dupe_prevp, *pb_out_dupe_last; - int n; int e; - int c, d; + int c; int pb_out_count, pb_out_dupe_count; time_t cleanup_tick = tick; @@ -740,7 +739,6 @@ static void dupecheck_thread(void) hlog(LOG_INFO, "Dupecheck thread ready."); while (!dupecheck_shutting_down) { - n = d = 0; pb_out = NULL; pb_out_prevp = &pb_out; pb_out_dupe = NULL; @@ -754,21 +752,21 @@ static void dupecheck_thread(void) if (!w->pbuf_incoming) continue; - n += dupecheck_drain_worker(w, + dupecheck_drain_worker(w, &pb_out_prevp, &pb_out_last, &pb_out_dupe_prevp, &pb_out_dupe_last, &pb_out_count, &pb_out_dupe_count); } if ((http_worker) && http_worker->pbuf_incoming) { - n += dupecheck_drain_worker(http_worker, + dupecheck_drain_worker(http_worker, &pb_out_prevp, &pb_out_last, &pb_out_dupe_prevp, &pb_out_dupe_last, &pb_out_count, &pb_out_dupe_count); } if ((udp_worker) && udp_worker->pbuf_incoming) { - n += dupecheck_drain_worker(udp_worker, + dupecheck_drain_worker(udp_worker, &pb_out_prevp, &pb_out_last, &pb_out_dupe_prevp, &pb_out_dupe_last, &pb_out_count, &pb_out_dupe_count); @@ -846,8 +844,6 @@ static void dupecheck_thread(void) dupecheck_cleanup(); } - // if (n > 0) - // hlog(LOG_DEBUG, "Dupecheck did analyze %d packets, found %d duplicates", n, pb_out_dupe_count); /* sleep a little */ #ifdef USE_EVENTFD int p = poll(&dupecheck_eventfd_poll, 1, 1000); From cf468c9af9143bb94fe4d3e3bf49a203f418e5f4 Mon Sep 17 00:00:00 2001 From: Heikki Hannikainen Date: Tue, 1 Nov 2022 23:20:46 +0200 Subject: [PATCH 3/5] coverity: Fix some uninitialized variables --- src/filter.c | 2 +- src/http.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/filter.c b/src/filter.c index 089a654..50bf0e7 100644 --- a/src/filter.c +++ b/src/filter.c @@ -554,7 +554,7 @@ static int filter_wx_insert(struct pbuf_t *pb) const int keylen = pb->srccall_end - key; uint32_t hash; int idx; - char uckey[CALLSIGNLEN_MAX+1]; + char uckey[CALLSIGNLEN_MAX+1] = ""; /* If it is not a WX packet without position, we are not intrerested */ if (!((pb->packettype & T_WX) && !(pb->flags & F_HASPOS))) diff --git a/src/http.c b/src/http.c index ed99790..d681aec 100644 --- a/src/http.c +++ b/src/http.c @@ -419,6 +419,8 @@ static int http_check_req_compressed(struct evhttp_request *r) static int http_compress_gzip(char *in, int ilen, char *out, int ospace) { z_stream ctx; + + memset(&ctx, 0, sizeof(ctx)); ctx.zalloc = Z_NULL; ctx.zfree = Z_NULL; From 4dedec62b29cb634b0d2e0e34ce55ab257bbf412 Mon Sep 17 00:00:00 2001 From: Heikki Hannikainen Date: Tue, 1 Nov 2022 23:24:55 +0200 Subject: [PATCH 4/5] uplink: Check return value of getsockname, fix rare resource leak --- src/uplink.c | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/uplink.c b/src/uplink.c index 2f52e6d..eeca686 100644 --- a/src/uplink.c +++ b/src/uplink.c @@ -584,6 +584,7 @@ connerr: close(fd); fd = -1; hfree(addr_s); + addr_s = NULL; } freeaddrinfo(ai); /* Not needed anymore.. */ @@ -598,6 +599,7 @@ connerr: hlog(LOG_ERR, "Uplink %s: client_alloc() failed, too many clients", l->name); close(fd); l->state = UPLINK_ST_NOT_LINKED; + hfree(addr_s); return -3; /* No successfull connection at any address.. */ } @@ -619,26 +621,33 @@ connerr: on us in which case it gets abandoned a bit further below. */ addr_len = sizeof(sa); - getpeername(fd, (struct sockaddr *)&sa, &addr_len); - //s = strsockaddr( &sa.sa, addr_len ); /* server side address */ - strncpy(c->addr_rem, addr_s, sizeof(c->addr_rem)); - c->addr_rem[sizeof(c->addr_rem)-1] = 0; - hfree(addr_s); - c->addr = sa; + if (getpeername(fd, (struct sockaddr *)&sa, &addr_len) == -1) { + hlog(LOG_ERR, "Uplink %s: Failed to get socket peer name: %s", l->name, strerror(errno)); + } else { + //s = strsockaddr( &sa.sa, addr_len ); /* server side address */ + strncpy(c->addr_rem, addr_s, sizeof(c->addr_rem)); + c->addr_rem[sizeof(c->addr_rem)-1] = 0; + hfree(addr_s); + addr_s = NULL; + c->addr = sa; - /* hex format of client's IP address + port */ - - char *s = hexsockaddr( &sa.sa, addr_len ); - strncpy(c->addr_hex, s, sizeof(c->addr_hex)); - c->addr_hex[sizeof(c->addr_hex)-1] = 0; - hfree(s); + /* hex format of client's IP address + port */ + char *s = hexsockaddr( &sa.sa, addr_len ); + strncpy(c->addr_hex, s, sizeof(c->addr_hex)); + c->addr_hex[sizeof(c->addr_hex)-1] = 0; + hfree(s); + } addr_len = sizeof(sa); - getsockname(fd, (struct sockaddr *)&sa, &addr_len); - s = strsockaddr( &sa.sa, addr_len ); /* client side address */ - strncpy(c->addr_loc, s, sizeof(c->addr_loc)); - c->addr_loc[sizeof(c->addr_loc)-1] = 0; - hfree(s); + if (getsockname(fd, (struct sockaddr *)&sa, &addr_len) == -1) { + hlog(LOG_ERR, "Uplink %s: Failed to get local socket address: %s", l->name, strerror(errno)); + c->addr_loc[0] = 0; + } else { + char *s = strsockaddr( &sa.sa, addr_len ); /* client side address */ + strncpy(c->addr_loc, s, sizeof(c->addr_loc)); + c->addr_loc[sizeof(c->addr_loc)-1] = 0; + hfree(s); + } hlog(LOG_INFO, "Uplink %s: %s: Connection established on fd %d using source address %s", l->name, c->addr_rem, c->fd, c->addr_loc); @@ -698,6 +707,8 @@ err: if (uc) uplink_client_free(uc); l->state = UPLINK_ST_NOT_LINKED; + if (addr_s) + hfree(addr_s); return -1; } From f80e709945f131dbb764c905e9abbccc9e85c521 Mon Sep 17 00:00:00 2001 From: Heikki Hannikainen Date: Wed, 2 Nov 2022 00:02:27 +0200 Subject: [PATCH 5/5] tls: Disable TLSv1.1 to meet PCI-DSS requirements --- src/tls.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/src/tls.c b/src/tls.c index 5495e61..f60cdb1 100644 --- a/src/tls.c +++ b/src/tls.c @@ -395,28 +395,10 @@ int ssl_create(struct ssl_t *ssl, void *data) SSL_CTX_set_options(ssl->ctx, SSL_OP_SINGLE_DH_USE); /* SSL protocols not configurable for now */ - int protocols = SSL_PROTOCOLS; - - if (!(protocols & NGX_SSL_SSLv2)) { - SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_SSLv2); - } - if (!(protocols & NGX_SSL_SSLv3)) { - SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_SSLv3); - } - if (!(protocols & NGX_SSL_TLSv1)) { - SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_TLSv1); - } - -#ifdef SSL_OP_NO_TLSv1_1 - if (!(protocols & NGX_SSL_TLSv1_1)) { - SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_TLSv1_1); - } -#endif -#ifdef SSL_OP_NO_TLSv1_2 - if (!(protocols & NGX_SSL_TLSv1_2)) { - SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_TLSv1_2); - } -#endif + SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_SSLv2); + SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_SSLv3); + SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_TLSv1); + SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_TLSv1_1); #ifdef SSL_OP_NO_COMPRESSION SSL_CTX_set_options(ssl->ctx, SSL_OP_NO_COMPRESSION);