From 875eb58fb05a9075b3020e69cf8940d1d2fb6ad2 Mon Sep 17 00:00:00 2001
From: Philippe Antoine <contact@catenacyber.fr>
Date: Sat, 16 Apr 2022 16:46:01 +0200
Subject: [PATCH] file: use functions on fd to avoid toctou

Ticket: #5308
---
 src/suricata.c         | 17 +++++++-------
 src/util-debug.c       |  2 +-
 src/util-logopenfile.c |  6 ++++-
 src/util-pidfile.c     | 50 +++++++++++++++++++++---------------------
 4 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/src/suricata.c b/src/suricata.c
index f4f3b0c52d..9dcf8b3aa3 100644
--- a/src/suricata.c
+++ b/src/suricata.c
@@ -530,23 +530,22 @@ static void SetBpfStringFromFile(char *filename)
                               " Use firewall filtering if possible.");
     }
 
+    fp = fopen(filename, "r");
+    if (fp == NULL) {
+        SCLogError(SC_ERR_FOPEN, "Failed to open file %s", filename);
+        exit(EXIT_FAILURE);
+    }
+
 #ifdef OS_WIN32
-    if(_stat(filename, &st) != 0) {
+    if (_fstat(_fileno(fp), &st) != 0) {
 #else
-    if(stat(filename, &st) != 0) {
+    if (fstat(fileno(fp), &st) != 0) {
 #endif /* OS_WIN32 */
         SCLogError(SC_ERR_FOPEN, "Failed to stat file %s", filename);
         exit(EXIT_FAILURE);
     }
     bpf_len = st.st_size + 1;
 
-    // coverity[toctou : FALSE]
-    fp = fopen(filename,"r");
-    if (fp == NULL) {
-        SCLogError(SC_ERR_FOPEN, "Failed to open file %s", filename);
-        exit(EXIT_FAILURE);
-    }
-
     bpf_filter = SCMalloc(bpf_len * sizeof(char));
     if (unlikely(bpf_filter == NULL)) {
         SCLogError(SC_ERR_MEM_ALLOC, "Failed to allocate buffer for bpf filter in file %s", filename);
diff --git a/src/util-debug.c b/src/util-debug.c
index b96d75feef..3c8a95f35f 100644
--- a/src/util-debug.c
+++ b/src/util-debug.c
@@ -742,7 +742,7 @@ static inline SCLogOPIfaceCtx *SCLogInitFileOPIface(const char *file, uint32_t u
 
 #ifndef OS_WIN32
     if (userid != 0 || groupid != 0) {
-        if (chown(file, userid, groupid) == -1) {
+        if (fchown(fileno(iface_ctx->file_d), userid, groupid) == -1) {
             SCLogWarning(SC_WARN_CHOWN, "Failed to change ownership of file %s: %s", file,
                     strerror(errno));
         }
diff --git a/src/util-logopenfile.c b/src/util-logopenfile.c
index 1150166d93..8e0f89dd16 100644
--- a/src/util-logopenfile.c
+++ b/src/util-logopenfile.c
@@ -401,7 +401,11 @@ SCLogOpenFileFp(const char *path, const char *append_setting, uint32_t mode)
                    filename, strerror(errno));
     } else {
         if (mode != 0) {
-            int r = chmod(filename, (mode_t)mode);
+#ifdef OS_WIN32
+            int r = _chmod(filename, (mode_t)mode);
+#else
+            int r = fchmod(fileno(ret), (mode_t)mode);
+#endif
             if (r < 0) {
                 SCLogWarning(SC_WARN_CHMOD, "Could not chmod %s to %o: %s",
                              filename, mode, strerror(errno));
diff --git a/src/util-pidfile.c b/src/util-pidfile.c
index acb92b230e..cbc57d6cb4 100644
--- a/src/util-pidfile.c
+++ b/src/util-pidfile.c
@@ -104,37 +104,37 @@ void SCPidfileRemove(const char *pid_filename)
  */
 int SCPidfileTestRunning(const char *pid_filename)
 {
-    if (access(pid_filename, F_OK) == 0) {
-        /* Check if the existing process is still alive. */
-        FILE *pf;
+    /* Check if the existing process is still alive. */
+    FILE *pf;
 
-        // coverity[toctou : FALSE]
-        pf = fopen(pid_filename, "r");
-        if (pf == NULL) {
-            SCLogError(SC_ERR_INITIALIZATION,
-                    "pid file '%s' exists and can not be read. Aborting!",
+    pf = fopen(pid_filename, "r");
+    if (pf == NULL) {
+        if (access(pid_filename, F_OK) == 0) {
+            SCLogError(SC_ERR_INITIALIZATION, "pid file '%s' exists and can not be read. Aborting!",
                     pid_filename);
             return -1;
+        } else {
+            return 0;
         }
+    }
 
 #ifndef OS_WIN32
-        pid_t pidv;
-        if (fscanf(pf, "%d", &pidv) == 1 && kill(pidv, 0) == 0) {
-            SCLogError(SC_ERR_INITIALIZATION,
-                    "pid file '%s' exists and Suricata appears to be running. "
-                    "Aborting!", pid_filename);
-        } else
+    pid_t pidv;
+    if (fscanf(pf, "%d", &pidv) == 1 && kill(pidv, 0) == 0) {
+        SCLogError(SC_ERR_INITIALIZATION,
+                "pid file '%s' exists and Suricata appears to be running. "
+                "Aborting!",
+                pid_filename);
+    } else
 #endif
-        {
-            SCLogError(SC_ERR_INITIALIZATION,
-                    "pid file '%s' exists but appears stale. "
-                    "Make sure Suricata is not running and then remove %s. "
-                    "Aborting!",
-                    pid_filename, pid_filename);
-        }
-
-        fclose(pf);
-        return -1;
+    {
+        SCLogError(SC_ERR_INITIALIZATION,
+                "pid file '%s' exists but appears stale. "
+                "Make sure Suricata is not running and then remove %s. "
+                "Aborting!",
+                pid_filename, pid_filename);
     }
-    return 0;
+
+    fclose(pf);
+    return -1;
 }