At least objcopy and perhaps other utilities generate a temp file safely with mkstemp then rename it to atomically replace the original, but the smart_rename function (binutils/rename.c) used to do this then performs chown and chmod on the target pathname rather than fchown/fchmod on the file. This is subject to all the classic symlink race attacks and can be used to get root to chown or chmod arbitrary files. In a worst case, with multiple racing file replacements, it can be used to chmod arbitrary root-owned files suid. This is only an issue if the file being operated on is in a directory writable by the attacker. However, the whole point of the ownership/permissions restoration logic seems to be for the case where root is operating on other users' files, and it seems likely that the directory will also be user-owned. I'm reporting this through public issue rather than security because I don't think there are direct ways to exploit it programmatically in a normal environment. There may be when the affected tools are used in automated scripts acting on user-owned files, though.
Hi Rich, > At least objcopy and perhaps other utilities generate a temp file safely > with mkstemp then rename it to atomically replace the original, but the > smart_rename function (binutils/rename.c) used to do this then performs > chown and chmod on the target pathname rather than fchown/fchmod on the > file. This is subject to all the classic symlink race attacks and can be > used to get root to chown or chmod arbitrary files. In a worst case, with > multiple racing file replacements, it can be used to chmod arbitrary > root-owned files suid. Not being a security expert, please can I check a couple of things with you: The code in smart_rename() has already checked that the destination file is not a symbolic link, so presumably the vulnerability occurs if the attacker is able to replace the destination file after the rename has happened but before the chmod() and/or chown() happen, right ? So in order to protect the destination file it needs to be opened first and then fchown/fchmod can be used. Bit isn't there still a period of vulnerability between the call to rename() and the call to fopen() ? Or is there a way to rename a file and open it at the same time ? Cheers Nick
It's not that smart_rename should open it and use fchown+fchmod on the result; indeed as you figured that has the same race. The problem is that you threw away the only safe way to perform modifications on the temp file you just created: the fd returned by mkstemp. In addition to the source and dest names, smart_rename needs to take the already-open file descriptors for source and dest that were used for the objcopy (or whatever) operation. Note: I missed this in the original report, but it's also important to use fstat on the file that will be replaced, rather than lstat on the pathname, to ensure the permissions that will be set on the new file are the ones of the file that was processed, and not some other file that was substituted in its place. Failure to do this right may not be exploitable, since the hard link count is checked, but it's still possibly erroneous.
Created attachment 13003 [details] Proposed patch Hi Rich, Ok - here is my first attempt at creating a patch. Please could you give it a look over ? There is one place where smart_rename() is called without having a previous call to make_tempname(): arsup.c:ar_save(). I am not sure if represents a possible attack vector, so any advice would be appreciated. Cheers Nick
That's a lot to review without being familiar with the code, but a couple things I can tell you right off: 1. make_tempname with fd_return==NULL is *always* a bug, and defeats the whole purpose of mkstemp. It's the same as if you were using the deprecated insecure mktemp. Except in a directory that nobody else can write, or with sticky bit, you can never again be sure the name refers to the file you created. 2. smart_rename needs(*) *two* fds, not just one. It needs the fd of the file you're replacing, to get the ownership and mode from it via fstat, and the fd of the temp file it will be renaming over top of the old name, to set the ownership and mode via fchown and fchmod. If either of these is unavailable it can't safely copy ownership or mode information. * Technically the caller could have called fstat on the original file being replaced already, and pass the owner/mode information (or the whole stat structure) into smart_rename rather than passing the fd, but I think it makes more sense to just pass the fd.
Created attachment 13004 [details] Proposed patch Hi Rich, > 1. make_tempname with fd_return==NULL is *always* a bug, Well, I would argue that it is only a bug if the temporary file is going to be manipulated by other system functions. But it is also true that if a caller does not want the file descriptor they can always close/discard it. So I am attaching an updated patch which adds ATTRIBUTE_NONNULL to both of the parameters to make_tempname(). That way the compiler will ensure that any caller always receives the file descriptor. > 2. smart_rename needs(*) *two* fds, not just one Actually it does this. It is just that only one fd is passed in (the open temporary file). The function itself opens the destination file before it attempts to perform any other operations on it. Cheers Nick
Created attachment 13005 [details] Proposed patch Siddhesh Poyarekar pointed out a bug in the previous patches. I was calling fchmod() and fchown() on the TO file descriptor (after calling rename(2)), rather than the FROM file descriptor. This upload contains a revised patch that corrects this bug.
Created attachment 13006 [details] Third revision of patch
Created attachment 13007 [details] Fourth revision of patch Gah! The third version was still wrong, because it was trying to set the permissions of the FROM file descriptor onto the To file....
> > 1. make_tempname with fd_return==NULL is *always* a bug, > Well, I would argue that it is only a bug if the temporary file is going to be manipulated by other system functions. Are you going to create a temp file but never write anything to it? That seems pointless. As soon as you open it for write, you have bug, and probably an exploitable one. There is fundamentally no way of knowing it's the same temp file you created anymore since the only reference you retained to it is a name, which is ephemeral. > > 2. smart_rename needs(*) *two* fds, not just one > Actually it does this. It is just that only one fd is passed in (the open temporary file). The function itself opens the destination file before it attempts to perform any other operations on it. It doesn't. The second file descriptor I'm talking about is the one to the original file that was read from, that will be replaced by the rename. This is the one you need to read old ownership/mode from with fstat. Using the name here is not safe (wrt using the right data). As long as you use O_NOFOLLOW and check that the link count is 1, I don't think it's *exploitably* wrong, but I wouldn't want to bet on that. It's still conceptually wrong and should just be done the right way.
(In reply to Rich Felker from comment #9) Hi Rich, > It doesn't. The second file descriptor I'm talking about is the one to the > original file that was read from, that will be replaced by the rename. This > is the one you need to read old ownership/mode from with fstat. Using the > name here is not safe (wrt using the right data). OK - so would it be sufficient to pass in to smart_rename() a stat structure of the source file, obtained when the source file was open ? (And use that stat structure, obviously). The issue is that at the moment the callers of smart_rename close their input file prior to invoking the function, and restructuring the code to keep the input open is likely to lead to more problems. Cheers Nick
Yes, see my footnote in comment 4. That works just as well.
Created attachment 13016 [details] Fifth patch revision Hi Rich, OK, here is a revised patch that runs stat() on the open source file and then passes the resulting structure to smart_rename() for use when setting the mode and owners. How do you feel about this version ? Cheers Nick
This patch keeps expanding way beyond the scope of what I can commit to review, and I don't think it's nearing something that fixes the problem. It's not clear to me if bfd_stat uses fstat on the open file (safe) or stat on the name (unsafe) and I could go look this up, but this is ballooning into something much larger than I can take on and after I look that up there will be something else and something else. You really need someone who understands both this code and temp file/writable-dir TOCTOU race vulnerabilities to work with you on the fix. Short of that, the only thing I can say with confidence is that the chmod/chown behavior should be removed entirely (and even then I'm not sure it'd be safe to run in directories other users can write to, but at least it wouldn't explicitly be offering a feature meant only for that use case and encouraging users to use it that way). On major bug I will comment on individually -- in smart_rename you added a line: to_fd = open (to, O_WRONLY | O_TRUNC | O_BINARY, 0777); I don't understand the motivation for that at all, but it's creating a *completely new vulnerability* of the type we're discussing here. (And even when it's not being maliciously exploited, it will cause data loss whenever the file is a hardlink!) It looks like you're trying to avoid calling stat on the target because I told you that's unsafe, but instead you're truncating the target, which might be a symlink or hardlink to any file on the whole filesystem, *then* checking, after truncating it, whether it has any hardlinks. Here you really do want to stick with lstat. You're not using the result to copy permissions, just to see if the target you're about to replace would be a hard link or symlink, in which case I think the idea is to overwrite the existing file rather than replacing it so the linked status is preserved (at the cost of atomicity). There inherently will be races where you do "the wrong thing" here, and opening the file first doesn't do anything to avoid that. But they should not be "exploitable" for anything beyond annoyance.
I've posted a patch series on the list[1][2][3][4] that should resolve this. In summary, smart_rename now takes an FD for the file to rename and a struct describing the ownership and timestamps to fix up. Both are derived by callers from the BFDs that were either read from or written and hence should be safe for smart_rename to use in fixing up permissions using fchown/fchmod. The mkstemp returned FD is also now used instead of being closed and reopened. There are a number of other cases where reusing the stat buffer or file descriptors might just be quicker but I've avoided touching those for now since they are not directly related to this specific fix. [1] https://sourceware.org/pipermail/binutils/2020-December/114390.html [2] https://sourceware.org/pipermail/binutils/2020-December/114391.html [3] https://sourceware.org/pipermail/binutils/2020-December/114392.html [4] https://sourceware.org/pipermail/binutils/2020-December/114393.html
This is fixed in master: commit 014cc7f849e8209623fc99264814bce7b3b6faf2 (origin/master, origin/HEAD) Author: Siddhesh Poyarekar <siddhesh@gotplt.org> Date: Mon Dec 7 20:48:33 2020 +0530 binutils: Make smart_rename safe too smart_rename is capable of handling symlinks by copying and it also tries to preserve ownership and permissions of files when they're overwritten during the rename. This is useful in objcopy where the file properties need to be preserved. However because smart_rename does this using file names, it leaves a race window between renames and permission fixes. This change removes this race window by using file descriptors from the original BFDs that were used to manipulate these files wherever possible. The file that is to be renamed is also passed as a file descriptor so that we use fchown/fchmod on the file descriptor, thus making sure that we only modify the file we have opened to write. Further, in case the file is to be overwritten (as is the case in ar or objcopy), the permissions that need to be restored are taken from the file descriptor that was opened for input so that integrity of the file status is maintained all the way through to the rename. binutils/ * rename.c * ar.c (write_archive) [!defined (_WIN32) || defined (__CYGWIN32__)]: Initialize TARGET_STAT and OFD to pass to SMART_RENAME. * arsup.c (ar_save) [defined (_WIN32) || defined (__CYGWIN32__)]: Likewise. * bucomm.h (smart_rename): Add new arguments to declaration. * objcopy.c (strip_main)[defined (_WIN32) || defined (__CYGWIN32__)]: Initialize COPYFD and pass to SMART_RENAME. (copy_main) [defined (_WIN32) || defined (__CYGWIN32__)]: Likewise. * rename.c (try_preserve_permissions): New function. (smart_rename): Use it and add new arguments. commit 1a1c3b4cc17687091cff5a368bd6f13742bcfdf8 Author: Siddhesh Poyarekar <siddhesh@gotplt.org> Date: Mon Dec 7 20:48:28 2020 +0530 objcopy: Get input file stat after BFD open Get file state from the descriptor opened by copy_file for the input BFD. This ensures continuity in the view of the input file through the descriptor. At the moment it is only to preserve timestamps recorded at the point that we opened the file for input but in the next patch this state will also be used to preserve ownership and permissions wherever applicable. binutils/ * objcopy.c (copy_file): New argument IN_STAT. Return stat of ibfd through it. (strip_main): Remove redundant stat calls. adjust copy_file calls. (copy_main): Likewise. commit 365f5fb6d0f0da83817431a275e99e6f6babbe04 Author: Siddhesh Poyarekar <siddhesh@gotplt.org> Date: Mon Dec 7 20:48:23 2020 +0530 binutils: Use file descriptors from make_tempname The purpose of creating a temporary file securely using mkstemp is defeated if it is closed in make_tempname and reopened later for use; it is as good as using mktemp. Get the file descriptor instead and then use it to create the BFD object. bfd/ * opncls.c (bfd_fdopenw): New function. * bfd-in2.h: Regenerate. binutils/ * bucomm.c (make_tempname): Add argument to return file descriptor. * bucomm.h (make_tempname): Likewise. * ar.c: Include libbfd.h. (write_archive): Adjust for change in make_tempname. Call bfd_fdopenw instead of bfd_openw. * objcopy.c: Include libbfd.h. (copy_file): New argument OFD. Use bfd_fdopenw instead of bfd_openw. (strip_main): Adjust for change in make_tempname and copy_file. (copy_main): Likewise.
Unfortunately it looks like the patches applied for this bug failed to take into account bfd/cache.c twiddling of bfd->iostream.
Ugh, I thought I did, but clearly I overlooked the ar use cases. I'll take a closer look.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=95b91a043aeaeb546d2fea556d84a2de1e917770 commit 95b91a043aeaeb546d2fea556d84a2de1e917770 Author: Alan Modra <amodra@gmail.com> Date: Mon Feb 1 02:04:41 2021 +1030 pr27270 and pr27284, ar segfaults and wrong file mode PR 27270 PR 27284 PR 26945 * ar.c: Don't include libbfd.h. (write_archive): Replace xmalloc+strcpy with xstrdup. Use bfd_stat rather than fstat on iostream. Move stat and fd tests outside of _WIN32 ifdef. Delete skip_stat variable. * arsup.c (temp_name, real_ofd): New static variables. (ar_open): Use make_tempname and bfd_fdopenw. (ar_save): Adjust to suit ar_open changes. Move stat output of _WIN32 ifdef. * objcopy.c: Don't include libbfd.h. (copy_file): Use bfd_stat.
The binutils-2_36-branch branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6184480d7ce1bcd57669a62867efc68418d0de7c commit 6184480d7ce1bcd57669a62867efc68418d0de7c Author: Alan Modra <amodra@gmail.com> Date: Mon Feb 1 02:04:41 2021 +1030 pr27270 and pr27284, ar segfaults and wrong file mode PR 27270 PR 27284 PR 26945 * ar.c: Don't include libbfd.h. (write_archive): Replace xmalloc+strcpy with xstrdup. Use bfd_stat rather than fstat on iostream. Move stat and fd tests outside of _WIN32 ifdef. Delete skip_stat variable. * arsup.c (temp_name, real_ofd): New static variables. (ar_open): Use make_tempname and bfd_fdopenw. (ar_save): Adjust to suit ar_open changes. Move stat output of _WIN32 ifdef. * objcopy.c: Don't include libbfd.h. (copy_file): Use bfd_stat. (cherry picked from commit 95b91a043aeaeb546d2fea556d84a2de1e917770)
Should now be fixed
The binutils-2_36-branch branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d3edaa91d4cf7202ec14342410194841e2f67f12 commit d3edaa91d4cf7202ec14342410194841e2f67f12 Author: Alan Modra <amodra@gmail.com> Date: Fri Feb 26 11:30:32 2021 +1030 Reinstate various pieces backed out from smart_rename changes In the interests of a stable release various last minute smart_rename patches were backed out of the 2.36 branch. The main reason to reinstate some of those backed out changes here is to make necessary followup fixes to commit 8e03235147a9 simple cherry-picks from mainline. A secondary reason is that ar -M support isn't fixed for pr26945 without this patch. PR 26945 * ar.c: Don't include libbfd.h. (write_archive): Replace xmalloc+strcpy with xstrdup. * arsup.c (temp_name, real_ofd): New static variables. (ar_open): Use make_tempname and bfd_fdopenw. (ar_save): Adjust to suit ar_open changes. * objcopy.c: Don't include libbfd.h. * rename.c: Rename and reorder variables. (cherry picked from commit 95b91a043aeaeb546d2fea556d84a2de1e917770)
(In reply to cvs-commit@gcc.gnu.org from comment #21) > The binutils-2_36-branch branch has been updated by Alan Modra > <amodra@sourceware.org>: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git; > h=d3edaa91d4cf7202ec14342410194841e2f67f12 > > commit d3edaa91d4cf7202ec14342410194841e2f67f12 > Author: Alan Modra <amodra@gmail.com> > Date: Fri Feb 26 11:30:32 2021 +1030 > > Reinstate various pieces backed out from smart_rename changes > > In the interests of a stable release various last minute smart_rename > patches were backed out of the 2.36 branch. The main reason to > reinstate some of those backed out changes here is to make necessary > followup fixes to commit 8e03235147a9 simple cherry-picks from > mainline. A secondary reason is that ar -M support isn't fixed for > pr26945 without this patch. > > PR 26945 > * ar.c: Don't include libbfd.h. > (write_archive): Replace xmalloc+strcpy with xstrdup. > * arsup.c (temp_name, real_ofd): New static variables. > (ar_open): Use make_tempname and bfd_fdopenw. > (ar_save): Adjust to suit ar_open changes. > * objcopy.c: Don't include libbfd.h. > * rename.c: Rename and reorder variables. > > (cherry picked from commit 95b91a043aeaeb546d2fea556d84a2de1e917770) Hi, can we backport this patch to binutils-2_34-branch ?
(In reply to John Dong from comment #22) > Hi, can we backport this patch to binutils-2_34-branch ? You can of course do what you like with your own copy of binutils, but I don't consider this series of patches appropriate for backporting to earlier FSF binutils branches.