How to handle TOCTOU problem between access() and unlink()?

Question

A static-analysis tool (Coverity) flags the unlink() statement in the following code as having a time-of-check/time-of-use (TOCTOU) problem between the access() and unlink():

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

void del(const char* file) {
  if (! file) { return; }

  {
    struct stat ss;

    if (stat(file, &ss)) { return; }
    if (! S_ISREG(ss.st_mode)) { return; }
    if (ss.st_uid != getuid()) { return; }
    if (access(file, W_OK)) { return; }

    unlink(file);
  }
}

int main(int argc, char* argv[]) {
  const char* file = "./foo.txt";
  int fd = open(file, O_WRONLY | O_CREAT);

  close(fd);
  del(file);

  return 0;
}

Is it flagging the possibility that between the call to access() and unlink(), any other process on the device can come along and change the target file's permissions, so that the unlink() fails?

Is there a way that this situation can be handled to avoid the TOCTOU?
I'm at a loss how to "lock" the file for exclusive deletion-privilege by the current process.

TBH, I don't think I've seen code before that goes to these lengths to check if it's "okay" to delete a file -- most code of this sort that I've encountered just unconditionally call unlink() and move on. :)

"TBH, I don't think I've seen code before that goes to these lengths" - is it not your own code, then? In that case, why are you charged with fixing the reported issue, or why do you care about it? If it is your code - did you try just making the unconditional call? Did you try to [find out](https://meta.stackoverflow.com/questions/261592) what problems, if any, that would cause? — Karl Knechtel, Feb 28 '23 at 01:40

ikegami · Accepted Answer · 2023-02-28T02:29:43.623

Are those checks correct?

!S_ISREG(ss.st_mode)

This makes sure the file is a plain file. This isn't a redundant check since one can delete some files that aren't plain files.

If you're trying to make sure it doesn't delete a non-plain file, then the code has a TOCTOU race condition which is not easily resolvable.

Otherwise, if you don't care about the file's type, then it's wrong to perform the check since it incorrectly restricts which files can be deleted, and it should be removed.
ss.st_uid != getuid()

This makes sure the file is a plain file. This isn't a redundant check since one can delete files one doesn't own.

If you're trying to make sure it doesn't delete a file owned by someone else, then the code has a TOCTOU race condition which is not easily resolvable.

Otherwise, if you don't care who owns the file, then it's wrong to perform the check since it incorrectly restricts which files can be deleted, and it should be removed.
access(file, W_OK)

This makes the process has permission to write to the file. This check isn't redundant since one doesn't need permission to write to a file to delete it. (One needs permissions to write to the directory that contains it.)

If you're trying to make sure the process has permissions to write to the file, then the code has a TOCTOU race condition which is not easily resolvable.

Otherwise, if you don't care about this, then it's wrong to perform the check since it incorrectly restricts which files can be deleted, and it should be removed.

I suspect that when you fix all the bugs, you're left with

void del( const char* file ) {
   unlink( file );
}

If so, there's no TOCTOU race condition.

If I'm wrong and you really want to check those things, then I don't see a good solution. A race condition is virtually unavoidable without cooperation.

How to handle TOCTOU problem between access() and unlink()?

1 Answers1