From: Neeraj Pal <neerajpal09@gmail.com>
To: Sascha Hauer <sha@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [BUG] Stack buffer overflow WRITE of size 1 in nfs_start function
Date: Mon, 10 May 2021 16:38:51 +0530 [thread overview]
Message-ID: <CANi4_RWKvQV68RaZLNBFTJP1z3vObuZYo-HRqfU6r4rL8ZxKoQ@mail.gmail.com> (raw)
In-Reply-To: <20210507084102.GU19819@pengutronix.de>
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@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@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@gmail.com>
> Signed-off-by: Sascha Hauer <s.hauer@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 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
next prev parent reply other threads:[~2021-05-10 11:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-17 18:52 Neeraj Pal
2021-05-07 8:41 ` Sascha Hauer
2021-05-10 11:08 ` Neeraj Pal [this message]
2021-05-10 13:18 ` Neeraj Pal
2021-05-11 8:58 ` Sascha Hauer
2021-05-11 18:06 ` Neeraj Pal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANi4_RWKvQV68RaZLNBFTJP1z3vObuZYo-HRqfU6r4rL8ZxKoQ@mail.gmail.com \
--to=neerajpal09@gmail.com \
--cc=barebox@lists.infradead.org \
--cc=sha@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox