storage: Don't allow "." and ".." paths, enforce '/' dir separators.

Also clarify what characters are valid for Storage paths in the category docs.

Fixes #11079.
Fixes #11370.
Fixes #11369.

include/SDL3/SDL_storage.h

@@ -222,6 +222,22 @@
  * playing on another PC (and vice versa) with the save data fully
  * synchronized across all devices, allowing for a seamless experience without
  * having to do full restarts of the program.
+ *
+ * ## Notes on valid paths
+ *
+ * All paths in the Storage API use Unix-style path separators ('/'). Using a
+ * different path separator will not work, even if the underlying platform
+ * would otherwise accept it. This is to keep code using the Storage API
+ * portable between platforms and Storage implementations and simplify app
+ * code.
+ *
+ * Paths with relative directories ("." and "..") are forbidden by the Storage
+ * API.
+ *
+ * All valid UTF-8 strings (discounting the NULL terminator character and the
+ * '/' path separator) are usable for filenames, however, an underlying
+ * Storage implementation may not support particularly strange sequences and
+ * refuse to create files with those names, etc.
  */
 
 #ifndef SDL_storage_h_

src/storage/SDL_storage.c

@@ -56,6 +56,33 @@ struct SDL_Storage
         return result;                             \
     }
 
+// we don't make any effort to convert path separators here, because a)
+// everything including Windows will accept a '/' separator and b) that
+// conversion should probably happen in the storage backend anyhow.
+
+static bool ValidateStoragePath(const char *path)
+{
+    if (SDL_strchr(path, '\\')) {
+        return SDL_SetError("Windows-style path separators ('\\') not permitted, use '/' instead.");
+    }
+
+    const char *ptr;
+    const char *prev = path;
+    while ((ptr = SDL_strchr(prev, '/')) != NULL) {
+        if ((SDL_strncmp(prev, "./", 2) == 0) || (SDL_strncmp(prev, "../", 3) == 0)) {
+            return SDL_SetError("Relative paths not permitted");
+        }
+        prev = ptr + 1;
+    }
+
+    // check the last path element (or the only path element).
+    if ((SDL_strcmp(prev, ".") == 0) || (SDL_strcmp(prev, "..") == 0)) {
+        return SDL_SetError("Relative paths not permitted");
+    }
+
+    return true;
+}
+
 SDL_Storage *SDL_OpenTitleStorage(const char *override, SDL_PropertiesID props)
 {
     SDL_Storage *storage = NULL;
@@ -213,9 +240,9 @@ bool SDL_ReadStorageFile(SDL_Storage *storage, const char *path, void *destinati
 
     if (!path) {
         return SDL_InvalidParamError("path");
-    }
+    } else if (!ValidateStoragePath(path)) {
-
+        return false;
-    if (!storage->iface.read_file) {
+    } else if (!storage->iface.read_file) {
         return SDL_Unsupported();
     }
 
@@ -228,9 +255,9 @@ bool SDL_WriteStorageFile(SDL_Storage *storage, const char *path, const void *so
 
     if (!path) {
         return SDL_InvalidParamError("path");
-    }
+    } else if (!ValidateStoragePath(path)) {
-
+        return false;
-    if (!storage->iface.write_file) {
+    } else if (!storage->iface.write_file) {
         return SDL_Unsupported();
     }
 
@@ -243,9 +270,9 @@ bool SDL_CreateStorageDirectory(SDL_Storage *storage, const char *path)
 
     if (!path) {
         return SDL_InvalidParamError("path");
-    }
+    } else if (!ValidateStoragePath(path)) {
-
+        return false;
-    if (!storage->iface.mkdir) {
+    } else if (!storage->iface.mkdir) {
         return SDL_Unsupported();
     }
 
@@ -258,9 +285,9 @@ bool SDL_EnumerateStorageDirectory(SDL_Storage *storage, const char *path, SDL_E
 
     if (!path) {
         return SDL_InvalidParamError("path");
-    }
+    } else if (!ValidateStoragePath(path)) {
-
+        return false;
-    if (!storage->iface.enumerate) {
+    } else if (!storage->iface.enumerate) {
         return SDL_Unsupported();
     }
 
@@ -273,9 +300,9 @@ bool SDL_RemoveStoragePath(SDL_Storage *storage, const char *path)
 
     if (!path) {
         return SDL_InvalidParamError("path");
-    }
+    } else if (!ValidateStoragePath(path)) {
-
+        return false;
-    if (!storage->iface.remove) {
+    } else if (!storage->iface.remove) {
         return SDL_Unsupported();
     }
 
@@ -288,12 +315,13 @@ bool SDL_RenameStoragePath(SDL_Storage *storage, const char *oldpath, const char
 
     if (!oldpath) {
         return SDL_InvalidParamError("oldpath");
-    }
+    } else if (!newpath) {
-    if (!newpath) {
         return SDL_InvalidParamError("newpath");
-    }
+    } else if (!ValidateStoragePath(oldpath)) {
-
+        return false;
-    if (!storage->iface.rename) {
+    } else if (!ValidateStoragePath(newpath)) {
+        return false;
+    } else if (!storage->iface.rename) {
         return SDL_Unsupported();
     }
 
@@ -306,12 +334,13 @@ bool SDL_CopyStorageFile(SDL_Storage *storage, const char *oldpath, const char *
 
     if (!oldpath) {
         return SDL_InvalidParamError("oldpath");
-    }
+    } else if (!newpath) {
-    if (!newpath) {
         return SDL_InvalidParamError("newpath");
-    }
+    } else if (!ValidateStoragePath(oldpath)) {
-
+        return false;
-    if (!storage->iface.copy) {
+    } else if (!ValidateStoragePath(newpath)) {
+        return false;
+    } else if (!storage->iface.copy) {
         return SDL_Unsupported();
     }
 
@@ -331,9 +360,9 @@ bool SDL_GetStoragePathInfo(SDL_Storage *storage, const char *path, SDL_PathInfo
 
     if (!path) {
         return SDL_InvalidParamError("path");
-    }
+    } else if (!ValidateStoragePath(path)) {
-
+        return false;
-    if (!storage->iface.info) {
+    } else if (!storage->iface.info) {
         return SDL_Unsupported();
     }
 
@@ -365,6 +394,14 @@ static bool GlobStorageDirectoryEnumerator(const char *path, SDL_EnumerateDirect
 char **SDL_GlobStorageDirectory(SDL_Storage *storage, const char *path, const char *pattern, SDL_GlobFlags flags, int *count)
 {
     CHECK_STORAGE_MAGIC_RET(NULL)
+
+    if (!path) {
+        SDL_InvalidParamError("path");
+        return NULL;
+    } else if (!ValidateStoragePath(path)) {
+        return NULL;
+    }
+
     return SDL_InternalGlobDirectory(path, pattern, flags, count, GlobStorageDirectoryEnumerator, GlobStorageDirectoryGetPathInfo, storage);
 }
 

src/storage/generic/SDL_genericstorage.c

@@ -26,15 +26,8 @@
 
 static char *GENERIC_INTERNAL_CreateFullPath(const char *base, const char *relative)
 {
-    if (!base) {
+    char *result = NULL;
-        return SDL_strdup(relative);
+    SDL_asprintf(&result, "%s%s", base ? base : "", relative);
-    }
-
-    size_t len = SDL_strlen(base) + SDL_strlen(relative) + 1;
-    char *result = (char*)SDL_malloc(len);
-    if (result != NULL) {
-        SDL_snprintf(result, len, "%s%s", base, relative);
-    }
     return result;
 }
 
@@ -56,8 +49,27 @@ static SDL_EnumerationResult SDLCALL GENERIC_EnumerateDirectory(void *userdata,
     // SDL_EnumerateDirectory will return the full path, so for Storage we
     // can take the base directory and add its length to the dirname string,
     // effectively trimming the root without having to strdup anything.
-    GenericEnumerateData *wrap_data = (GenericEnumerateData *)userdata;
+    const GenericEnumerateData *wrap_data = (GenericEnumerateData *)userdata;
-    return wrap_data->real_callback(wrap_data->real_userdata, dirname + wrap_data->base_len, fname);
+
+    dirname += wrap_data->base_len;  // skip the base, just return the part inside of the Storage.
+
+    #ifdef SDL_PLATFORM_WINDOWS
+    char *dirnamecpy = NULL;
+    const size_t slen = SDL_strlen(dirname);
+    if (slen && (dirname[slen - 1] == '\\')) {
+        dirnamecpy = SDL_strdup(dirname);
+        if (!dirnamecpy) {
+            return SDL_ENUM_FAILURE;
+        }
+        dirnamecpy[slen - 1] = '/';  // storage layer always uses '/' path separators.
+        dirname = dirnamecpy;
+    }
+    const SDL_EnumerationResult retval = wrap_data->real_callback(wrap_data->real_userdata, dirname, fname);
+    SDL_free(dirnamecpy);
+    return retval;
+    #else
+    return wrap_data->real_callback(wrap_data->real_userdata, dirname, fname);
+    #endif
 }
 
 static bool GENERIC_EnumerateStorageDirectory(void *userdata, const char *path, SDL_EnumerateDirectoryCallback callback, void *callback_userdata)

test/testfilesystem.c

@@ -150,6 +150,7 @@ int main(int argc, char *argv[])
         SDL_Storage *storage = NULL;
         SDL_IOStream *stream;
         const char *text = "foo\n";
+        SDL_PathInfo pathinfo;
 
         if (!SDL_EnumerateDirectory(base_path, enum_callback, NULL)) {
             SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Base path enumeration failed!");
@@ -233,7 +234,7 @@ int main(int argc, char *argv[])
         if (!storage) {
             SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to open base path storage object: %s", SDL_GetError());
         } else {
-            if (!SDL_EnumerateStorageDirectory(storage, "", enum_storage_callback, storage)) {
+            if (!SDL_EnumerateStorageDirectory(storage, "CMakeFiles", enum_storage_callback, storage)) {
                 SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Storage Base path enumeration failed!");
             }
 
@@ -247,6 +248,62 @@ int main(int argc, char *argv[])
                 }
                 SDL_free(globlist);
             }
+
+            /* these should fail: */
+            if (!SDL_GetStoragePathInfo(storage, "CMakeFiles/../testsprite.c", &pathinfo)) {
+                SDL_Log("Storage access on path with internal '..' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with internal '..' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "CMakeFiles/./TargetDirectories.txt", &pathinfo)) {
+                SDL_Log("Storage access on path with internal '.' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with internal '.' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "../test", &pathinfo)) {
+                SDL_Log("Storage access on path with leading '..' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with leading '..' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "./CMakeFiles", &pathinfo)) {
+                SDL_Log("Storage access on path with leading '.' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with leading '.' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "CMakeFiles/..", &pathinfo)) {
+                SDL_Log("Storage access on path with trailing '..' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with trailing '..' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "CMakeFiles/.", &pathinfo)) {
+                SDL_Log("Storage access on path with trailing '.' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with trailing '.' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "..", &pathinfo)) {
+                SDL_Log("Storage access on path '..' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path '..' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, ".", &pathinfo)) {
+                SDL_Log("Storage access on path '.' refused correctly.");
+            } else {
+                SDL_Log("Storage access on path '.' accepted INCORRECTLY.");
+            }
+
+            if (!SDL_GetStoragePathInfo(storage, "CMakeFiles\\TargetDirectories.txt", &pathinfo)) {
+                SDL_Log("Storage access on path with Windows separator refused correctly.");
+            } else {
+                SDL_Log("Storage access on path with Windows separator accepted INCORRECTLY.");
+            }
+
             SDL_CloseStorage(storage);
         }