TOCTOU bug in make(1)

Mario Campos <[email protected]>
Newsgroups gmane.os.netbsd.devel.toolchain
Message-ID <CAHEkMs5BwztWaW0mGPs1czeWTSDC_91N9jqAh81Osc9obMaZYw@mail.gmail.com>
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;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.