PATCH: Stack-based buffer overflow in exec.c:hashcmd()

PATCH: Stack-based buffer overflow in exec.c:hashcmd()

[Oliver Kiddle]

The code in exec.c:hashcmd() alocates a PATH_MAX+1 sized buffer but
doesn't check for overflows when copying the path strings which come
from $PATH. This bug corresponds to CVE-2018-1071 and was reported
off-list.

The copy is done using strucpy() which is like strcpy(3) except that
we increment the pointer for reasons of efficency. We also have a
struncpy() function but it is unconditionally copying n bytes before
adding a null. This isn't especially helpful - you could just add n
to the initial pointer to gain the efficiency and writing n+1 bytes
is asking for trouble. So this changes struncpy() to be closer to the
strncpy(3) analogue except I don't see the point in padding the whole
buffer with nulls. There was only one existing use of struncpy() in
exec.c:search_defpath().

diff --git a/Src/exec.c b/Src/exec.c
index 35b0bb191..e154d1249 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -934,7 +934,7 @@ hashcmd(char *arg0, char **pp)
     for (; *pp; pp++)
 	if (**pp == '/') {
 	    s = buf;
-	    strucpy(&s, *pp);
+	    struncpy(&s, *pp, PATH_MAX);
 	    *s++ = '/';
 	    if ((s - buf) + strlen(arg0) >= PATH_MAX)
 		continue;
diff --git a/Src/utils.c b/Src/utils.c
index 3b589aa35..998b16220 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2283,10 +2283,10 @@ struncpy(char **s, char *t, int n)
 {
     char *u = *s;
 
-    while (n--)
-	*u++ = *t++;
+    while (n-- && (*u++ = *t++));
     *s = u;
-    *u = '\0';
+    if (n > 0) /* just one null-byte will do, unlike strncpy(3) */
+	*u = '\0';
 }
 
 /* Return the number of elements in an array of pointers. *