Skip to content

Various minor code improvements #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaredmauch wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Various minor code improvements #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <jared@puck.nether.net>

* 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 <colin@spakka.net>
* Include CPPFLAGS

Expand Down
52 changes: 27 additions & 25 deletions bgpdump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 ");
}
Expand All @@ -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");
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;idx<count;idx++)
{
Expand Down Expand Up @@ -1629,9 +1631,9 @@ static void table_line_announce_1(struct mp_nlri *prefix,int count,BGPDUMP_ENTRY
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;idx<count;idx++)
{
Expand Down Expand Up @@ -1792,9 +1794,9 @@ static void table_line_announce6(struct mp_nlri *prefix,int count,BGPDUMP_ENTRY
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;idx<count;idx++)
{
Expand Down Expand Up @@ -1908,13 +1910,13 @@ static void table_line_mrtd_route(BGPDUMP_MRTD_TABLE_DUMP *route,BGPDUMP_ENTRY *
char aggregate[20];
unsigned int npref;
unsigned int nmed;
char time_str[20];
char time_str[32];
char peer[BGPDUMP_ADDRSTRLEN], prefix[BGPDUMP_ADDRSTRLEN], nexthop[BGPDUMP_ADDRSTRLEN];

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");

time_t *t;
if (timetype==0) {
Expand All @@ -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");

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
53 changes: 38 additions & 15 deletions bgpdump_lib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]");
Expand Down Expand Up @@ -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;
}


Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
};
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
}
}

Expand Down
2 changes: 1 addition & 1 deletion example.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion inet_ntop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
18 changes: 10 additions & 8 deletions util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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