Re: [PATCH bluez v2] monitor: fix buffer overflow when terminal width > 255

[Celeste Liu <coelacanthushex@gmail.com>]

On 2024-09-17 00:22, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Mon, Sep 16, 2024 at 11:57 AM Celeste Liu <coelacanthushex@gmail.com> wrote:
>>
>>
>> On 2024-09-16 23:39, Celeste Liu wrote:
>>>
>>> On 2024-09-16 23:10, Luiz Augusto von Dentz wrote:
>>>> Hi Celeste,
>>>>
>>>> On Sat, Sep 14, 2024 at 12:10 PM Celeste Liu <coelacanthushex@gmail.com> wrote:
>>>>>
>>>>> In current code, we create line buffer with size 256, which can contains
>>>>> 255 ASCII characters. But in modern system, terminal can have larger
>>>>> width. It may cause buffer overflow in snprintf() text.
>>>>>
>>>>> We need allocate line buffer with size which can contains one line in
>>>>> terminal. The size should be difficult to calculate because of multibyte
>>>>> characters, but our code using line buffer assumed all characters has
>>>>> 1 byte size (e.g. when we put packet text into line buffer via
>>>>> snprintf(), we calculate max size by 1B * col.), so it's safe to
>>>>> allocate line buffer with col + 1.
>>>>>
>>>>> Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Add free() forgot in v1.
>>>>> - Link to v1: https://patch.msgid.link/20240914-fix-log-buffer-overflow-v1-1-733cb4fff673@gmail.com
>>>>> ---
>>>>>  monitor/packet.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/monitor/packet.c b/monitor/packet.c
>>>>> index c2599fe6864ab44d657c121fcc3ceecc1ebc52a6..bef55477a221b6cb43ff224454ac3fa593cd8221 100644
>>>>> --- a/monitor/packet.c
>>>>> +++ b/monitor/packet.c
>>>>> @@ -376,7 +376,8 @@ static void print_packet(struct timeval *tv, struct ucred *cred, char ident,
>>>>>                                         const char *text, const char *extra)
>>>>>  {
>>>>>         int col = num_columns();
>>>>> -       char line[256], ts_str[96], pid_str[140];
>>>>> +       char ts_str[96], pid_str[140];
>>>>> +       char *line = (char *) malloc(sizeof(char) * col + 1);
>>>>
>>>> Perhaps we could replace malloc with alloca here so we allocate the
>>>> line on the heap rather than stack.
>>>
>>> I will replace it with alloca() in the next version.
>>> But to be honest, I think alloca() is not a good choice. The compiler will
>>> prevent the functions that call alloca() be inline, otherwise it may trigger
>>> unexpected stack overflow because it's not a scope-based lifetime. It may be
>>> better to replace it with VLA once we bump the standard requirement to C99 or
>>> above.
>>
>> But I found a VLA usage in monitor/display.h:82, which was introduced by Marcel Holtmann
>> in d9e3aab39d2af7d7a822993ededaa41cd0311c53 in 2012. Could we use VLA directly? Or we
>> need to treat that usage as a bug and fix it?
> 
> Afaik VLA has more problems than using alloca, because depending on
> the C (11-23) version VLA is optional, so afaik alloca is a safer
> option provided the length is not considered too big, anyway perhaps
> we should use some define like LINE_MAX instead:
> 
>        {LINE_MAX}
>              Unless otherwise noted, the maximum length, in bytes, of a
>              utility's input line (either standard input or another
>              file), when the utility is described as processing text
>              files. The length includes room for the trailing <newline>.
>              Minimum Acceptable Value: {_POSIX2_LINE_MAX}
> https://www.man7.org/linux/man-pages/man0/limits.h.0p.html

v3 has been sent.

> So something like the following:
> 
> diff --git a/monitor/packet.c b/monitor/packet.c
> index c2599fe6864a..32a440bbea68 100644
> --- a/monitor/packet.c
> +++ b/monitor/packet.c
> @@ -26,6 +26,7 @@
>  #include <time.h>
>  #include <sys/time.h>
>  #include <sys/socket.h>
> +#include <limits.h>
> 
>  #include "lib/bluetooth.h"
>  #include "lib/uuid.h"
> @@ -376,7 +377,7 @@ static void print_packet(struct timeval *tv,
> struct ucred *cred, char ident,
>                                         const char *text, const char *extra)
>  {
>         int col = num_columns();
> -       char line[256], ts_str[96], pid_str[140];
> +       char line[LINE_MAX], ts_str[96], pid_str[140];
>         int n, ts_len = 0, ts_pos = 0, len = 0, pos = 0;
>         static size_t last_frame;
> 
>>>
>>>>
>>>>>         int n, ts_len = 0, ts_pos = 0, len = 0, pos = 0;
>>>>>         static size_t last_frame;
>>>>>
>>>>> @@ -525,6 +526,7 @@ static void print_packet(struct timeval *tv, struct ucred *cred, char ident,
>>>>>                 printf("%s%s\n", use_color() ? COLOR_TIMESTAMP : "", ts_str);
>>>>>         } else
>>>>>                 printf("%s\n", line);
>>>>> +       free(line);
>>>>>  }
>>>>>
>>>>>  static const struct {
>>>>>
>>>>> ---
>>>>> base-commit: 41f943630d9a03c40e95057b2ac3d96470b9c71e
>>>>> change-id: 20240914-fix-log-buffer-overflow-9aa5e61ee5b8
>>>>>
>>>>> Best regards,
>>>>> --
>>>>> Celeste Liu <CoelacanthusHex@gmail.com>
>>>>>
>>>>>
>>>>
>>>>
>>
> 
>