[BUG] Stack buffer overflow WRITE of size 1 in nfs_start function

[Neeraj Pal]

Hi Sascha,

Thank you for the patches.

I have confirmed it and observed no crashes as reported earlier but I
think there is a small typo in the nfs_start() function in
net/nfs.c#L677.

672    static int nfs_start(char *p)
673    {
674        debug("%s\n", __func__);
675
676        nfs_path = strdup(p);
677        if (nfs_path)
678            return -ENOMEM;
679

In line 677, if strdup is successful then it is returning ENOMEM so I
think there is a typo, it is supposed to check for NULL so it would be
if (!nfs_path) or if (nfs_path == NULL) then it should return ENOMEM.

Please confirm and also sending a small patch.

Thanks and regards,
Neeraj

On Fri, May 7, 2021 at 2:11 PM Sascha Hauer <sha at pengutronix.de> wrote:
>
> Hi,
>
> On Sun, Apr 18, 2021 at 12:22:30AM +0530, Neeraj Pal wrote:
> > Hi,
> >
> > While reviewing the code of barebox-2021.04.0 and git commit
> > af0f068a6edad45b033e772056ac0352e1ba3613  I found a stack buffer
> > overflow WRITE of size 1 in
> > nfs_start() net/nfs.c L664 through strcpy call which furthers crashes at
> > function strcpy in lib/string.c L96.
>
> Thanks for reporting this. Indeed the nfs filename is stored in a fixed
> size buffer which can easily overflow with the right input.
>
> This patch should fix this issue.
>
> Regards,
>   Sascha
>
> -----------------------------8<---------------------------------
> From 3978396bf88c4ab567ddf36dff1218502e32a94d Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer at pengutronix.de>
> Date: Fri, 7 May 2021 10:26:51 +0200
> Subject: [PATCH] nfs command: Fix possible buffer overflow
>
> the nfs command stores the nfs filename in a fixed size buffer without
> checking its length. Instead of using a static buffer use strdup() to
> dynamically allocate a suitably sized buffer.
>
> Reported-by: Neeraj Pal <neerajpal09 at gmail.com>
> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> ---
>  net/nfs.c | 41 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/net/nfs.c b/net/nfs.c
> index 591417e0de..440e410a83 100644
> --- a/net/nfs.c
> +++ b/net/nfs.c
> @@ -148,7 +148,6 @@ static int  nfs_state;
>
>  static char *nfs_filename;
>  static char *nfs_path;
> -static char nfs_path_buff[2048];
>
>  static int net_store_fd;
>  static struct net_connection *nfs_con;
> @@ -522,11 +521,26 @@ static int nfs_readlink_reply(unsigned char *pkt, unsigned len)
>         path = (char *)data;
>
>         if (*path != '/') {
> -               strcat(nfs_path, "/");
> -               strncat(nfs_path, path, rlen);
> +               char *n;
> +
> +               n = calloc(strlen(nfs_path) + sizeof('/') + rlen + 1, 1);
> +               if (!n)
> +                       return -ENOMEM;
> +
> +               strcpy(n, nfs_path);
> +               strcat(n, "/");
> +               strncat(n, path, rlen);
> +
> +               free(nfs_path);
> +               nfs_path = n;
>         } else {
> +               free(nfs_path);
> +
> +               nfs_path = calloc(rlen + 1, 1);
> +               if (!nfs_path)
> +                       return -ENOMEM;
> +
>                 memcpy(nfs_path, path, rlen);
> -               nfs_path[rlen] = 0;
>         }
>         return 0;
>  }
> @@ -655,13 +669,13 @@ err_out:
>         nfs_err = ret;
>  }
>
> -static void nfs_start(char *p)
> +static int nfs_start(char *p)
>  {
>         debug("%s\n", __func__);
>
> -       nfs_path = (char *)nfs_path_buff;
> -
> -       strcpy(nfs_path, p);
> +       nfs_path = strdup(p);
> +       if (nfs_path)
> +               return -ENOMEM;
>
>         nfs_filename = basename (nfs_path);
>         nfs_path     = dirname (nfs_path);
> @@ -671,6 +685,8 @@ static void nfs_start(char *p)
>         nfs_state = STATE_PRCLOOKUP_PROG_MOUNT_REQ;
>
>         nfs_send();
> +
> +       return 0;
>  }
>
>  static int do_nfs(int argc, char *argv[])
> @@ -701,9 +717,9 @@ static int do_nfs(int argc, char *argv[])
>         }
>         net_udp_bind(nfs_con, 1000);
>
> -       nfs_err = 0;
> -
> -       nfs_start(remotefile);
> +       nfs_err = nfs_start(remotefile);
> +       if (nfs_err)
> +               goto err_udp;
>
>         while (nfs_state != STATE_DONE) {
>                 if (ctrlc()) {
> @@ -727,6 +743,9 @@ err_udp:
>
>         printf("\n");
>
> +       free(nfs_path);
> +       nfs_path = NULL;
> +
>         return nfs_err == 0 ? 0 : 1;
>  }
>
> --
> 2.29.2
>
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |