Merge pull request #21 from NathanNeurotic/codex/replace-sprintf-with-snprintf-in-getmountinfo

Guard getMountInfo against buffer overflows

Author: NathanNeurotic

include/util.h

@@ -1,3 +1,5 @@
+#include <stddef.h>
+
 /**
  * @brief checks for file existence
  * @param filepath path to the file to check
@@ -26,14 +28,17 @@ int get_CNF_string(char **CNF_p_p,
                    char **value_p_p);
 
 /**
- * @brief  method returns true if it can extract needed info from path, otherwise false.
- * In case of true, it also updates mountString, mountPoint and newCWD parameters
+ * @brief  method returns 0 if it can extract needed info from path, otherwise a negative error code.
+ * In case of success, it also updates mountString, mountPoint and newCWD parameters
  * It splits path by ":", and requires a minimum of 3 elements
  * @example if path = hdd0:__common:pfs:/retroarch/ then: mountString = "pfs:", mountPoint = "hdd0:__common", newCWD = pfs:/retroarch/
  * @param path input parameter with full hdd path (`hdd0:__common:pfs:/retroarch/`)
  * @param mountString pointer to char* wich will contain pfs mountpoint (`pfs:`)
+ * @param mountStringSize total size of the mountString buffer
  * @param mountPoint returns the path of mounted partition (`hdd0:__common`)
+ * @param mountPointSize total size of the mountPoint buffer
  * @param newCWD returns the path to the file as pfs mount point string
- * @return true on success
+ * @param newCWDSize total size of the newCWD buffer
+ * @return 0 on success, negative error code on failure or truncation
 */
-int getMountInfo(char *path, char *mountString, char *mountPoint, char *newCWD);
+int getMountInfo(char *path, char *mountString, size_t mountStringSize, char *mountPoint, size_t mountPointSize, char *newCWD, size_t newCWDSize);

src/main.c

@@ -1,3 +1,5 @@
+#include <errno.h>
+
 #include "main.h"
 // --------------- glob stuff --------------- //
 #define RUNKELF_ARG_BUF_SIZE 64
@@ -1296,16 +1298,29 @@ int MountParty(const char *path)
     DPRINTF("%s: %s\n", __func__, path);
     char *BUF = NULL;
     BUF = strdup(path); //use strdup, otherwise, path will become `hdd0:`
-    char MountPoint[40];
-    if (getMountInfo(BUF, NULL, MountPoint, NULL)) {
-        mnt(MountPoint);
-        if (BUF != NULL)
-            free(BUF);
-        strcpy(PART, MountPoint);
-        strcat(PART, ":");
-        return 0;
+    if (BUF == NULL) {
+        DPRINTF("ERROR: could not duplicate path '%s'\n", path);
+        return ret;
+    }
+    char MountPoint[sizeof(PART)];
+    int mountInfoRet = getMountInfo(BUF, NULL, 0, MountPoint, sizeof(MountPoint), NULL, 0);
+    if (mountInfoRet == 0) {
+        ret = mnt(MountPoint);
+        if (ret == 0) {
+            int part_written = snprintf(PART, sizeof(PART), "%s:", MountPoint);
+            if (part_written < 0 || (size_t)part_written >= sizeof(PART)) {
+                DPRINTF("ERROR: partition path too long for PART buffer\n");
+                PART[0] = '\0';
+                ret = -ENAMETOOLONG;
+            } else {
+                free(BUF);
+                return 0;
+            }
+        }
     } else {
-        DPRINTF("ERROR: could not process path '%s'\n", path);
+        DPRINTF("ERROR: could not process path '%s' (err=%d)\n", path, mountInfoRet);
+        if (mountInfoRet == -ENAMETOOLONG)
+            DPRINTF("Path components exceed buffer limits and were rejected\n");
         PART[0] = '\0';
     }
     if (BUF != NULL)

src/util.c

@@ -1,3 +1,4 @@
+#include <errno.h>
 #include <stdio.h>
 #include <string.h>
 #include <fcntl.h>
@@ -138,52 +139,86 @@ char **str_split(char *a_str, const char a_delim)
 }
 
 /**
- * @brief  method returns true if it can extract needed info from path, otherwise false.
- * In case of true, it also updates mountString, mountPoint and newCWD parameters
+ * @brief  method returns 0 if it can extract needed info from path, otherwise a negative error code.
+ * In case of success, it also updates mountString, mountPoint and newCWD parameters
  * It splits path by ":", and requires a minimum of 3 elements
  * Example: if path = hdd0:__common:pfs:/retroarch/ then
  * mountString = "pfs:"
  * mountPoint = "hdd0:__common"
  * newCWD = pfs:/retroarch/
- * return true
+ * return 0 on success, negative error code on failure
 */
-int getMountInfo(char *path, char *mountString, char *mountPoint, char *newCWD)
+int getMountInfo(char *path, char *mountString, size_t mountStringSize, char *mountPoint, size_t mountPointSize, char *newCWD, size_t newCWDSize)
 {
-    int expected_items = 4;
     int i = 0;
-    char *items[expected_items];
     char **tokens = str_split(path, ':');
+    int ret = -EINVAL;
 
     if (!tokens)
-        return 0;
+        return -ENOMEM;
 
-    for (i = 0; *(tokens + i); i++) {
-        if (i < expected_items) {
-            items[i] = *(tokens + i);
-        } else {
-            free(*(tokens + i));
-        }
-    }
+    for (i = 0; *(tokens + i); i++)
+        ;
 
     if (i < 3)
-        return 0;
+        goto cleanup;
 
-    if (mountPoint != NULL)
-        sprintf(mountPoint, "%s:%s", items[0], items[1]);
+    if (mountPoint != NULL) {
+        if (mountPointSize == 0) {
+            ret = -ENAMETOOLONG;
+            goto cleanup;
+        }
+        int written = snprintf(mountPoint, mountPointSize, "%s:%s", tokens[0], tokens[1]);
+        if (written < 0) {
+            ret = -EIO;
+            goto cleanup;
+        }
+        if ((size_t)written >= mountPointSize) {
+            ret = -ENAMETOOLONG;
+            goto cleanup;
+        }
+    }
 
-    if (mountString != NULL)
-        sprintf(mountString, "%s:", items[2]);
+    if (mountString != NULL) {
+        if (mountStringSize == 0) {
+            ret = -ENAMETOOLONG;
+            goto cleanup;
+        }
+        int written = snprintf(mountString, mountStringSize, "%s:", tokens[2]);
+        if (written < 0) {
+            ret = -EIO;
+            goto cleanup;
+        }
+        if ((size_t)written >= mountStringSize) {
+            ret = -ENAMETOOLONG;
+            goto cleanup;
+        }
+    }
 
-    if (newCWD != NULL)
-        sprintf(newCWD, "%s:%s", items[2], i > 3 ? items[3] : "");
+    if (newCWD != NULL) {
+        if (newCWDSize == 0) {
+            ret = -ENAMETOOLONG;
+            goto cleanup;
+        }
+        int written = snprintf(newCWD, newCWDSize, "%s:%s", tokens[2], i > 3 ? tokens[3] : "");
+        if (written < 0) {
+            ret = -EIO;
+            goto cleanup;
+        }
+        if ((size_t)written >= newCWDSize) {
+            ret = -ENAMETOOLONG;
+            goto cleanup;
+        }
+    }
 
-    free(items[0]);
-    free(items[1]);
-    free(items[2]);
+    ret = 0;
 
-    if (i > 3)
-        free(items[3]);
+cleanup:
+    for (int idx = 0; *(tokens + idx); idx++) {
+        free(*(tokens + idx));
+    }
+    free(tokens);
 
-    return 1;
+    return ret;
 }
 #endif