From 4bd357ecaacd388963489c91e84f244a6d4c174d Mon Sep 17 00:00:00 2001 From: Jared Mauch Date: Wed, 17 Dec 2025 12:01:11 -0500 Subject: [PATCH] Various minor code improvements --- ChangeLog | 14 ++++++++++++++ bgpdump.c | 52 ++++++++++++++++++++++++++------------------------ bgpdump_lib.c | 53 ++++++++++++++++++++++++++++++++++++--------------- example.c | 2 +- inet_ntop.c | 3 ++- util.c | 18 +++++++++-------- util.h | 4 ++-- 7 files changed, 94 insertions(+), 52 deletions(-) diff --git a/ChangeLog b/ChangeLog index c1c6c1d..136816c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,20 @@ PLEASE DO NOT E-MAIL INDIVIDUAL DEVELOPERS ABOUT ISSUES, AS YOUR E-MAIL MAY BE LOST ========================================================== +2024-12-17 Jared Mauch + + * Fixed critical return value bug in process_mrtd_table_dump_v2_peer_index_table + that caused peer index tables to be incorrectly marked as failed + * Fixed buffer overflow risks in time string formatting and logging functions + * Fixed integer overflow/underflow risks in AS path processing and message parsing + * Fixed incorrect memory copy in large community processing (byte offset bug) + * Fixed memory leak in bgpdump_open_dump when malloc fails + * Fixed wrong field reference in state change processing + * Replaced all sprintf calls with snprintf for buffer safety + * Added size parameters to time2str() and int2str() utility functions + * Fixed alignment warnings by replacing unsafe pointer casts with memcpy + * Improved code consistency by using sizeof() for all buffer size checks + 2023-11-05 Colin Petrie * Include CPPFLAGS diff --git a/bgpdump.c b/bgpdump.c index d0f3233..e4c505c 100644 --- a/bgpdump.c +++ b/bgpdump.c @@ -318,17 +318,19 @@ void process(BGPDUMP_ENTRY *entry) { int len; date=gmtime(&entry->time); - time2str(date,time_str_fixed); + time2str(date,time_str_fixed, sizeof(time_str_fixed)); if (mode == 1) { // Timestamp mode - len = sprintf(time_str, "%lld", (long long)entry->time); + len = snprintf(time_str, sizeof(time_str), "%lld", (long long)entry->time); } else { - len = time2str(date,time_str); + len = time2str(date,time_str, sizeof(time_str)); } // Appending microseconds to time_str if needed if (entry->type == BGPDUMP_TYPE_ZEBRA_BGP_ET) { - sprintf(time_str + len, ".%06ld", entry->ms); + if (len < sizeof(time_str) - 8) { + sprintf(time_str + len, ".%06ld", entry->ms); + } } if (mode==0) @@ -395,7 +397,7 @@ void process(BGPDUMP_ENTRY *entry) { //printf("FROM: %s AS%d\n",inet_ntoa(entry->body.mrtd_table_dump.peer_ip.v4_addr),entry->body.mrtd_table_dump.peer_as); //time2str(localtime(&entry->body.mrtd_table_dump.uptime),time_str2); - time2str(gmtime(&entry->body.mrtd_table_dump.uptime),time_str2); + time2str(gmtime(&entry->body.mrtd_table_dump.uptime),time_str2, sizeof(time_str2)); printf("ORIGINATED: %s\n",time_str2); if (entry->attr && entry->attr->len) show_attr(entry->attr); @@ -463,11 +465,11 @@ void process(BGPDUMP_ENTRY *entry) { fmt_ipv6(e->entries[i].peer->peer_ip, peer_ip); #endif } else { - sprintf(peer_ip, "[N/A, unsupported AF]"); + snprintf(peer_ip, sizeof(peer_ip), "[N/A, unsupported AF]"); } printf("FROM: %s AS%u\n", peer_ip, e->entries[i].peer->peer_as); time_t time_temp = (time_t)((e->entries[i]).originated_time); - time2str(gmtime(&time_temp),time_str); + time2str(gmtime(&time_temp),time_str, sizeof(time_str)); printf("ORIGINATED: %s\n",time_str); if (e->entries[i].attr && e->entries[i].attr->len) show_attr(e->entries[i].attr); @@ -1017,7 +1019,7 @@ void process(BGPDUMP_ENTRY *entry) { case AFI_IP: default: if (entry->body.zebra_state_change.source_ip.v4_addr.s_addr != 0x00000000L) - printf(" %s ",inet_ntoa(entry->body.zebra_message.source_ip.v4_addr)); + printf(" %s ",inet_ntoa(entry->body.zebra_state_change.source_ip.v4_addr)); else printf(" N/A "); } @@ -1040,7 +1042,7 @@ void process(BGPDUMP_ENTRY *entry) { #endif case AFI_IP: default: - sprintf(prefix, "%s", inet_ntoa(entry->body.zebra_state_change.source_ip.v4_addr)); + snprintf(prefix, sizeof(prefix), "%s", inet_ntoa(entry->body.zebra_state_change.source_ip.v4_addr)); break; } show_line_prefix(bgp4mp_format, time_str, "STATE"); @@ -1078,10 +1080,10 @@ void process_bgpdump_mrtd_bgp(BGPDUMP_ENTRY *entry) { date=gmtime(&entry->time); if (mode == 1) { - sprintf(time_str, "%lld", (long long)entry->time); + snprintf(time_str, sizeof(time_str), "%lld", (long long)entry->time); } else { - time2str(date, time_str); + time2str(date, time_str, sizeof(time_str)); } switch (entry->subtype) { @@ -1530,9 +1532,9 @@ static void table_line_announce(struct prefix *prefix,int count,BGPDUMP_ENTRY *e unsigned int nmed; if (entry->attr->flag & ATTR_FLAG_BIT(BGP_ATTR_ATOMIC_AGGREGATE)) - sprintf(aggregate,"AG"); + snprintf(aggregate, sizeof(aggregate), "AG"); else - sprintf(aggregate,"NAG"); + snprintf(aggregate, sizeof(aggregate), "NAG"); for (idx=0;idxattr->flag & ATTR_FLAG_BIT(BGP_ATTR_ATOMIC_AGGREGATE)) - sprintf(aggregate,"AG"); + snprintf(aggregate, sizeof(aggregate), "AG"); else - sprintf(aggregate,"NAG"); + snprintf(aggregate, sizeof(aggregate), "NAG"); for (idx=0;idxattr->flag & ATTR_FLAG_BIT(BGP_ATTR_ATOMIC_AGGREGATE)) - sprintf(aggregate,"AG"); + snprintf(aggregate, sizeof(aggregate), "AG"); else - sprintf(aggregate,"NAG"); + snprintf(aggregate, sizeof(aggregate), "NAG"); for (idx=0;idxattr->flag & ATTR_FLAG_BIT(BGP_ATTR_ATOMIC_AGGREGATE)) - sprintf(aggregate,"AG"); + snprintf(aggregate, sizeof(aggregate), "AG"); else - sprintf(aggregate,"NAG"); + snprintf(aggregate, sizeof(aggregate), "NAG"); time_t *t; if (timetype==0) { @@ -1923,10 +1925,10 @@ static void table_line_mrtd_route(BGPDUMP_MRTD_TABLE_DUMP *route,BGPDUMP_ENTRY * t = &route->uptime; } if (mode == 1) { - sprintf(time_str, "%lld", (long long)*t); + snprintf(time_str, sizeof(time_str), "%lld", (long long)*t); } else { date=gmtime(t); - time2str(date, time_str); + time2str(date, time_str, sizeof(time_str)); } show_line_prefix("TABLE_DUMP", time_str, "B"); @@ -2015,7 +2017,7 @@ static void table_line_dump_v2_prefix(BGPDUMP_TABLE_DUMP_V2_PREFIX *e,BGPDUMP_EN struct tm *date = NULL; unsigned int npref; unsigned int nmed; - char time_str[20]; + char time_str[32]; char peer[BGPDUMP_ADDRSTRLEN], prefix[BGPDUMP_ADDRSTRLEN], nexthop[BGPDUMP_ADDRSTRLEN]; int i; @@ -2054,10 +2056,10 @@ static void table_line_dump_v2_prefix(BGPDUMP_TABLE_DUMP_V2_PREFIX *e,BGPDUMP_EN t = &tmp; } if (mode == 1) { - sprintf(time_str, "%lld", (long long)*t); + snprintf(time_str, sizeof(time_str), "%lld", (long long)*t); } else { date=gmtime(t); - time2str(date, time_str); + time2str(date, time_str, sizeof(time_str)); } (addpath) diff --git a/bgpdump_lib.c b/bgpdump_lib.c index b26b01d..eaf2340 100644 --- a/bgpdump_lib.c +++ b/bgpdump_lib.c @@ -97,6 +97,7 @@ BGPDUMP *bgpdump_open_dump(const char *filename) { BGPDUMP *this_dump = malloc(sizeof(BGPDUMP)); if (this_dump == NULL) { perror("malloc"); + cfr_close(f); return NULL; } strcpy(this_dump->filename, "[STDIN]"); @@ -622,7 +623,7 @@ int process_mrtd_table_dump_v2_peer_index_table(struct mstream *s,BGPDUMP_ENTRY t->entries[i].peer_as = read_asn(s, ASN16_LEN); } - return 0; + return 1; } @@ -919,6 +920,12 @@ int process_zebra_bgp_message(struct mstream *s,BGPDUMP_ENTRY *entry, u_int8_t a mstream_getw(s,&entry->body.zebra_message.size); + /* Validate message size to prevent integer underflow */ + if (entry->body.zebra_message.size < sizeof(marker) + sizeof(u_int16_t)) { + warn("bgp_message: invalid message size %d", entry->body.zebra_message.size); + return 0; + } + int expected = entry->body.zebra_message.size - sizeof(marker) - sizeof(u_int16_t); mstream_t copy = mstream_copy(s, expected); @@ -1309,6 +1316,12 @@ void process_attr_aspath_string(struct aspath *as) { } /* Check AS length. */ + /* Check for integer overflow before multiplication */ + if (segment->length > 0 && as->asn_len > 0 && + segment->length > ((size_t)-1 - AS_HEADER_SIZE) / as->asn_len) { + aspath_error(as); + return; + } if ((pnt + (segment->length * as->asn_len) + AS_HEADER_SIZE) > end) { aspath_error(as); @@ -1357,19 +1370,27 @@ void process_attr_aspath_string(struct aspath *as) { int asn_pos = i * as->asn_len; switch(as->asn_len) { - case ASN16_LEN: - asn = ntohs (*(u_int16_t *) (segment->data + asn_pos)); + case ASN16_LEN: { + u_int16_t val; + memcpy(&val, segment->data + asn_pos, sizeof(val)); + asn = ntohs(val); break; - case ASN32_LEN: - asn = ntohl (*(u_int32_t *) (segment->data + asn_pos)); + } + case ASN32_LEN: { + u_int32_t val; + memcpy(&val, segment->data + asn_pos, sizeof(val)); + asn = ntohl(val); break; + } default: assert("invalid asn_len" && false); } - pos += int2str(asn, as->str + pos); - if(pos > MAX_ASPATH_LEN - 100) { - strcpy(as->str + pos, "..."); + pos += int2str(asn, as->str + pos, MAX_ASPATH_LEN - pos); + if(pos >= MAX_ASPATH_LEN - 4) { + if (pos < MAX_ASPATH_LEN - 1) { + strcpy(as->str + pos, "..."); + } return; }; } @@ -1466,11 +1487,11 @@ void process_attr_lcommunity_string(struct lcommunity *lcom) { memset (buf, 0, BUFSIZ); - for (i = 0; i < lcom->size; i++) + for (i = 0; i < lcom->size; i++) { - memcpy (&global, lcom->val + (i * 3), sizeof (u_int32_t)); - memcpy (&local1, lcom->val + (i * 3) + 1, sizeof (u_int32_t)); - memcpy (&local2, lcom->val + (i * 3) + 2, sizeof (u_int32_t)); + memcpy (&global, lcom->val + (i * 12), sizeof (u_int32_t)); + memcpy (&local1, lcom->val + (i * 12) + 4, sizeof (u_int32_t)); + memcpy (&local2, lcom->val + (i * 12) + 8, sizeof (u_int32_t)); global = ntohl (global); local1 = ntohl (local1); @@ -1794,12 +1815,14 @@ struct aspath *asn32_merge_paths(struct aspath *path, struct aspath *newpath) { } void asn32_expand_16_to_32(char *dst, char *src, int len) { - u_int32_t *dstarray = (u_int32_t *) dst; - u_int16_t *srcarray = (u_int16_t *) src; int i; for(i = 0; i < len; i++) { - dstarray[i] = htonl(ntohs(srcarray[i])); + u_int16_t src_val; + u_int32_t dst_val; + memcpy(&src_val, src + (i * sizeof(u_int16_t)), sizeof(src_val)); + dst_val = htonl(ntohs(src_val)); + memcpy(dst + (i * sizeof(u_int32_t)), &dst_val, sizeof(dst_val)); } } diff --git a/example.c b/example.c index 0633dad..b9f6979 100644 --- a/example.c +++ b/example.c @@ -218,7 +218,7 @@ if(entry->type == BGPDUMP_TYPE_ZEBRA_BGP && entry->subtype == BGPDUMP_SUBTYPE_ZE inet_ntop(AF_INET6, &e->entries[i].peer->peer_ip, peer_ip, INET6_ADDRSTRLEN); #endif } else { - sprintf(peer_ip, "N/A, unsupported AF"); + snprintf(peer_ip, sizeof(peer_ip), "N/A, unsupported AF"); } printf(" PEER IP : %s\n",peer_ip); printf(" PEER AS : %u\n",e->entries[i].peer->peer_as); diff --git a/inet_ntop.c b/inet_ntop.c index a05dca6..436e7aa 100644 --- a/inet_ntop.c +++ b/inet_ntop.c @@ -142,7 +142,8 @@ char *fmt_ipv6(BGPDUMP_IP_ADDRESS addr, char *buffer) char buffer2[100]; BGPDUMP_IP_ADDRESS mapped = { .v4_addr.s_addr = ((uint32_t *)addr.v6_addr.s6_addr)[3] }; - sprintf(buffer, "::%s%s", m ? "ffff:" : "", fmt_ipv4(mapped, buffer2)); + /* IPv4-mapped IPv6 address format: "::ffff:xxx.xxx.xxx.xxx" (max 24 bytes) */ + snprintf(buffer, BGPDUMP_ADDRSTRLEN, "::%s%s", m ? "ffff:" : "", fmt_ipv4(mapped, buffer2)); return buffer; } diff --git a/util.c b/util.c index d6db86c..17b2dd1 100644 --- a/util.c +++ b/util.c @@ -56,8 +56,8 @@ static void _log(int lvl, char *lvl_str, const char *fmt, va_list args) { if(use_syslog) { syslog(lvl, fmt, args); } else { - char prefix[strlen(fmt) + 1000]; - sprintf(prefix, "%s [%s] %s\n", now_str(), lvl_str, fmt); + char prefix[strlen(fmt) + 1100]; + snprintf(prefix, sizeof(prefix), "%s [%s] %s\n", now_str(), lvl_str, fmt); vfprintf(stderr, prefix, args); } } @@ -66,21 +66,23 @@ void err(const char *fmt, ...) { log(ERR, error); } void warn(const char *fmt, ...) { log(WARNING, warn); } void debug(const char *fmt, ...) { log(INFO, info); } -int time2str(struct tm* date,char *time_str) +int time2str(struct tm* date,char *time_str, size_t size) { - return sprintf(time_str, "%02d/%02d/%02d %02d:%02d:%02d", date->tm_mon+1, date->tm_mday, date->tm_year%100, + /* Format produces exactly 17 bytes: "MM/DD/YY HH:MM:SS" */ + return snprintf(time_str, size, "%02d/%02d/%02d %02d:%02d:%02d", date->tm_mon+1, date->tm_mday, date->tm_year%100, date->tm_hour, date->tm_min, date->tm_sec); } -int int2str(uint32_t value, char* str) +int int2str(uint32_t value, char* str, size_t size) { - return sprintf(str, "%u", value); + /* uint32_t max is 4294967295 (10 digits) */ + return snprintf(str, size, "%u", value); } static void ti2s(uint32_t value) { char buf[100], ref[100]; - sprintf(ref, "%u", value); - int len = int2str(value, buf); + snprintf(ref, sizeof(ref), "%u", value); + int len = int2str(value, buf, sizeof(buf)); printf("%s =?= %s (%i)\n", ref, buf, len); } diff --git a/util.h b/util.h index a8cf95b..fa8c20a 100644 --- a/util.h +++ b/util.h @@ -38,8 +38,8 @@ char *fmt_ipv4(BGPDUMP_IP_ADDRESS addr, char *buffer); char *fmt_ipv6(BGPDUMP_IP_ADDRESS addr, char *buffer); void test_fmt_ip(void); -int time2str(struct tm* date,char *time_str); -int int2str(uint32_t value, char* str); +int time2str(struct tm* date,char *time_str, size_t size); +int int2str(uint32_t value, char* str, size_t size); void test_utils(void); #endif