Create temporary files and folders #10966
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* | ||
| Simple DirectMedia Layer | ||
| Copyright (C) 1997-2024 Sam Lantinga <[email protected]> | ||
|
|
||
| This software is provided 'as-is', without any express or implied | ||
| warranty. In no event will the authors be held liable for any damages | ||
| arising from the use of this software. | ||
|
|
||
| Permission is granted to anyone to use this software for any purpose, | ||
| including commercial applications, and to alter it and redistribute it | ||
| freely, subject to the following restrictions: | ||
|
|
||
| 1. The origin of this software must not be misrepresented; you must not | ||
| claim that you wrote the original software. If you use this software | ||
| in a product, an acknowledgment in the product documentation would be | ||
| appreciated but is not required. | ||
| 2. Altered source versions must be plainly marked as such, and must not be | ||
| misrepresented as being the original software. | ||
| 3. This notice may not be removed or altered from any source distribution. | ||
| */ | ||
|
|
||
| #include "SDL_internal.h" | ||
| #include "../SDL_sysfilesystem.h" | ||
|
|
||
| SDL_IOStream *SDL_SYS_CreateSafeTempFile(void) | ||
| { | ||
| SDL_Unsupported(); | ||
| return NULL; | ||
| } | ||
|
|
||
| char *SDL_SYS_CreateUnsafeTempFile(void) | ||
| { | ||
| SDL_Unsupported(); | ||
| return NULL; | ||
| } | ||
|
|
||
| char *SDL_SYS_CreateTempFolder(void) | ||
| { | ||
| SDL_Unsupported(); | ||
| return NULL; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /* | ||
| Simple DirectMedia Layer | ||
| Copyright (C) 1997-2024 Sam Lantinga <[email protected]> | ||
|
|
||
| This software is provided 'as-is', without any express or implied | ||
| warranty. In no event will the authors be held liable for any damages | ||
| arising from the use of this software. | ||
|
|
||
| Permission is granted to anyone to use this software for any purpose, | ||
| including commercial applications, and to alter it and redistribute it | ||
| freely, subject to the following restrictions: | ||
|
|
||
| 1. The origin of this software must not be misrepresented; you must not | ||
| claim that you wrote the original software. If you use this software | ||
| in a product, an acknowledgment in the product documentation would be | ||
| appreciated but is not required. | ||
| 2. Altered source versions must be plainly marked as such, and must not be | ||
| misrepresented as being the original software. | ||
| 3. This notice may not be removed or altered from any source distribution. | ||
| */ | ||
|
|
||
| #include "SDL_internal.h" | ||
| #include "../SDL_sysfilesystem.h" | ||
| #include "../../file/SDL_iostream_c.h" | ||
|
|
||
| #include <unistd.h> | ||
| #include <string.h> | ||
| #include <errno.h> | ||
|
|
||
| SDL_IOStream *SDL_SYS_CreateSafeTempFile(void) | ||
| { | ||
| FILE *file = tmpfile(); | ||
|
|
||
| if (!file) { | ||
| SDL_SetError("Could not tmpfile(): %s", strerror(errno)); | ||
| return NULL; | ||
| } | ||
|
|
||
| return SDL_IOFromFP(file, true); | ||
| } | ||
|
|
||
| char *SDL_SYS_CreateUnsafeTempFile(void) | ||
| { | ||
| /* TODO: Check for possible alternatives to /tmp, like $TMP */ | ||
| char template[] = "/tmp/tmp.XXXXXX"; | ||
|
Comment on lines
+44
to
+45
There was a problem hiding this comment. It's conventional to try The template can be |
||
|
|
||
| char *file = SDL_strdup(template); | ||
|
|
||
| if (!file) { | ||
| return NULL; | ||
| } | ||
|
|
||
| int fd = mkstemp(file); | ||
|
|
||
| if (fd < 0) { | ||
| SDL_free(file); | ||
| SDL_SetError("Could not mkstemp(): %s", strerror(errno)); | ||
| return NULL; | ||
| } | ||
|
|
||
| /* Normal usage of mkstemp() would use the file descriptor rather than the | ||
| path, to avoid issues. In this function, security is assumed to be | ||
| unimportant, so no need to worry about it. */ | ||
|
Comment on lines
+62
to
+63
There was a problem hiding this comment. Unfortunately, on a multi-user system, security is never unimportant: even if the data you're saving is unimportant, symlink attacks like the one described in #11887 can be used to destroy unrelated data owned by the same user. There was a problem hiding this comment. I've called the function "unsafe" to convey that :) This function is meant for interoperability with software that require a path and accept nothing else, for example AppIndicator. Normally, the symlink attack is mitigated within the function by |
||
| /* See https://stackoverflow.com/questions/27680807/mkstemp-is-it-safe-to-close-descriptor-and-reopen-it-again */ | ||
|
|
||
| close(fd); | ||
|
|
||
| return file; | ||
| } | ||
|
|
||
| char *SDL_SYS_CreateTempFolder(void) | ||
| { | ||
| /* TODO: Check for possible alternatives to /tmp, like $TMP */ | ||
| char template[] = "/tmp/tmp.XXXXXX"; | ||
|
Comment on lines
+72
to
+73
There was a problem hiding this comment. Same comments as for the function that uses |
||
|
|
||
| char *folder = SDL_strdup(template); | ||
|
|
||
| if (!folder) { | ||
| return NULL; | ||
| } | ||
|
|
||
| char *res = mkdtemp(folder); | ||
|
|
||
| if (!res) { | ||
| SDL_free(folder); | ||
| SDL_SetError("Could not mkdtemp(): %s", strerror(errno)); | ||
| return NULL; | ||
| } | ||
|
|
||
| return folder; | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function, as documented here, is too dangerous to be useful: the author of a SDL game cannot know whether their game will be run on a multi-user system where another user is attempting to carry out a symlink attack.
Instead, I would suggest modelling it on Python's
NamedTemporaryFile, which returns (an object containing) a file handle and a path:For example a good signature might be:
SDL_IOStream *SDL_CreateNamedTempFile(char **name_out)(and perhapsSDL_IOStream *SDL_CreateTempFile(void)could just be implemented as syntactic sugar forSDL_CreateNamedTempFile(NULL)).https://docs.gtk.org/glib/func.mkstemp.html is another implementation of the same idea.
It would be a good idea to document what you can safely do with the temporary file by its path (read it, edit it in-place, or overwrite it), and what you cannot safely do with it (after you delete it, even if it's only for a moment, it is no longer safe to reuse that path because an attacker could have hijacked it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The moment paths are involved, a temporary file is no longer "safe" in at least one way, because other processes can refer to it and at least read it/write to it (I believe no environment enforce file locks? Linux provide locks, but they require voluntary implementation). I'm worried about developers thinking that the function is merely a more convenient path-based alternative to the current
SDL_CreateSafeTempFile, rather than an insecure alternative to be used by APIs that are inherently insecure, like AppIndicator.I would prefer to stick to the "safe"/"unsafe" naming to make sure this is properly conveyed to people who eagerly copy-paste code without reading the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What threat model do you have in mind here - what are you assuming that the attacker can and can't do? (It isn't ever really possible for a component to be "secure" in the abstract, but it can be secure against a specific threat model.)
Other processes that are running as the same user can do that, yes; but in the typical Unix security model, there is already no security boundary between processes that are running as the same user. They can ptrace() or kill() each other; they are reading and trusting the same configuration files in $HOME and state in $XDG_RUNTIME_DIR; IPC services like X11, Wayland and D-Bus treat them as essentially equivalent; and so on. The threat model that Unix is/was designed for is that you trust yourself and root, but other users are assumed to be hostile.
Other users can be prevented from reading and writing files by setting appropriate permissions on those files, so if your threat model is that the attacker is running code as a different user ID, "all" we need to do is to avoid time-of-check/time-of-use attacks. Once the file is in place and has our desired permissions, other users can't mess with it - unless we delete it, in which case we no longer "own" that path and all bets are off.
If you have a different threat model in mind, then please describe it, and we should evaluate these APIs against that threat model as well, but it's entirely possible that the answer will be: if that is your threat model, then you have already lost, because the OS was not designed for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, mandatory locking isn't implemented on Unix, and the advisory locking that does exist is a data-integrity mechanism rather than a security mechanism (it doesn't prevent an adversary from attacking you, all it does is to let two cooperating processes agree on how to avoid data loss).
Linux used to have mandatory locking on some filesystems, but it was removed, because it was considered to be a denial-of-service security vulnerability (a malicious user with read access could prevent another user from accessing files).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, on Linux at least, there are some kernel settings that can provide better protection. For example, the YAMA module provides
/proc/sys/kernel/yama/ptrace_scope, which allows different levels of restrictions on which process can ptrace which other. It provides 4 levels of security (0 = any process with the same uid, 1 = only parent processes, 2 = only processes withCAP_SYS_PTRACE, 3 = deactivated entirely). On my Ubuntu 24.04, it defaults to a value of 1.The linked documentation states, towards the beginning, some of the issues with processes having the ability to read the memory of other processes with the same uid. That's the threat I'd like to avoid: leaking potentially sensitive information to other processes.
ptraceseems to be mitigated to a certain degree on modern systems, andkillwouldn't leak information from temporary files created withtmpfile(), which deletes on close. Other possible attacks have their own mitigation, though all those I can think of is within the control of the OS, the environment, the app developer, the end user, or an entity other than SDL.I understand SDL can't change how operating systems work, but operating systems may provide settings to adjust how they work. However, they can't reasonably provide settings to change how SDL works, so if we choose to ignore the problem and leave the issue there, there isn't much that developers and end users can do about it, short of patching SDL manually. I'd rather have SDL not be the cause of the issue, even if some OSes will hold some issues out of SDL's control.