fix(vfs): Fix TOCTOU vulnerability

Author: lucasssvaz

libraries/FS/src/vfs_api.cpp

@@ -13,6 +13,9 @@
 // limitations under the License.
 
 #include "vfs_api.h"
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
 
 using namespace fs;
 
@@ -38,67 +41,49 @@ FileImplPtr VFSImpl::open(const char *fpath, const char *mode, const bool create
   strcpy(temp, _mountpoint);
   strcat(temp, fpath);
 
-  struct stat st;
-  //file found
-  if (!stat(temp, &st)) {
-    free(temp);
-    if (S_ISREG(st.st_mode) || S_ISDIR(st.st_mode)) {
-      return std::make_shared<VFSFileImpl>(this, fpath, mode);
-    }
-    log_e("%s has wrong mode 0x%08X", fpath, st.st_mode);
-    return FileImplPtr();
-  }
-
-  //try to open this as directory (might be mount point)
-  DIR *d = opendir(temp);
-  if (d) {
-    closedir(d);
-    free(temp);
-    return std::make_shared<VFSFileImpl>(this, fpath, mode);
-  }
-
-  //file not found but mode permits file creation without folder creation
-  if ((mode && mode[0] != 'r') && (!create)) {
-    free(temp);
-    return std::make_shared<VFSFileImpl>(this, fpath, mode);
-  }
-
-  ////file not found but mode permits file creation and folder creation
-  if ((mode && mode[0] != 'r') && create) {
-
-    char *token;
-    char *folder = (char *)malloc(strlen(fpath) + 1);
-
-    int start_index = 0;
-    int end_index = 0;
-
-    token = strchr(fpath + 1, '/');
-    end_index = (token - fpath);
-
-    while (token != NULL) {
-      memcpy(folder, fpath + start_index, end_index - start_index);
-      folder[end_index - start_index] = '\0';
-
-      if (!VFSImpl::mkdir(folder)) {
-        log_e("Creating folder: %s failed!", folder);
-        return FileImplPtr();
+  // Try to open as file first - let the file operation handle errors
+  if (mode && mode[0] != 'r') {
+    // For write modes, attempt to create directories if needed
+    if (create) {
+      char *token;
+      char *folder = (char *)malloc(strlen(fpath) + 1);
+
+      int start_index = 0;
+      int end_index = 0;
+
+      token = strchr(fpath + 1, '/');
+      end_index = (token - fpath);
+
+      while (token != NULL) {
+        memcpy(folder, fpath + start_index, end_index - start_index);
+        folder[end_index - start_index] = '\0';
+
+        if (!VFSImpl::mkdir(folder)) {
+          log_e("Creating folder: %s failed!", folder);
+          free(folder);
+          free(temp);
+          return FileImplPtr();
+        }
+
+        token = strchr(token + 1, '/');
+        if (token != NULL) {
+          end_index = (token - fpath);
+          memset(folder, 0, strlen(folder));
+        }
       }
 
-      token = strchr(token + 1, '/');
-      if (token != NULL) {
-        end_index = (token - fpath);
-        memset(folder, 0, strlen(folder));
-      }
+      free(folder);
     }
 
-    free(folder);
+    // Try to open the file directly - let fopen handle errors
     free(temp);
     return std::make_shared<VFSFileImpl>(this, fpath, mode);
   }
 
-  log_e("%s does not exist, no permits for creation", temp);
+  // For read mode, let the VFSFileImpl constructor handle the file opening
+  // This avoids the TOCTOU race condition while maintaining proper functionality
   free(temp);
-  return FileImplPtr();
+  return std::make_shared<VFSFileImpl>(this, fpath, mode);
 }
 
 bool VFSImpl::exists(const char *fpath) {
@@ -125,10 +110,7 @@ bool VFSImpl::rename(const char *pathFrom, const char *pathTo) {
     log_e("bad arguments");
     return false;
   }
-  if (!exists(pathFrom)) {
-    log_e("%s does not exists", pathFrom);
-    return false;
-  }
+
   size_t mountpointLen = strlen(_mountpoint);
   char *temp1 = (char *)malloc(strlen(pathFrom) + mountpointLen + 1);
   if (!temp1) {
@@ -148,6 +130,7 @@ bool VFSImpl::rename(const char *pathFrom, const char *pathTo) {
   strcpy(temp2, _mountpoint);
   strcat(temp2, pathTo);
 
+  // Let rename() handle the error if source doesn't exist
   auto rc = ::rename(temp1, temp2);
   free(temp1);
   free(temp2);
@@ -165,16 +148,6 @@ bool VFSImpl::remove(const char *fpath) {
     return false;
   }
 
-  VFSFileImpl f(this, fpath, "r");
-  if (!f || f.isDirectory()) {
-    if (f) {
-      f.close();
-    }
-    log_e("%s does not exists or is directory", fpath);
-    return false;
-  }
-  f.close();
-
   char *temp = (char *)malloc(strlen(fpath) + strlen(_mountpoint) + 1);
   if (!temp) {
     log_e("malloc failed");
@@ -184,6 +157,7 @@ bool VFSImpl::remove(const char *fpath) {
   strcpy(temp, _mountpoint);
   strcat(temp, fpath);
 
+  // Let unlink() handle the error if file doesn't exist
   auto rc = unlink(temp);
   free(temp);
   return rc == 0;
@@ -195,17 +169,6 @@ bool VFSImpl::mkdir(const char *fpath) {
     return false;
   }
 
-  VFSFileImpl f(this, fpath, "r");
-  if (f && f.isDirectory()) {
-    f.close();
-    //log_w("%s already exists", fpath);
-    return true;
-  } else if (f) {
-    f.close();
-    log_e("%s is a file", fpath);
-    return false;
-  }
-
   char *temp = (char *)malloc(strlen(fpath) + strlen(_mountpoint) + 1);
   if (!temp) {
     log_e("malloc failed");
@@ -215,9 +178,15 @@ bool VFSImpl::mkdir(const char *fpath) {
   strcpy(temp, _mountpoint);
   strcat(temp, fpath);
 
+  // Let mkdir() handle the error if directory already exists
   auto rc = ::mkdir(temp, ACCESSPERMS);
   free(temp);
-  return rc == 0;
+
+  // EEXIST means directory already exists, which is fine
+  if (rc == 0 || errno == EEXIST) {
+    return true;
+  }
+  return false;
 }
 
 bool VFSImpl::rmdir(const char *fpath) {
@@ -231,16 +200,6 @@ bool VFSImpl::rmdir(const char *fpath) {
     return false;
   }
 
-  VFSFileImpl f(this, fpath, "r");
-  if (!f || !f.isDirectory()) {
-    if (f) {
-      f.close();
-    }
-    log_e("%s does not exists or is a file", fpath);
-    return false;
-  }
-  f.close();
-
   char *temp = (char *)malloc(strlen(fpath) + strlen(_mountpoint) + 1);
   if (!temp) {
     log_e("malloc failed");
@@ -250,6 +209,7 @@ bool VFSImpl::rmdir(const char *fpath) {
   strcpy(temp, _mountpoint);
   strcat(temp, fpath);
 
+  // Let rmdir() handle the error if directory doesn't exist
   auto rc = ::rmdir(temp);
   free(temp);
   return rc == 0;
@@ -271,29 +231,30 @@ VFSFileImpl::VFSFileImpl(VFSImpl *fs, const char *fpath, const char *mode) : _fs
     return;
   }
 
-  if (!stat(temp, &_stat)) {
-    //file found
-    if (S_ISREG(_stat.st_mode)) {
-      _isDirectory = false;
-      _f = fopen(temp, mode);
-      if (!_f) {
-        log_e("fopen(%s) failed", temp);
-      }
-      if (_f && (_stat.st_blksize == 0)) {
-        setvbuf(_f, NULL, _IOFBF, DEFAULT_FILE_BUFFER_SIZE);
-      }
-    } else if (S_ISDIR(_stat.st_mode)) {
-      _isDirectory = true;
-      _d = opendir(temp);
-      if (!_d) {
-        log_e("opendir(%s) failed", temp);
+  // For read mode, check if file exists first to determine type
+  if (!mode || mode[0] == 'r') {
+    if (!stat(temp, &_stat)) {
+      //file found
+      if (S_ISREG(_stat.st_mode)) {
+        _isDirectory = false;
+        _f = fopen(temp, mode);
+        if (!_f) {
+          log_e("fopen(%s) failed", temp);
+        }
+        if (_f && (_stat.st_blksize == 0)) {
+          setvbuf(_f, NULL, _IOFBF, DEFAULT_FILE_BUFFER_SIZE);
+        }
+      } else if (S_ISDIR(_stat.st_mode)) {
+        _isDirectory = true;
+        _d = opendir(temp);
+        if (!_d) {
+          log_e("opendir(%s) failed", temp);
+        }
+      } else {
+        log_e("Unknown type 0x%08X for file %s", ((_stat.st_mode) & _IFMT), temp);
       }
     } else {
-      log_e("Unknown type 0x%08X for file %s", ((_stat.st_mode) & _IFMT), temp);
-    }
-  } else {
-    //file not found
-    if (!mode || mode[0] == 'r') {
+      //file not found
       //try to open as directory
       _d = opendir(temp);
       if (_d) {
@@ -302,16 +263,16 @@ VFSFileImpl::VFSFileImpl(VFSImpl *fs, const char *fpath, const char *mode) : _fs
         _isDirectory = false;
         //log_w("stat(%s) failed", temp);
       }
-    } else {
-      //lets create this new file
-      _isDirectory = false;
-      _f = fopen(temp, mode);
-      if (!_f) {
-        log_e("fopen(%s) failed", temp);
-      }
-      if (_f && (_stat.st_blksize == 0)) {
-        setvbuf(_f, NULL, _IOFBF, DEFAULT_FILE_BUFFER_SIZE);
-      }
+    }
+  } else {
+    //lets create this new file
+    _isDirectory = false;
+    _f = fopen(temp, mode);
+    if (!_f) {
+      log_e("fopen(%s) failed", temp);
+    }
+    if (_f && (_stat.st_blksize == 0)) {
+      setvbuf(_f, NULL, _IOFBF, DEFAULT_FILE_BUFFER_SIZE);
     }
   }
   free(temp);