Fix: agent port file is o+w when launching as root

Observed issue
==============

When starting as root, the following permissions are observed:

[-rw-rw-rw-]  agent.port
[-rw-r--r--]  lttng-sessiond.pid

When starting as user:

[-rw-rw----]  agent.port
[-rw-rw-r--]  lttng-sessiond.pid

Note that despite being created by the same function,
`utils_create_pid_file`, the permissions are not the same.

Cause
=====

`get_wait_shm` manipulates the umask and does not restore it, thus
influencing the outcome of following file creations that don't enforce
specific permissions (using chmod).

Also `fopen` defaults to mode `0666 & ~umask`, thus resulting in
unnecessarily lax permissions when the session daemon is started as a
non-privileged user (umask = 0002, most of the time).

Solution
========

Mimic other call sites of umask(), modify then revert the umask.

Open the pid and agent port files as 0644 letting the umask to do its
job as necessary for those files.

Remove unnecessary umask() usage when chmod is directly used.

Known drawbacks
===============

Use of umask in a multi-threaded process is not recommended. Still our
current usage is limited and mostly happens during the initialization
phase. The usage of umask() is required for the `wait_shm` since on
FreeBSD it is not possible to chmod an shm file descriptor. The default
umask would interfere here.

Discussion
==========

The usage in run-as is valid even when in no-clone mode (valgrind) since
it is the sole user of umask() following the initialization phase. When
spawned as a separate process the clearing of umask is totally valid
even if it is not ideal since we are ignoring any umask set by the user.

It seems like the current usage is the lesser evil here.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: Ie224d254714fff05f4bced471ebfa8f19eede26a

