Re: [PATCH v2 1/2] um: Set secure access mode for temporary file

[Tristan Schmelcher <tschmelcher@google.com>]

On 6 December 2015 at 09:43, Mickaël Salaün <mic@digikod.net> wrote:
> Well, I'm concerned to use umask because it is not thread-safe and drivers may use create_mem_file() in a multi-theaded context.

You are right. We should perhaps set the umask to 0700 permanently
during process start. But I am not sure if this will interfere with
other UML code.

> I prefer to stick to fchmod and handle the race-condition with O_TMPFILE unsell someone is sure that this will not create bugs :)

The fchmod call is basically useless and should probably be removed.
Even mmap only checks the file descriptor, not the file permissions. I
have pasted a test program below if you wish to confirm. AFAICT
changing the permissions after file deletion accomplishes nothing
unless the attacker bizarrely chooses to hard-link the file during the
race instead of opening it.

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

int main(int argc, char **argv) {
  int fd = open("./foo", O_RDWR|O_CREAT|O_EXCL, 0700);
  assert(fd >= 0);
  int ret = write(fd, "bar\n", 4);
  assert(ret == 4);
  ret = fchmod(fd, 0400);
  assert(ret >= 0);
  char *buf = mmap(0, 4, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_SHARED, fd, 0);
  assert(buf);
  buf[2] = 'z';
  ret = munmap(buf, 4);
  assert(ret >= 0);
  return 0;
}