Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1284058 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Tristan Schmelcher <tschmelcher@google.com> - 2015-12-04 18:20 +0100
    Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Mickaël Salaün <mic@digikod.net> - 2015-12-06 13:00 +0100
    Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Mickaël Salaün <mic@digikod.net> - 2015-12-06 13:20 +0100
      Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Mickaël Salaün <mic@digikod.net> - 2015-12-06 16:00 +0100
        Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Tristan Schmelcher <tschmelcher@google.com> - 2015-12-08 21:40 +0100
          Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Richard Weinberger <richard@nod.at> - 2015-12-08 22:50 +0100
            Re: [PATCH v2 1/2] um: Set secure access mode for temporary file Mickaël Salaün <mic@digikod.net> - 2015-12-10 01:00 +0100

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

File permissions are checked at time of open, so I think this fchmod
call has never had any effect.

If there is a concern that the mkstemp implementation may be insecure,
why not set and restore the umask?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1284849

[Multipart message — attachments visible in raw view] — view raw

On 06/12/2015 12:32, Mickaël Salaün wrote:
> On 04/12/2015 18:13, Tristan Schmelcher wrote:
>> File permissions are checked at time of open, so I think this fchmod
>> call has never had any effect.
> 
> Indeed, mmap doesn't require the file to be executable, only readable and writable. The fchmod seems to be a guarantee for this permissions but I don't see why a file newly created could not be writable.

The fchmod may be useful if the umask is 0777 :)

 Mickaël

[toc] | [prev] | [next] | [standalone]

#1284850

[Multipart message — attachments visible in raw view] — view raw

On 04/12/2015 18:13, Tristan Schmelcher wrote:
> File permissions are checked at time of open, so I think this fchmod
> call has never had any effect.

Indeed, mmap doesn't require the file to be executable, only readable and writable. The fchmod seems to be a guarantee for this permissions but I don't see why a file newly created could not be writable.

> 
> If there is a concern that the mkstemp implementation may be insecure,
> why not set and restore the umask?
> 

I will add this safeguard.

 Mickaël

[toc] | [prev] | [next] | [standalone]

#1284872

[Multipart message — attachments visible in raw view] — view raw

On 06/12/2015 12:32, Mickaël Salaün wrote:
> On 04/12/2015 18:13, Tristan Schmelcher wrote:
>> If there is a concern that the mkstemp implementation may be insecure,
>> why not set and restore the umask?
>>
> 
> I will add this safeguard.

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. I prefer to stick to fchmod and handle the race-condition with O_TMPFILE unsell someone is sure that this will not create bugs :)

 Mickaël

[toc] | [prev] | [next] | [standalone]

#1286838

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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286866

Am 08.12.2015 um 21:37 schrieb Tristan Schmelcher:
> 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.

It *should* not hurt. Let's see what explodes. :)

>> 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.

I agree.

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288066

[Multipart message — attachments visible in raw view] — view raw

On 08/12/2015 22:45, Richard Weinberger wrote:
> Am 08.12.2015 um 21:37 schrieb Tristan Schmelcher:
>> 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.
> 
> It *should* not hurt. Let's see what explodes. :)

We can't force the kernel process umask without breaking compatibility with current implementation, especially with hostfs (i.e. the host umask prevail the guest one).

>>> 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.
> 
> I agree.

I propose then to simply remove the fchmod call and let the O_TMPFILE flag do the magic :)

Regards,
 Mickaël

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web