GNOME Bugzilla – Bug 358894
Security: race conditions in temporary file creation.
Last modified: 2006-11-05 20:25:45 UTC
The way that temporary files are created in apply_transformation_jpeg() in src/rotation-utils.c is vulnerable to race conditions, which could allow an attacker to overwrite arbitrary files with the permissions of the user running gthumb. The functions get_temp_dir_name() and get_temp_file_name() in libgthumb/file-utils.c, as well as get_temp_filename() in src/dlg-photo-importer.c, are also vulnerable to race conditions. For more information on this type of vulnerability, see http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html#TEMPORARY-FILES Patch to follow.
Created attachment 73823 [details] [review] Fix for race conditions in temporary file creation This should fix the race conditions in src/dlg-photo-importer.c, src/gth-window-actions-callbacks.c and src/rotation-utils.c. If there are any other places where temporary files are created, let me know and I'll take a look at them.
Created attachment 74377 [details] [review] Fix for race conditions in temporary file creation (v2) Update of the original patch to apply cleanly to the latest CVS.
Rennie, I've been trying to understand your patch. I see how the get_temp_file_name function is better now with the use of mutex locks, but I don't understand why the new get_temp_dir_name is better than the old one. Can you clarify that? Also, CVS has changed again (sorry!). Could you update your patch? I can commit now, so I can promise that you won't have to re-work your patch again because of CVS changes :-) - Mike
Created attachment 76050 [details] [review] Fix for race conditions in temporary file creation (v3) The problem is that currently, gthumb creates temporary files with predictable names in /tmp, which is world-writable. It checks that the file doesn't exist, and if not, then it creates it - but this allows a race condition where an attacker can guess the name of the temporary file that you're about to create and (sym)link it to some other file on the system after you check that it doesn't exist but before you open it, causing you to overwrite an arbitrary file with your permissions. More details on this type of attack are available at the link I gave above. There are generally two ways to avoid this type of attack: 1. use a function like mkstemp() to create a file descriptor, and be careful how you use the file name that it generates, or 2. ensure that temporary files are only created in directories to which only the current user may write. gthumb would require a major overhaul to use solution 1 (although in general, it is the best solution), so I opted for solution 2. g_get_tmp_dir() may return a temporary path inside the user's home directory, in which case there is no problem with files created inside it, but in many cases, g_get_tmp_dir() will simply return /tmp. The new get_temp_dir_name() creates a directory to which only the current user may write, so an attacker can't mess with any temporary files created therein. The intention is that temporary files be created only inside this directory. An attacker sill could try replacing this temporary directory with a link to some other directory, causing you to overwrite files in that other directory, but only if the directory was in a non-sticky world-writable parent directory (/tmp is sticky). As usual, there's no way to protect users from their own stupidity. The mutex in get_temp_file_name() only protects against gthumb generating the same temporary file name twice; it doesn't address the race condition with another process that leads to the security problem. I've updated my patch to apply cleanly to the latest CVS. Rennie
Rennie, Thanks for the explanations! Patch committed. - Mike