TOCTOU bug in make(1)

[Mario Campos <mario.andres.campos%gmail.com@localhost>]

Hi, tech-toolchain@.

This is a first for me, so if I'm addressing the wrong mailing list or doing something wrong, please let me know.

I ran CodeQL, a SAST tool, against trunk. It found a TOCTOU vulnerability in the `unlink_file` function of make(1). The function is a small wrapper over unlink(2), but it first checks that the file exists using lstat(2). Although I don't see an immediate danger here, I admit I'm not very imaginative for vulnerabilities.

There appears to be two call sites of `unlink_file` in make(1) and neither of them use the return value beyond checking that it is not zero. Therefore, I think we can safely remove the `unlink_file` wrapper function, since it doesn't *seem* to be providing much value. Diff below.

diff --git a/usr.bin/make/compat.c b/usr.bin/make/compat.c
index affbf09ec..d99a5f2de 100644
--- a/usr.bin/make/compat.c
+++ b/usr.bin/make/compat.c
@@ -107,7 +107,7 @@ CompatDeleteTarget(GNode *gn)
        if (gn != NULL && !GNode_IsPrecious(gn)) {
                const char *file = GNode_VarTarget(gn);
 
-               if (!opts.noExecute && unlink_file(file)) {
+               if (!opts.noExecute && unlink(file)) {
                        Error("*** %s removed", file);
                }
        }
diff --git a/usr.bin/make/job.c b/usr.bin/make/job.c
index f69627797..90b200cf7 100644
--- a/usr.bin/make/job.c
+++ b/usr.bin/make/job.c
@@ -512,7 +512,7 @@ JobDeleteTarget(GNode *gn)
                return;
 
        file = GNode_Path(gn);
-       if (unlink_file(file))
+       if (unlink(file))
                Error("*** %s removed", file);
 }
 
diff --git a/usr.bin/make/main.c b/usr.bin/make/main.c
index 6d45e1be0..b3625ffbf 100644
--- a/usr.bin/make/main.c
+++ b/usr.bin/make/main.c
@@ -1881,21 +1881,6 @@ Finish(int errs)
        Fatal("%d error%s", errs, errs == 1 ? "" : "s");
 }
 
-bool
-unlink_file(const char *file)
-{
-       struct stat st;
-
-       if (lstat(file, &st) == -1)
-               return false;
-
-       if (S_ISDIR(st.st_mode)) {
-               errno = EISDIR;
-               return false;
-       }
-       return unlink(file) == 0;
-}
-
 static void
 write_all(int fd, const void *data, size_t n)
 {
diff --git a/usr.bin/make/make.h b/usr.bin/make/make.h
index e8b329d71..fcb8d41e1 100644
--- a/usr.bin/make/make.h
+++ b/usr.bin/make/make.h
@@ -842,7 +842,6 @@ void Fatal(const char *, ...) MAKE_ATTR_PRINTFLIKE(1, 2) MAKE_ATTR_DEAD;
 void Punt(const char *, ...) MAKE_ATTR_PRINTFLIKE(1, 2) MAKE_ATTR_DEAD;
 void DieHorribly(void) MAKE_ATTR_DEAD;
 void Finish(int) MAKE_ATTR_DEAD;
-bool unlink_file(const char *) MAKE_ATTR_USE;
 void execDie(const char *, const char *);
 char *getTmpdir(void) MAKE_ATTR_USE;
 bool ParseBoolean(const char *, bool) MAKE_ATTR_USE;