Skip to content

Commit

Permalink
maybe_escape_markup: Make function memory-safe (#526)
Browse files Browse the repository at this point in the history 
* 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 &<<<<<<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 &amp;&lt;&lt;&lt;&lt;&lt;&lt;hello world&gt;&gt;, 1,2009 Gb/s) 192.168.26.237
```
and
```
W: ( 73% at Hello world &<<<<<<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)

* Revert to snprintf
  • Loading branch information
@orestisfl
orestisfl authored May 8, 2024
1 parent c07b9ca commit 1039768
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 19 deletions.
2 changes: 1 addition & 1 deletion include/i3status.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
void maybe_escape_markup(char *text, char *buffer, size_t size);

char *rtrim(const char *s);
char *ltrim(const char *s);
Expand Down
29 changes: 18 additions & 11 deletions src/output.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,35 +88,42 @@ 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) {
void maybe_escape_markup(char *text, char *buffer, size_t size) {
size--; /* Leave a byte for NUL termination. */

if (markup_format == M_NONE) {
*buffer += sprintf(*buffer, "%s", text);
*buffer += snprintf(buffer, size, "%s", text);
return;
}
for (; *text != '\0'; text++) {

for (size_t i = 0; *text != '\0'; text++) {
if (i >= size) {
return;
}
switch (*text) {
case '&':
*buffer += sprintf(*buffer, "%s", "&amp;");
i += snprintf(&buffer[i], size - i, "%s", "&amp;");
break;
case '<':
*buffer += sprintf(*buffer, "%s", "&lt;");
i += snprintf(&buffer[i], size - i, "%s", "&lt;");
break;
case '>':
*buffer += sprintf(*buffer, "%s", "&gt;");
i += snprintf(&buffer[i], size - i, "%s", "&gt;");
break;
case '\'':
*buffer += sprintf(*buffer, "%s", "&apos;");
i += snprintf(&buffer[i], size - i, "%s", "&apos;");
break;
case '"':
*buffer += sprintf(*buffer, "%s", "&quot;");
i += snprintf(&buffer[i], size - i, "%s", "&quot;");
break;
default:
if ((0x1 <= *text && *text <= 0x8) ||
(0xb <= *text && *text <= 0xc) ||
(0xe <= *text && *text <= 0x1f)) {
*buffer += sprintf(*buffer, "&#x%x;", *text);
i += snprintf(&buffer[i], size - i, "&#x%x;", *text);
} else {
*(*buffer)++ = *text;
buffer[i] = *text;
i++;
}
break;
}
Expand Down Expand Up @@ -154,4 +161,4 @@ char *trim(const char *s) {
char *f = ltrim(r);
free(r);
return f;
}
}
13 changes: 6 additions & 7 deletions src/print_wireless_info.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@

#include "i3status.h"

#define STRING_SIZE 30
#define STRING_SIZE 60

#define WIRELESS_INFO_FLAG_HAS_ESSID (1 << 0)
#define WIRELESS_INFO_FLAG_HAS_QUALITY (1 << 1)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -601,10 +601,9 @@ void print_wireless_info(wireless_info_ctx_t *ctx) {
snprintf(string_noise, STRING_SIZE, "?");
}

char *tmp = string_essid;
#ifdef IW_ESSID_MAX_SIZE
if (info.flags & WIRELESS_INFO_FLAG_HAS_ESSID)
maybe_escape_markup(info.essid, &tmp);
maybe_escape_markup(info.essid, string_essid, STRING_SIZE);
else
#endif
snprintf(string_essid, STRING_SIZE, "?");
Expand Down

0 comments on commit 1039768

Please sign in to comment.