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

[Sascha Hauer <sha@pengutronix.de>]

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