From: Jérémie Galarneau Date: Fri, 14 Aug 2020 20:59:18 +0000 (-0400) Subject: sessiond: enforce user-exclusive session access in session_access_ok X-Git-Tag: v2.13.0-rc1~493 X-Git-Url: https://git.lttng.org./?a=commitdiff_plain;h=d7b377ed1acd4043bde353d99122e0e56fa4e975;p=lttng-tools.git sessiond: enforce user-exclusive session access in session_access_ok The current session_access_ok logic disallows the access to a session when: uid != session->uid && gid != session->gid && uid != 0 This means that any user that is part of the same primary group as the session's owner can access the session. The primary group is not necessarily (and most likely) not the `tracing` group. Moreover, the `tracing` group is not meant to provide shared access to sessions, but to allow interactions with a root session daemon. For instance: - the session has uid = 1000, gid = 100 - the current user has uid = 1001, gid = 100 access to the session is granted. This is way too broad and unexpected from most users as the LTTng documentation never mentions this "primary group share tracing sessions" behaviour. The documentation only alludes to the fact that separate users have "their own set of sessions". On most distributions, this change will have no impact as `useradd` creates a new group for every user. Users will never share a primary group and thus can't control each others' sessions. However, it is not unusual to have users share a primary group (e.g. `users`) and set the default umask to `0700`. In that case, there is no expectation that every user will share files and there would be no reasonable expectation that they should share all sessions. For instance, it would be unexpected for one user to tear down the sessions of other users with a single `lttng destroy -a` command. If this type of session sharing is desirable to some users, then the default umask of users could be checked or sessions could be created as part of a group. However, in doubt, it is preferable to be strict. This is not marked as a fix since this was most likely deliberate and the change could, although unlikely, break existing deployment scenarios. Signed-off-by: Jérémie Galarneau Change-Id: I98f7ffb29d5f6dcb9d660535c1d3f5a1d1a68293 --- diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c index 689dc258a..8a2ef85b7 100644 --- a/src/bin/lttng-sessiond/client.c +++ b/src/bin/lttng-sessiond/client.c @@ -579,15 +579,14 @@ static unsigned int lttng_sessions_count(uid_t uid, gid_t gid) struct ltt_session *session; const struct ltt_session_list *session_list = session_get_list(); - DBG("Counting number of available session for UID %d GID %d", - uid, gid); + DBG("Counting number of available session for UID %d", uid); cds_list_for_each_entry(session, &session_list->head, list) { if (!session_get(session)) { continue; } session_lock(session); /* Only count the sessions the user can control. */ - if (session_access_ok(session, uid, gid) && + if (session_access_ok(session, uid) && !session->destroyed) { i++; } @@ -1106,13 +1105,12 @@ skip_domain: } /* - * Check that the UID or GID match that of the tracing session. + * Check that the UID matches that of the tracing session. * The root user can interact with all sessions. */ if (need_tracing_session) { if (!session_access_ok(cmd_ctx->session, - LTTNG_SOCK_GET_UID_CRED(&cmd_ctx->creds), - LTTNG_SOCK_GET_GID_CRED(&cmd_ctx->creds)) || + LTTNG_SOCK_GET_UID_CRED(&cmd_ctx->creds)) || cmd_ctx->session->destroyed) { ret = LTTNG_ERR_EPERM; goto error; diff --git a/src/bin/lttng-sessiond/cmd.c b/src/bin/lttng-sessiond/cmd.c index b1b22012c..dc442035a 100644 --- a/src/bin/lttng-sessiond/cmd.c +++ b/src/bin/lttng-sessiond/cmd.c @@ -3715,7 +3715,7 @@ void cmd_list_lttng_sessions(struct lttng_session *sessions, /* * Only list the sessions the user can control. */ - if (!session_access_ok(session, uid, gid) || + if (!session_access_ok(session, uid) || session->destroyed) { session_put(session); continue; diff --git a/src/bin/lttng-sessiond/save.c b/src/bin/lttng-sessiond/save.c index 09194008a..052928c89 100644 --- a/src/bin/lttng-sessiond/save.c +++ b/src/bin/lttng-sessiond/save.c @@ -2633,8 +2633,7 @@ int save_session(struct ltt_session *session, memset(config_file_path, 0, sizeof(config_file_path)); if (!session_access_ok(session, - LTTNG_SOCK_GET_UID_CRED(creds), - LTTNG_SOCK_GET_GID_CRED(creds)) || session->destroyed) { + LTTNG_SOCK_GET_UID_CRED(creds)) || session->destroyed) { ret = LTTNG_ERR_EPERM; goto end; } diff --git a/src/bin/lttng-sessiond/session.c b/src/bin/lttng-sessiond/session.c index 95395c282..f8134f8ad 100644 --- a/src/bin/lttng-sessiond/session.c +++ b/src/bin/lttng-sessiond/session.c @@ -1296,18 +1296,13 @@ error: } /* - * Check if the UID or GID match the session. Root user has access to all + * Check if the UID matches the session. Root user has access to all * sessions. */ -int session_access_ok(struct ltt_session *session, uid_t uid, gid_t gid) +bool session_access_ok(struct ltt_session *session, uid_t uid) { assert(session); - - if (uid != session->uid && gid != session->gid && uid != 0) { - return 0; - } else { - return 1; - } + return (uid == session->uid) || uid == 0; } /* diff --git a/src/bin/lttng-sessiond/session.h b/src/bin/lttng-sessiond/session.h index 34e51fe5a..e6ed40cc2 100644 --- a/src/bin/lttng-sessiond/session.h +++ b/src/bin/lttng-sessiond/session.h @@ -240,7 +240,7 @@ struct ltt_session *session_find_by_id(uint64_t id); struct ltt_session_list *session_get_list(void); void session_list_wait_empty(void); -int session_access_ok(struct ltt_session *session, uid_t uid, gid_t gid); +bool session_access_ok(struct ltt_session *session, uid_t uid); int session_reset_rotation_state(struct ltt_session *session, enum lttng_rotation_state result);