Hi Thien,
Ack with comments inline. Thanks.
Regards, Vu
________________________________
From: Thien Minh Huynh <thien.m.hu...@dektech.com.au>
Sent: Friday, February 21, 2020 5:09 PM
To: Thuan Tran <thuan.t...@dektech.com.au>; Vu Minh Nguyen
<vu.m.ngu...@dektech.com.au>
Cc: opensaf-devel@lists.sourceforge.net <opensaf-devel@lists.sourceforge.net>;
Thien Minh Huynh <thien.m.hu...@dektech.com.au>
Subject: [PATCH 1/1] imm: fix non-local user cannot access IMM when
accessControlMode is in ENFORCED [#3043]
---
src/base/osaf_secutil.c | 78 +++++++++++++++++++++++++++++++++++++++
src/base/osaf_secutil.h | 9 +++++
src/imm/immnd/immnd_evt.c | 4 +-
3 files changed, 90 insertions(+), 1 deletion(-)
diff --git a/src/base/osaf_secutil.c b/src/base/osaf_secutil.c
index 0e175c915..a899c1a7f 100644
--- a/src/base/osaf_secutil.c
+++ b/src/base/osaf_secutil.c
@@ -42,6 +42,9 @@
#include <pwd.h>
#include <grp.h>
#include <pthread.h>
+#include <stdio.h>
+#include <ctype.h>
+#include <assert.h>
#include "base/osaf_poll.h"
#include "base/logtrace.h"
@@ -184,6 +187,60 @@ static void *auth_server_main(void *_fd)
return 0;
}
+/**
+ * Get all supplementary groups via process ID from /proc/<pid>/status
+ * @param pid: this param to get all information of process
+ * @return a string containing all group id separated by a space
+ *
+ * NOTE: The caller has to free the allocated memory returned by this function.
+ */
+static char *get_supplementary_group_list(pid_t pid)
+{
+ char process_info_path[255];
+ size_t line_buf_size = 0;
+ char *line_buf = NULL;
+ char *group_list = NULL;
[Vu] Narrow down the scope of these `life_buf` and `group_list` variable by
putting them prior to `getline` below.
+ FILE *fd = NULL;
+
+ sprintf(process_info_path, "/proc/%d/status", pid);
+ while (true) {
+ fd = fopen(process_info_path, "r");
+ if (fd != NULL)
+ break;
+
+ if (errno != EINTR) {
+ LOG_ER("%s fopen Fail %s", __func__, strerror(errno));
+ goto done;
[Vu] Should `return NULL` here, otherwise getting undefined behavior on
fclose(NULL).
+ }
+ }
+
+ while (getline(&line_buf, &line_buf_size, fd) != -1) {
+ const char *groups = "Groups:";
+ size_t groups_len = strlen(groups);
+ if (strncmp(line_buf, groups, groups_len) != 0)
+ continue;
+
+ // No suplementary groups.
+ if (line_buf[groups_len + 1] == '\0')
+ break;
+
+ // Exclude 'Groups:' string and a following tab
+ size_t len = strlen(line_buf) - groups_len - 1;
+ assert(len && "Invalid sumplementary group list");
+ group_list = (char *)malloc(len);
+ strcpy(group_list, line_buf + groups_len + 1);
+
+ // Remove a character 'new line' end of a string
+ group_list[len - 1] = '\0';
+ break;
+ }
+
+done:
[Vu] Remove this label.
+ free(line_buf);
+ fclose(fd);
+ return group_list;
+}
+
/*************** public interface follows*************************** */
int osaf_auth_server_create(const char *pathname,
@@ -305,6 +362,27 @@ bool osaf_user_is_member_of_group(uid_t uid, const char
*groupname)
return false;
}
+bool osaf_pid_is_member_of_group(pid_t pid, const char *groupname)
[Vu] Rename this function to `osaf_pid_has_supplementary_group()
+{
+ struct group *gr = getgrnam(groupname);
+ if (!gr)
[Vu] Use `if (gr == NULL)` to keep consistent with the rest
+ return false;
+
+ gid_t gid = gr->gr_gid;
+ char *group_list = get_supplementary_group_list(pid);
+ if (group_list == NULL)
+ return false;
+
+ const char *delimiter = " ";
+ char *pch = strtok(group_list, delimiter);
+ while (pch != NULL && (gid_t)atoi(pch) != gid) {
+ pch = strtok(NULL, delimiter);
+ }
+
+ free(group_list);
+ return pch != NULL;
+}
+
/* used in libraries, do not log. Only trace */
int osaf_auth_server_connect(const char *path, const void *req_buf,
size_t req_size, void *resp_buf, size_t resp_size,
diff --git a/src/base/osaf_secutil.h b/src/base/osaf_secutil.h
index a2389241c..b0b5485f4 100644
--- a/src/base/osaf_secutil.h
+++ b/src/base/osaf_secutil.h
@@ -88,6 +88,15 @@ int osaf_auth_server_create(const char *_pathname,
*/
bool osaf_user_is_member_of_group(uid_t uid, const char *groupname);
+/**
+ * Checks if user represented by pid is member of group
[Vu] Checks if the `groupname` belongs to supplementary group of the process PID
+ *
+ * @param pid
+ * @param groupname
+ * @return true if member
[Vu] @return true if `groupname` is one of the process ID's supplementary
groups.
+ */
+bool osaf_pid_is_member_of_group(pid_t pid, const char *groupname);
+
/**
* Get list of groups that a user belong to
* There already is a function in LSB for this purpose (getgrouplist) but it is
diff --git a/src/imm/immnd/immnd_evt.c b/src/imm/immnd/immnd_evt.c
index 51a9b8517..ac49fe5e5 100644
--- a/src/imm/immnd/immnd_evt.c
+++ b/src/imm/immnd/immnd_evt.c
@@ -895,7 +895,9 @@ static uint32_t immnd_evt_proc_imm_init(IMMND_CB *cb,
IMMND_EVT *evt,
immModel_authorizedGroup(immnd_cb);
if ((authorized_group != NULL) &&
(osaf_user_is_member_of_group(sinfo->uid,
- authorized_group))) {
+ authorized_group) ||
+ osaf_pid_is_member_of_group(sinfo->pid,
+ authorized_group))) {
TRACE("configured group");
} else {
if (mode == ACCESS_CONTROL_PERMISSIVE) {
--
2.17.1
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
