From 874c07f8de25e479ad720d1ccb15a830da7d842a Mon Sep 17 00:00:00 2001
From: "Ryan C. Gordon" <[EMAIL REDACTED]>
Date: Wed, 15 Jan 2025 20:16:10 -0500
Subject: [PATCH] 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 | 16 ++++
src/storage/SDL_storage.c | 93 +++++++++++++++++-------
src/storage/generic/SDL_genericstorage.c | 34 ++++++---
test/testfilesystem.c | 59 ++++++++++++++-
4 files changed, 162 insertions(+), 40 deletions(-)
diff --git a/include/SDL3/SDL_storage.h b/include/SDL3/SDL_storage.h
index afe50cb1ddc53..f6be770db81ad 100644
--- a/include/SDL3/SDL_storage.h
+++ b/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_
diff --git a/src/storage/SDL_storage.c b/src/storage/SDL_storage.c
index 15e68673bde74..a5687c651660e 100644
--- a/src/storage/SDL_storage.c
+++ b/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");
- }
-
- if (!storage->iface.read_file) {
+ } else if (!ValidateStoragePath(path)) {
+ return false;
+ } 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");
- }
-
- if (!storage->iface.write_file) {
+ } else if (!ValidateStoragePath(path)) {
+ return false;
+ } 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");
- }
-
- if (!storage->iface.mkdir) {
+ } else if (!ValidateStoragePath(path)) {
+ return false;
+ } 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");
- }
-
- if (!storage->iface.enumerate) {
+ } else if (!ValidateStoragePath(path)) {
+ return false;
+ } 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");
- }
-
- if (!storage->iface.remove) {
+ } else if (!ValidateStoragePath(path)) {
+ return false;
+ } 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");
- }
- if (!newpath) {
+ } else if (!newpath) {
return SDL_InvalidParamError("newpath");
- }
-
- if (!storage->iface.rename) {
+ } else if (!ValidateStoragePath(oldpath)) {
+ return false;
+ } 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");
- }
- if (!newpath) {
+ } else if (!newpath) {
return SDL_InvalidParamError("newpath");
- }
-
- if (!storage->iface.copy) {
+ } else if (!ValidateStoragePath(oldpath)) {
+ return false;
+ } 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");
- }
-
- if (!storage->iface.info) {
+ } else if (!ValidateStoragePath(path)) {
+ return false;
+ } 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);
}
diff --git a/src/storage/generic/SDL_genericstorage.c b/src/storage/generic/SDL_genericstorage.c
index d8c300c59fc1e..3d3e93025b1cc 100644
--- a/src/storage/generic/SDL_genericstorage.c
+++ b/src/storage/generic/SDL_genericstorage.c
@@ -26,15 +26,8 @@
static char *GENERIC_INTERNAL_CreateFullPath(const char *base, const char *relative)
{
- if (!base) {
- return SDL_strdup(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);
- }
+ char *result = NULL;
+ SDL_asprintf(&result, "%s%s", base ? 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;
- return wrap_data->real_callback(wrap_data->real_userdata, dirname + wrap_data->base_len, fname);
+ const GenericEnumerateData *wrap_data = (GenericEnumerateData *)userdata;
+
+ 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)
diff --git a/test/testfilesystem.c b/test/testfilesystem.c
index a2559119f8083..bdd0bac59ff31 100644
--- a/test/testfilesystem.c
+++ b/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);
}