From 6efb409b5377b8ef44a4a7a1c1599c7c5243fe7a Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Sun, 5 May 2024 10:22:36 +0200 Subject: [PATCH] maybe_escape_markup: Make function memory-safe This fixes #492 and an additional buffer overflow that can happen when pango markup is enabled. Using config ``` general { output_format = "none" markup = "pango" } order += "wireless _first_" wireless _first_ { format_up = "W: (%quality at %essid, %bitrate) %ip" } ``` and renaming my phone's hotspot to `Hello world &<<<<<>` i3status will throw an AddressSanitizer error: ``` ==1373240==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7411d720923e at pc 0x7411daa7cee9 bp 0x7ffdae6ce070 sp 0x7ffdae6cd800 WRITE of size 5 at 0x7411d720923e thread T0 #0 0x7411daa7cee8 in __interceptor_vsprintf /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:1765 #1 0x7411daa7d0ff in __interceptor_sprintf /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:1808 #2 0x653b2764cdaf in maybe_escape_markup ../src/output.c:102 #3 0x653b27677df9 in print_wireless_info ../src/print_wireless_info.c:607 #4 0x653b27640bf1 in main ../i3status.c:709 #5 0x7411da641ccf (/usr/lib/libc.so.6+0x25ccf) (BuildId: 6542915cee3354fbcf2b3ac5542201faec43b5c9) #6 0x7411da641d89 in __libc_start_main (/usr/lib/libc.so.6+0x25d89) (BuildId: 6542915cee3354fbcf2b3ac5542201faec43b5c9) #7 0x653b27633f24 in _start (/tmp/xx/i3status/build/i3status+0x4ff24) (BuildId: c737ce6288265fa02a7617c66f51ddd16b5a8275) Address 0x7411d720923e is located in stack of thread T0 at offset 574 in frame #0 0x653b276750ed in print_wireless_info ../src/print_wireless_info.c:513 This frame has 10 object(s): [48, 56) 'tmp' (line 604) [80, 168) 'info' (line 516) [208, 320) 'placeholders' (line 623) [352, 382) 'string_quality' (line 569) [416, 446) 'string_signal' (line 570) [480, 510) 'string_noise' (line 571) [544, 574) 'string_essid' (line 572) <== Memory access at offset 574 overflows this variable [608, 638) 'string_frequency' (line 573) [672, 702) 'string_ip' (line 574) [736, 766) 'string_bitrate' (line 575) ``` With pango disabled, the error is thrown elsewhere (#492): ``` ==1366779==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7bab43a0923e at pc 0x7bab4727cee9 bp 0x7ffc289d2540 sp 0x7ffc289d1cd0 WRITE of size 33 at 0x7bab43a0923e thread T0 #0 0x7bab4727cee8 in __interceptor_vsprintf /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:1765 #1 0x7bab4727d0ff in __interceptor_sprintf /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:1808 #2 0x5dd180858aa4 in maybe_escape_markup ../src/output.c:93 #3 0x5dd180883df9 in print_wireless_info ../src/print_wireless_info.c:607 #4 0x5dd18084cbf1 in main ../i3status.c:709 #5 0x7bab46843ccf (/usr/lib/libc.so.6+0x25ccf) (BuildId: 6542915cee3354fbcf2b3ac5542201faec43b5c9) #6 0x7bab46843d89 in __libc_start_main (/usr/lib/libc.so.6+0x25d89) (BuildId: 6542915cee3354fbcf2b3ac5542201faec43b5c9) #7 0x5dd18083ff24 in _start (/tmp/xx/i3status/build/i3status+0x4ff24) (BuildId: c737ce6288265fa02a7617c66f51ddd16b5a8275) Address 0x7bab43a0923e is located in stack of thread T0 at offset 574 in frame #0 0x5dd1808810ed in print_wireless_info ../src/print_wireless_info.c:513 This frame has 10 object(s): [48, 56) 'tmp' (line 604) [80, 168) 'info' (line 516) [208, 320) 'placeholders' (line 623) [352, 382) 'string_quality' (line 569) [416, 446) 'string_signal' (line 570) [480, 510) 'string_noise' (line 571) [544, 574) 'string_essid' (line 572) <== Memory access at offset 574 overflows this variable [608, 638) 'string_frequency' (line 573) [672, 702) 'string_ip' (line 574) [736, 766) 'string_bitrate' (line 575) ``` With the patch output is correct: ``` W: ( 72% at Hello world &<<<<<<hello world>>, 1,2009 Gb/s) 192.168.26.237 ``` and ``` W: ( 73% at Hello world &<<<<<>, 1,1342 Gb/s) 192.168.26.237 ``` The patch changes the maybe_escape_markup function to use dynamic allocation instead of a static buffer. Confusing pointer arithmetic is replaced with index-based memory access. The `buffer` pointer does not move around except for `realloc`ations. Fixes #492 Closes #525 (alternative PR) --- include/i3status.h | 2 +- src/output.c | 32 +++++++++++++++++++++----------- src/print_wireless_info.c | 20 ++++++++++---------- 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/include/i3status.h b/include/i3status.h index 1bc2c85f..1c75eaa0 100644 --- a/include/i3status.h +++ b/include/i3status.h @@ -204,7 +204,7 @@ void print_separator(const char *separator); char *color(const char *colorstr); char *endcolor() __attribute__((pure)); void reset_cursor(void); -void maybe_escape_markup(char *text, char **buffer); +char *maybe_escape_markup(char *text); char *rtrim(const char *s); char *ltrim(const char *s); diff --git a/src/output.c b/src/output.c index 9a180497..d8c6a25b 100644 --- a/src/output.c +++ b/src/output.c @@ -88,39 +88,49 @@ void reset_cursor(void) { * https://git.gnome.org/browse/glib/tree/glib/gmarkup.c?id=03db1f455b4265654e237d2ad55464b4113cba8a#n2142 * */ -void maybe_escape_markup(char *text, char **buffer) { +char *maybe_escape_markup(char *text) { if (markup_format == M_NONE) { - *buffer += sprintf(*buffer, "%s", text); - return; + return strdup(text); } + + size_t idx = 0; + size_t size = 32; + char *buffer = malloc(size); for (; *text != '\0'; text++) { + if (idx + 10 > size) { + size *= 2; + buffer = realloc(buffer, size); + } switch (*text) { case '&': - *buffer += sprintf(*buffer, "%s", "&"); + idx += sprintf(&buffer[idx], "%s", "&"); break; case '<': - *buffer += sprintf(*buffer, "%s", "<"); + idx += sprintf(&buffer[idx], "%s", "<"); break; case '>': - *buffer += sprintf(*buffer, "%s", ">"); + idx += sprintf(&buffer[idx], "%s", ">"); break; case '\'': - *buffer += sprintf(*buffer, "%s", "'"); + idx += sprintf(&buffer[idx], "%s", "'"); break; case '"': - *buffer += sprintf(*buffer, "%s", """); + idx += sprintf(&buffer[idx], "%s", """); break; default: if ((0x1 <= *text && *text <= 0x8) || (0xb <= *text && *text <= 0xc) || (0xe <= *text && *text <= 0x1f)) { - *buffer += sprintf(*buffer, "&#x%x;", *text); + idx += sprintf(&buffer[idx], "&#x%x;", *text); } else { - *(*buffer)++ = *text; + buffer[idx] = *text; + idx++; } break; } } + buffer[idx] = 0; + return buffer; } /* @@ -154,4 +164,4 @@ char *trim(const char *s) { char *f = ltrim(r); free(r); return f; -} \ No newline at end of file +} diff --git a/src/print_wireless_info.c b/src/print_wireless_info.c index 7c577f50..cd8f494e 100644 --- a/src/print_wireless_info.c +++ b/src/print_wireless_info.c @@ -402,9 +402,9 @@ static int get_wireless_info(const char *interface, wireless_info_t *info) { close(s); if (na.i_len >= sizeof(u.req)) { /* - * Just use the first BSSID returned even if there are - * multiple APs sharing the same BSSID. - */ + * Just use the first BSSID returned even if there are + * multiple APs sharing the same BSSID. + */ info->signal_level = u.req.info[0].isi_rssi / 2 + u.req.info[0].isi_noise; info->flags |= WIRELESS_INFO_FLAG_HAS_SIGNAL; @@ -523,7 +523,7 @@ void print_wireless_info(wireless_info_ctx_t *ctx) { /* * Removing '%' and following characters from IPv6 since the interface identifier is redundant, * as the output already includes the interface name. - */ + */ if (ipv6_address != NULL) { char *prct_ptr = strstr(ipv6_address, "%"); if (prct_ptr != NULL) { @@ -569,7 +569,6 @@ void print_wireless_info(wireless_info_ctx_t *ctx) { char string_quality[STRING_SIZE] = {'\0'}; char string_signal[STRING_SIZE] = {'\0'}; char string_noise[STRING_SIZE] = {'\0'}; - char string_essid[STRING_SIZE] = {'\0'}; char string_frequency[STRING_SIZE] = {'\0'}; char string_ip[STRING_SIZE] = {'\0'}; char string_bitrate[STRING_SIZE] = {'\0'}; @@ -601,13 +600,13 @@ void print_wireless_info(wireless_info_ctx_t *ctx) { snprintf(string_noise, STRING_SIZE, "?"); } - char *tmp = string_essid; + char *string_essid_tmp = NULL; /* Dynamic allocation of ESSID */ + char *string_essid = "?"; #ifdef IW_ESSID_MAX_SIZE - if (info.flags & WIRELESS_INFO_FLAG_HAS_ESSID) - maybe_escape_markup(info.essid, &tmp); - else + if (info.flags & WIRELESS_INFO_FLAG_HAS_ESSID) { + string_essid = string_essid_tmp = maybe_escape_markup(info.essid); + } #endif - snprintf(string_essid, STRING_SIZE, "?"); if (info.flags & WIRELESS_INFO_FLAG_HAS_FREQUENCY) snprintf(string_frequency, STRING_SIZE, "%1.1f GHz", info.frequency / 1e9); @@ -633,6 +632,7 @@ void print_wireless_info(wireless_info_ctx_t *ctx) { char *formatted = format_placeholders(walk, &placeholders[0], num); OUTPUT_FORMATTED; free(formatted); + free(string_essid_tmp); END_COLOR; free(ipv4_address);