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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
> 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 |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
