diff --git a/include/SDL3/SDL_storage.h b/include/SDL3/SDL_storage.h index afe50cb1dd..f6be770db8 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 15e68673bd..a5687c6516 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 d8c300c59f..3d3e93025b 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 a2559119f8..bdd0bac59f 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); }