[PATCH v2 0/2] um: Protect memory mapped file

[PATCH v2 0/2] um: Protect memory mapped file

[Mickaël Salaün]

This series protect the memory mapped file.

Changes since v1; addressed Richard Weinberger's comments:
* add attacker model to the patch description [1/2]
* remove errno reset [2/2]

Regards,
 Mickaël

Mickaël Salaün (2):
  um: Set secure access mode for temporary file
  um: Use race-free temporary file creation

 arch/um/os-Linux/mem.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.6.2

[PATCH v2 0/2] um: Protect memory mapped file

[Mickaël Salaün]

This series protect the memory mapped file.

Changes since v1; addressed Richard Weinberger's comments:
* add attacker model to the patch description [1/2]
* remove errno reset [2/2]

Regards,
 Mickaël

Mickaël Salaün (2):
  um: Set secure access mode for temporary file
  um: Use race-free temporary file creation

 arch/um/os-Linux/mem.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.6.2

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

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

[Mickaël Salaün]

Replace the insecure mode 0777 with 0700 for temporary file to prohibit
other users to change the executable mapped code.

An attacker could gain access to the mapped file descriptor from the
temporary file (before it is unlinked) in a read-only mode but it should
not be accessible in write mode to avoid arbitrary code execution.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 arch/um/os-Linux/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 897e9ad..798aeb4 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -142,7 +142,7 @@ static int __init create_tmp_file(unsigned long long len)
 	if (fd < 0)
 		exit(1);
 
-	err = fchmod(fd, 0777);
+	err = fchmod(fd, 0700);
 	if (err < 0) {
 		perror("fchmod");
 		exit(1);
-- 
2.6.2

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

[Mickaël Salaün]

Replace the insecure mode 0777 with 0700 for temporary file to prohibit
other users to change the executable mapped code.

An attacker could gain access to the mapped file descriptor from the
temporary file (before it is unlinked) in a read-only mode but it should
not be accessible in write mode to avoid arbitrary code execution.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 arch/um/os-Linux/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 897e9ad..798aeb4 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -142,7 +142,7 @@ static int __init create_tmp_file(unsigned long long len)
 	if (fd < 0)
 		exit(1);
 
-	err = fchmod(fd, 0777);
+	err = fchmod(fd, 0700);
 	if (err < 0) {
 		perror("fchmod");
 		exit(1);
-- 
2.6.2

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

[PATCH v2 2/2] um: Use race-free temporary file creation

[Mickaël Salaün]

Open the memory mapped file with the O_TMPFILE flag when available.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 arch/um/os-Linux/mem.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 798aeb4..6ee4233 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -106,6 +106,17 @@ static int __init make_tempfile(const char *template)
 		}
 	}
 
+#ifdef O_TMPFILE
+	fd = open(tempdir, O_CLOEXEC | O_RDWR | O_EXCL | O_TMPFILE, 0700);
+	/*
+	 * If the running system does not support O_TMPFILE flag then retry
+	 * without it.
+	 */
+	if (fd != -1 || (errno != EINVAL && errno != EISDIR &&
+			errno != EOPNOTSUPP))
+		return fd;
+#endif
+
 	tempname = malloc(strlen(tempdir) + strlen(template) + 1);
 	if (tempname == NULL)
 		return -1;
-- 
2.6.2

[PATCH v2 2/2] um: Use race-free temporary file creation

[Mickaël Salaün]

Open the memory mapped file with the O_TMPFILE flag when available.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 arch/um/os-Linux/mem.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 798aeb4..6ee4233 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -106,6 +106,17 @@ static int __init make_tempfile(const char *template)
 		}
 	}
 
+#ifdef O_TMPFILE
+	fd = open(tempdir, O_CLOEXEC | O_RDWR | O_EXCL | O_TMPFILE, 0700);
+	/*
+	 * If the running system does not support O_TMPFILE flag then retry
+	 * without it.
+	 */
+	if (fd != -1 || (errno != EINVAL && errno != EISDIR &&
+			errno != EOPNOTSUPP))
+		return fd;
+#endif
+
 	tempname = malloc(strlen(tempdir) + strlen(template) + 1);
 	if (tempname == NULL)
 		return -1;
-- 
2.6.2

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

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

[Tristan Schmelcher]

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?

Re: [PATCH v2 2/2] um: Use race-free temporary file creation

[Tristan Schmelcher]

Acked-by: Tristan Schmelcher <tschmelcher@google.com>

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

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

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

> 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;
}

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

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

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

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

[Richard Weinberger]

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

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

[Richard Weinberger]

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

------------------------------------------------------------------------------
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

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

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]