mbox series

[BlueZ,00/12] Fix a number of static analysis issues #5

Message ID 20240704102617.1132337-1-hadess@hadess.net
Headers show
Series Fix a number of static analysis issues #5 | expand

Message

Bastien Nocera July 4, 2024, 10:24 a.m. UTC 
Bastien Nocera (12):
  gatt-server: Don't allocate negative data
  shared/shell: Free w.we_wordv on early function exit
  shared/shell: Free memory allocated by wordexp()
  shared/shell: Fix fd leak if -s is passed multiple times
  btsnoop: Fix possible negative memcpy length
  sdp: Fix possible null dereference
  sdp: Fix mismatched int casting
  emulator: Fix integer truncation warnings
  gatt-server: Fix integer overflow due to cast operation
  mesh: Fix integer overflow due to cast operation
  tools/mesh: Fix integer overflow due to cast operation
  unit/ringbuf: Fix ineffective guard due to signedness

 emulator/amp.c           |  3 ++-
 emulator/bthost.c        |  8 +++++---
 lib/sdp.c                |  6 +++++-
 mesh/pb-adv.c            |  2 +-
 src/shared/gatt-server.c |  8 ++++----
 src/shared/ringbuf.c     |  2 +-
 src/shared/shell.c       | 20 +++++++++++++++-----
 tools/btsnoop.c          |  4 ++--
 tools/mesh/mesh-db.c     |  6 ++----
 9 files changed, 37 insertions(+), 22 deletions(-)

Comments