diff --git a/src/bin/lttng-sessiond/client.cpp b/src/bin/lttng-sessiond/client.cpp
index 1f94bdad8954f94a7aa06c1576dec4668b6141e0..a52062e07f877ddc08ccf84604a56c5f2ff1fd59 100644 (file)
--- a/src/bin/lttng-sessiond/client.cpp
+++ b/src/bin/lttng-sessiond/client.cpp
@@ -2396,7 +2396,6 @@ init_setup_error:
 static int create_client_sock(void)
 {
        int ret, client_sock;
-       const mode_t old_umask = umask(0);
 
        /* Create client tool unix socket */
        client_sock = lttcomm_create_unix_sock(
@@ -2430,7 +2429,6 @@ static int create_client_sock(void)
        DBG("Created client socket (fd = %i)", client_sock);
        ret = client_sock;
 end:
-       umask(old_umask);
        return ret;
 }
 

diff --git a/src/bin/lttng-sessiond/register.cpp b/src/bin/lttng-sessiond/register.cpp
index f6aaef623f23369328ec1fc61b1748b12ed177e0..34efeac40f57729fd0811806e56cb7665847fefc 100644 (file)
--- a/src/bin/lttng-sessiond/register.cpp
+++ b/src/bin/lttng-sessiond/register.cpp
@@ -41,7 +41,6 @@ static int create_application_socket(void)
 {
        int ret = 0;
        int apps_sock;
-       const mode_t old_umask = umask(0);
 
        /* Create the application unix socket */
        apps_sock = lttcomm_create_unix_sock(
@@ -74,7 +73,6 @@ static int create_application_socket(void)
        DBG3("Session daemon application socket created (fd = %d) ", apps_sock);
        ret = apps_sock;
 end:
-       umask(old_umask);
        return ret;
 error_close_socket:
        if (close(apps_sock)) {

diff --git a/src/common/shm.cpp b/src/common/shm.cpp
index e3fa9b6404c3ad6f85d99293f664aeac9aebefb9..300ffe357e98cafdfabb02146ab8736bc8474104 100644 (file)
--- a/src/common/shm.cpp
+++ b/src/common/shm.cpp
@@ -21,8 +21,7 @@
 #include "shm.hpp"
 
 /*
- * Using fork to set umask in the child process (not multi-thread safe). We
- * deal with the shm_open vs ftruncate race (happening when the sessiond owns
+ * We deal with the shm_open vs ftruncate race (happening when the sessiond owns
  * the shm and does not let everybody modify it, to ensure safety against
  * shm_unlink) by simply letting the mmap fail and retrying after a few
  * seconds. For global shm, everybody has rw access to it until the sessiond
@@ -31,7 +30,7 @@
 static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
 {
        int wait_shm_fd, ret;
-       mode_t mode;
+       mode_t mode, old_mode;
 
        LTTNG_ASSERT(shm_path);
 
@@ -51,11 +50,7 @@ static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
                mode |= S_IROTH | S_IWOTH;
        }
 
-       /*
-        * We're alone in a child process, so we can modify the process-wide
-        * umask.
-        */
-       umask(~mode);
+       old_mode = umask(~mode);
 
        /*
         * Try creating shm (or get rw access). We don't do an exclusive open,
@@ -104,7 +99,7 @@ static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
        if (ret < 0) {
                PERROR("Failed to truncate \"wait\" shared memory object: fd = %d, size = %zu",
                                wait_shm_fd, mmap_size);
-               exit(EXIT_FAILURE);
+               goto error;
        }
 
        if (global) {
@@ -112,7 +107,7 @@ static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
                if (ret < 0) {
                        PERROR("Failed to set ownership of \"wait\" shared memory object: fd = %d, owner = 0, group = 0",
                                        wait_shm_fd);
-                       exit(EXIT_FAILURE);
+                       goto error;
                }
                /*
                 * If global session daemon, any application can
@@ -124,14 +119,14 @@ static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
                if (ret < 0) {
                        PERROR("Failed to set the mode of the \"wait\" shared memory object: fd = %d, mode = %d",
                                        wait_shm_fd, mode);
-                       exit(EXIT_FAILURE);
+                       goto error;
                }
        } else {
                ret = fchown(wait_shm_fd, getuid(), getgid());
                if (ret < 0) {
                        PERROR("Failed to set ownership of \"wait\" shared memory object: fd = %d, owner = %d, group = %d",
                                        wait_shm_fd, getuid(), getgid());
-                       exit(EXIT_FAILURE);
+                       goto error;
                }
        }
 
@@ -139,13 +134,20 @@ static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
                        shm_path, mmap_size, global ? "true" : "false",
                        wait_shm_fd);
 
+end:
+       (void) umask(old_mode);
        return wait_shm_fd;
 
 error:
-       DBG("Failed to open shared memory file descriptor: path = '%s', mmap_size = %zu, global = %s",
-                       shm_path, mmap_size, global ? "true" : "false");
+       DBG("Failing to get the wait shm fd");
+       if (wait_shm_fd >= 0) {
+               if (close(wait_shm_fd)) {
+                       PERROR("Failed to close wait shm file descriptor during error handling");
+               }
+       }
 
-       return -1;
+       wait_shm_fd = -1;
+       goto end;
 }
 
 /*

diff --git a/src/common/utils.cpp b/src/common/utils.cpp
index b4b7f749f1ef1a6e5286225437f639d01f2966d2..6c0fd261c7b8a4033d5eb35cc7247af509ddfca3 100644 (file)
--- a/src/common/utils.cpp
+++ b/src/common/utils.cpp
@@ -212,30 +212,40 @@ end:
  */
 int utils_create_pid_file(pid_t pid, const char *filepath)
 {
-       int ret;
-       FILE *fp;
+       int ret, fd = -1;
+       FILE *fp = NULL;
 
        LTTNG_ASSERT(filepath);
 
-       fp = fopen(filepath, "w");
+       fd = open(filepath, O_CREAT | O_WRONLY, S_IRUSR |S_IWUSR | S_IRGRP | S_IROTH);
+       if (fd < 0) {
+               PERROR("open file %s", filepath);
+               ret = -1;
+               goto error;
+       }
+
+       fp = fdopen(fd, "w");
        if (fp == NULL) {
-               PERROR("open pid file %s", filepath);
+               PERROR("fdopen file %s", filepath);
                ret = -1;
+               close(fd);
                goto error;
        }
 
        ret = fprintf(fp, "%d\n", (int) pid);
        if (ret < 0) {
-               PERROR("fprintf pid file");
+               PERROR("fprintf file %s", filepath);
+               ret = -1;
                goto error;
        }
 
-       if (fclose(fp)) {
-               PERROR("fclose");
-       }
-       DBG("Pid %d written in file %s", (int) pid, filepath);
+       DBG("'%d' written in file %s", (int) pid, filepath);
        ret = 0;
+
 error:
+       if (fp && fclose(fp)) {
+               PERROR("fclose file %s", filepath);
+       }
        return ret;
 }