mail archive of the barebox mailing list
 help / color / mirror / Atom feed
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 18:48:20 +0530	[thread overview]
Message-ID: <CANi4_RXrSqYkVJmCFRGSEtQvtZnWZ1Mnxu3nNH3caUvKE7d11A@mail.gmail.com> (raw)
In-Reply-To: <CANi4_RWKvQV68RaZLNBFTJP1z3vObuZYo-HRqfU6r4rL8ZxKoQ@mail.gmail.com>

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@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@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@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


  reply	other threads:[~2021-05-10 13:19 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
2021-05-10 13:18     ` Neeraj Pal [this message]
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_RXrSqYkVJmCFRGSEtQvtZnWZ1Mnxu3nNH3caUvKE7d11A@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