Bastien Nocera July 4, 2024, 2:03 p.m. UTC | #1 
On Thu, 2024-07-04 at 15:25 +0300, Pauli Virtanen wrote:
> Hi,
> 
> to, 2024-07-04 kello 12:24 +0200, Bastien Nocera kirjoitti:
> > Set a lower-bound to the data MTU to avoid allocating -1 elements
> > if
> > bt_att_get_mtu() returns zero.
> > 
> > Error: OVERRUN (CWE-119): [#def36] [important]
> > bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function
> > call "bt_att_get_mtu(server->att)" returns 0.
> > bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning:
> > "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data-
> > >mtu" is now 0.
> > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning:
> > "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now
> > 18446744073709551615.
> > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning:
> > "__s" = "1UL".
> > bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg:
> > Calling "memset" with "__p" and "__n * __s" is suspicious because
> > of the very large index, 18446744073709551615. The index may be due
> > to a negative parameter being interpreted as unsigned. [Note: The
> > source code implementation of the function has been overridden by a
> > builtin model.]
> > 1121|		data->mtu = bt_att_get_mtu(server->att);
> > 1122|		data->length = 0;
> > 1123|->		data->rsp_data = new0(uint8_t, data->mtu - 1);
> > 1124|
> > 1125|		return data;
> > ---
> >  src/shared/gatt-server.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> > index 3a53d5dfde6b..c587553d655d 100644
> > --- a/src/shared/gatt-server.c
> > +++ b/src/shared/gatt-server.c
> > @@ -1118,7 +1118,7 @@ static struct read_mult_data
> > *read_mult_data_new(struct bt_gatt_server *server,
> >  	data->server = server;
> >  	data->num_handles = num_handles;
> >  	data->cur_handle = 0;
> > -	data->mtu = bt_att_get_mtu(server->att);
> > +	data->mtu = MAX(bt_att_get_mtu(server->att),
> > BT_ATT_DEFAULT_LE_MTU);
> 
> Is this correct, probably MTU less than default are valid?

This is the same code as in bt_gatt_server_new().

> 
> >  	data->length = 0;
> >  	data->rsp_data = new0(uint8_t, data->mtu - 1);
> >  
> 
> Might be better to instead: MAX(data->mtu, 1) - 1

I'd be fine with either, if somebody knows that particular part of the
code better than I do...
Luiz Augusto von Dentz July 5, 2024, 1:26 a.m. UTC | #2 
Hi Bastien,

On Thu, Jul 4, 2024 at 6:33 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> Error: RESOURCE_LEAK (CWE-772): [#def40] [important]
> bluez-5.76/src/shared/shell.c:1113:3: alloc_arg: "wordexp" allocates memory that is stored into "w.we_wordv".
> bluez-5.76/src/shared/shell.c:1114:4: leaked_storage: Variable "w" going out of scope leaks the storage "w.we_wordv" points to.
> 1112|
> 1113|                   if (wordexp(rl_line_buffer, &w, WRDE_NOCMD))
> 1114|->                         return NULL;

Derr, this is NOCMD has been found...

> 1115|
> 1116|                   matches = menu_completion(default_menu, text, w.we_wordc,
>
> Error: RESOURCE_LEAK (CWE-772): [#def42] [important]
> bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp" allocates memory that is stored into "w.we_wordv".
> bluez-5.76/src/shared/shell.c:1415:3: leaked_storage: Variable "w" going out of scope leaks the storage "w.we_wordv" points to.
> 1413|           switch (err) {
> 1414|           case WRDE_BADCHAR:
> 1415|->                 return -EBADMSG;
> 1416|           case WRDE_BADVAL:
> 1417|           case WRDE_SYNTAX:

Ok, but where in the documentation of wordexp it is saying that
we_wordv is left with anything allocated if it fails? Ive assumed if
it returns an error no argument has been processed, otherwise this is
sort of misleading and the errors shall be returned by index of the
word.

> Error: RESOURCE_LEAK (CWE-772): [#def43] [important]
> bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp" allocates memory that is stored into "w.we_wordv".
> bluez-5.76/src/shared/shell.c:1418:3: leaked_storage: Variable "w" going out of scope leaks the storage "w.we_wordv" points to.
> 1416|           case WRDE_BADVAL:
> 1417|           case WRDE_SYNTAX:
> 1418|->                 return -EINVAL;
> 1419|           case WRDE_NOSPACE:
> 1420|                   return -ENOMEM;
>
> Error: RESOURCE_LEAK (CWE-772): [#def44] [important]
> bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp" allocates memory that is stored into "w.we_wordv".
> bluez-5.76/src/shared/shell.c:1420:3: leaked_storage: Variable "w" going out of scope leaks the storage "w.we_wordv" points to.
> 1418|                   return -EINVAL;
> 1419|           case WRDE_NOSPACE:
> 1420|->                 return -ENOMEM;
> 1421|           case WRDE_CMDSUB:
> 1422|                   if (wordexp(input, &w, 0))
>
> Error: RESOURCE_LEAK (CWE-772): [#def45] [important]
> bluez-5.76/src/shared/shell.c:1422:3: alloc_arg: "wordexp" allocates memory that is stored into "w.we_wordv".
> bluez-5.76/src/shared/shell.c:1423:4: leaked_storage: Variable "w" going out of scope leaks the storage "w.we_wordv" points to.
> 1421|           case WRDE_CMDSUB:
> 1422|                   if (wordexp(input, &w, 0))
> 1423|->                         return -ENOEXEC;
> 1424|                   break;
> 1425|           };
> ---
>  src/shared/shell.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/shared/shell.c b/src/shared/shell.c
> index 878be140c336..c09d41ee54df 100644
> --- a/src/shared/shell.c
> +++ b/src/shared/shell.c
> @@ -1117,8 +1117,10 @@ static char **shell_completion(const char *text, int start, int end)
>         if (start > 0) {
>                 wordexp_t w;
>
> -               if (wordexp(rl_line_buffer, &w, WRDE_NOCMD))
> +               if (wordexp(rl_line_buffer, &w, WRDE_NOCMD)) {
> +                       wordfree(&w);
>                         return NULL;
> +               }
>
>                 matches = menu_completion(default_menu, text, w.we_wordc,
>                                                         w.we_wordv[0]);
> @@ -1421,15 +1423,20 @@ int bt_shell_exec(const char *input)
>         err = wordexp(input, &w, WRDE_NOCMD);
>         switch (err) {
>         case WRDE_BADCHAR:
> +               wordfree(&w);
>                 return -EBADMSG;
>         case WRDE_BADVAL:
>         case WRDE_SYNTAX:
> +               wordfree(&w);
>                 return -EINVAL;
>         case WRDE_NOSPACE:
> +               wordfree(&w);
>                 return -ENOMEM;
>         case WRDE_CMDSUB:
> -               if (wordexp(input, &w, 0))
> +               if (wordexp(input, &w, 0)) {
> +                       wordfree(&w);
>                         return -ENOEXEC;
> +               }
>                 break;
>         };

If we really need to call wordfree regardless then I'd probably have a
function that wraps wordexp and automatically does wordfree on error.
Bastien Nocera July 5, 2024, 6:48 a.m. UTC | #3 
On Thu, 2024-07-04 at 21:26 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Thu, Jul 4, 2024 at 6:33 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > Error: RESOURCE_LEAK (CWE-772): [#def40] [important]
> > bluez-5.76/src/shared/shell.c:1113:3: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1114:4: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1112|
> > 1113|                   if (wordexp(rl_line_buffer, &w,
> > WRDE_NOCMD))
> > 1114|->                         return NULL;
> 
> Derr, this is NOCMD has been found...

Still needs parsing *shrug*

> 
> > 1115|
> > 1116|                   matches = menu_completion(default_menu,
> > text, w.we_wordc,
> > 
> > Error: RESOURCE_LEAK (CWE-772): [#def42] [important]
> > bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1415:3: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1413|           switch (err) {
> > 1414|           case WRDE_BADCHAR:
> > 1415|->                 return -EBADMSG;
> > 1416|           case WRDE_BADVAL:
> > 1417|           case WRDE_SYNTAX:
> 
> Ok, but where in the documentation of wordexp it is saying that
> we_wordv is left with anything allocated if it fails? Ive assumed if
> it returns an error no argument has been processed, otherwise this is
> sort of misleading and the errors shall be returned by index of the
> word.

There's nothing says it frees wordv on error, and the code shows it
doesn't:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/wordexp.c;h=a7362ef31b05280001e961c3a630e953110b7397;hb=HEAD#l2203

> > Error: RESOURCE_LEAK (CWE-772): [#def43] [important]
> > bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1418:3: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1416|           case WRDE_BADVAL:
> > 1417|           case WRDE_SYNTAX:
> > 1418|->                 return -EINVAL;
> > 1419|           case WRDE_NOSPACE:
> > 1420|                   return -ENOMEM;
> > 
> > Error: RESOURCE_LEAK (CWE-772): [#def44] [important]
> > bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1420:3: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1418|                   return -EINVAL;
> > 1419|           case WRDE_NOSPACE:
> > 1420|->                 return -ENOMEM;
> > 1421|           case WRDE_CMDSUB:
> > 1422|                   if (wordexp(input, &w, 0))
> > 
> > Error: RESOURCE_LEAK (CWE-772): [#def45] [important]
> > bluez-5.76/src/shared/shell.c:1422:3: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1423:4: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1421|           case WRDE_CMDSUB:
> > 1422|                   if (wordexp(input, &w, 0))
> > 1423|->                         return -ENOEXEC;
> > 1424|                   break;
> > 1425|           };
> > ---
> >  src/shared/shell.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/shared/shell.c b/src/shared/shell.c
> > index 878be140c336..c09d41ee54df 100644
> > --- a/src/shared/shell.c
> > +++ b/src/shared/shell.c
> > @@ -1117,8 +1117,10 @@ static char **shell_completion(const char
> > *text, int start, int end)
> >         if (start > 0) {
> >                 wordexp_t w;
> > 
> > -               if (wordexp(rl_line_buffer, &w, WRDE_NOCMD))
> > +               if (wordexp(rl_line_buffer, &w, WRDE_NOCMD)) {
> > +                       wordfree(&w);
> >                         return NULL;
> > +               }
> > 
> >                 matches = menu_completion(default_menu, text,
> > w.we_wordc,
> >                                                        
> > w.we_wordv[0]);
> > @@ -1421,15 +1423,20 @@ int bt_shell_exec(const char *input)
> >         err = wordexp(input, &w, WRDE_NOCMD);
> >         switch (err) {
> >         case WRDE_BADCHAR:
> > +               wordfree(&w);
> >                 return -EBADMSG;
> >         case WRDE_BADVAL:
> >         case WRDE_SYNTAX:
> > +               wordfree(&w);
> >                 return -EINVAL;
> >         case WRDE_NOSPACE:
> > +               wordfree(&w);
> >                 return -ENOMEM;
> >         case WRDE_CMDSUB:
> > -               if (wordexp(input, &w, 0))
> > +               if (wordexp(input, &w, 0)) {
> > +                       wordfree(&w);
> >                         return -ENOEXEC;
> > +               }
> >                 break;
> >         };
> 
> If we really need to call wordfree regardless then I'd probably have
> a
> function that wraps wordexp and automatically does wordfree on error.

OK.

>