[BUG] Stack buffer overflow WRITE of size 1 in nfs_start function
Neeraj Pal
neerajpal09 at gmail.com
Mon May 10 06:18:20 PDT 2021
Hi Sascha,
to confirm the suggestion that I have mentioned in the previous mail
regarding the check on strdup return value, I have modified the
condition to "if (!nfs_path)" then compiled the sandbox and observed
the following crash with the mentioned input, after configuring the
network and nfs-server on the host:
barebox at Sandbox:/ nfs hh h
Filename './hh'.
NFS failed: Permission denied
=================================================================
==2828613==ERROR: AddressSanitizer: global-buffer-overflow on address
0x000000955ea0 at pc 0x000000422a2c bp 0x7ffec7bc9670 sp
0x7ffec7bc9660
READ of size 8 at 0x000000955ea0 thread T0
#0 0x422a2b in barebox_free common/dlmalloc.c:1397
#1 0x522fdf in do_nfs net/nfs.c:746
#2 0x419e85 in execute_command common/command.c:62
#3 0x434342 in run_pipe_real common/hush.c:837
#4 0x434342 in run_list_real common/hush.c:961
#5 0x434342 in run_list_real common/hush.c:849
#6 0x431dc7 in run_list common/hush.c:1078
#7 0x431dc7 in parse_stream_outer common/hush.c:1705
#8 0x434fab in run_shell common/hush.c:1928
#9 0x40a2ac in run_init common/startup.c:378
#10 0x40a385 in start_barebox common/startup.c:421
#11 0x590e63 in main (/home/bsdboy/barebox-9may/barebox/barebox+0x590e63)
#12 0x7fdaf187f0b2 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
#13 0x4061bd in _start (/home/bsdboy/barebox-9may/barebox/barebox+0x4061bd)
0x000000955ea2 is located 0 bytes to the right of global variable
'str' defined in 'lib/libgen.c:51:14' (0x955ea0) of size 2
'str' is ascii string '.'
SUMMARY: AddressSanitizer: global-buffer-overflow
common/dlmalloc.c:1397 in barebox_free
Shadow bytes around the buggy address:
0x000080122b80: 01 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9
0x000080122b90: 01 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
0x000080122ba0: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
0x000080122bb0: 00 00 00 06 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
0x000080122bc0: 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
=>0x000080122bd0: 00 00 00 00[02]f9 f9 f9 f9 f9 f9 f9 00 00 00 00
0x000080122be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x000080122bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x000080122c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x000080122c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x000080122c20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==2828613==ABORTING
So, it seems that we also need to handle this issue as well.
Please confirm and let me know for further information.
Thanks and regards,
Neeraj
On Mon, May 10, 2021 at 4:38 PM Neeraj Pal <neerajpal09 at gmail.com> wrote:
>
> 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 |
More information about the barebox
mailing list