Cookie executable path logic fixed and refactored.

[Issue#] SSDWSSP-237 / P130508-4841
[Bug] Security-server has closed unexpectedly
[Problem] N/A
[Cause] Executable paths were improperly compared and triggered pid reusage code
branch.
[Solution] Executable paths logic fixed and refactored.

[Verification] Run all security-server tests

Change-Id: I68219631378be17c980b52fa8995d9bc37d69ed7

Conflicts:

	src/server/security-server-cookie.c

diff --git a/src/include/security-server-common.h b/src/include/security-server-common.h
index afc7c24..86af882 100644 (file)
--- a/src/include/security-server-common.h
+++ b/src/include/security-server-common.h
@@ -89,7 +89,6 @@
 typedef struct _cookie_list
 {
        unsigned char   cookie[SECURITY_SERVER_COOKIE_LEN];     /* 20 bytes random Cookie */
-       int             path_len;                               /* Client process cmd line length */
        int             permission_len;                         /* Client process permissions (aka group IDs) */
        pid_t           pid;                                    /* Client process's PID */
        char            *path;                                  /* Client process's executable path */

diff --git a/src/include/security-server-cookie.h b/src/include/security-server-cookie.h
index c24fcda..576c08a 100644 (file)
--- a/src/include/security-server-cookie.h
+++ b/src/include/security-server-cookie.h
@@ -24,7 +24,7 @@
 \r
 #include "security-server-common.h"\r
 \r
-int free_cookie_item(cookie_list *cookie);
+void free_cookie_item(cookie_list *cookie);
 cookie_list *delete_cookie_item(cookie_list *cookie);
 cookie_list *search_existing_cookie(int pid, const cookie_list *c_list);
 cookie_list *search_cookie(const cookie_list *c_list, const unsigned char *cookie, int * privileges, int privilegesSize);

diff --git a/src/server/security-server-cookie.c b/src/server/security-server-cookie.c
index fca106d..f772450 100644 (file)
--- a/src/server/security-server-cookie.c
+++ b/src/server/security-server-cookie.c
@@ -34,21 +34,16 @@
 
 /* Delete useless cookie item *
  * then connect prev and next */
-int free_cookie_item(cookie_list *cookie)
+void free_cookie_item(cookie_list *cookie)
 {
-       if(cookie->path != NULL)
-               free(cookie->path);
-       if(cookie->permissions != NULL)
-               free(cookie->permissions);
-        if(cookie->smack_label != NULL)
-                free(cookie->smack_label);
+       free(cookie->path);
+       free(cookie->permissions);
+       free(cookie->smack_label);
        if(cookie->prev != NULL)
                cookie->prev->next = cookie->next;
        if(cookie->next != NULL)
                cookie->next->prev = cookie->prev;
        free(cookie);
-       cookie = NULL;
-       return 0;
 }
 
 /* Cut the link of the current cookie item and connect previous link and next line *
@@ -150,30 +145,9 @@ cookie_list *search_existing_cookie(int pid, const cookie_list *c_list)
                        /* Check the path is different.  */
                        if(strcmp(exe, current->path) != 0)
                        {
-                               SEC_SVR_DBG("pid [%d] has been reused by %s. deleting the old cookie.", pid, exe);
-                               debug_cmdline = malloc(current->path_len + 1);
-                               if(debug_cmdline == NULL)
-                               {
-                                       SEC_SVR_DBG("%s", "out of memory error");
-                                       free(exe);
-                                       return NULL;
-                               }
-                               strncpy(debug_cmdline, current->path, current->path_len);
-                               debug_cmdline[current->path_len] = 0;
-                               SEC_SVR_DBG("[%s] --> [%s]", exe, debug_cmdline);
-                               if(debug_cmdline != NULL)
-                               {
-                                       free(debug_cmdline);
-                                       debug_cmdline = NULL;
-                               }
-                               /* Okay. delete current cookie */
+                               /* Delete cookie for reused pid. This is an extremely rare situation. */
+                               SEC_SVR_DBG("Pid [%d] for exec [%s] has been reused by [%s]. Deleting the old cookie.", pid, current->path, exe);
                                current = delete_cookie_item(current);
-                               if(exe != NULL)
-                               {
-                                       free(exe);
-                                       exe = NULL;
-                               }
-                               continue;
                        }
                        else
                        {
@@ -525,10 +499,8 @@ out_of_while:
         }
     }
 
-       added->path_len = strlen(exe);
-       added->path = calloc(1, strlen(exe));
-       memcpy(added->path, exe, strlen(exe));
-
+       added->path = exe;
+       exe = NULL;
     added->permission_len = perm_num;
     added->pid = pid;
     added->permissions = permissions;
@@ -622,7 +594,6 @@ cookie_list *create_default_cookie(void)
                return NULL;
        }
 
-       first->path_len = 0;
        first->permission_len = 0;
        first->pid = 0;
        first->path = NULL;

diff --git a/src/server/security-server-main.c b/src/server/security-server-main.c
index 42518b0..a18ceb4 100644 (file)
--- a/src/server/security-server-main.c
+++ b/src/server/security-server-main.c
@@ -89,7 +89,7 @@ void print_cookie(cookie_list *list)
        int i;
        printf("%s", "cookie:\n");
        printhex(list->cookie, SECURITY_SERVER_COOKIE_LEN);
-       printf("path_len: %d\n", list->path_len);
+       printf("path_len: %d\n", list->path ? strlen(list->path) : 0);
        printf("permission_len: %d\n", list->permission_len);
        printf("PID: %d\n", list->pid);
        printf("path: %s\n", list->path);

diff --git a/src/util/security-server-util-common.c b/src/util/security-server-util-common.c
index b53d2b1..df85270 100644 (file)
--- a/src/util/security-server-util-common.c
+++ b/src/util/security-server-util-common.c
@@ -37,6 +37,34 @@
 #include "security-server-util.h"
 #include "security-server.h"
 
+/*
+ * @buffer   output buffer
+ * @position target position in output buffer
+ * @source   source data
+ * @len      source data length
+ */
+static void append_to_buffer(unsigned char *buffer, int* position, const void* source, size_t len) {
+       if (len <= 0) {
+               SEC_SVR_DBG("Appending nothing.");
+               return;
+       }
+       memcpy(buffer + *position, source, len);
+       *position += len;
+}
+
+static void append_cookie(unsigned char *buffer, int* position, const cookie_list* cookie) {
+       int i;
+       int path_len = cookie->path ? strlen(cookie->path) : 0;
+
+       append_to_buffer(buffer, position, &path_len, sizeof(int));
+       append_to_buffer(buffer, position, &cookie->permission_len, sizeof(int));
+       append_to_buffer(buffer, position, &cookie->cookie, SECURITY_SERVER_COOKIE_LEN);
+       append_to_buffer(buffer, position, &cookie->pid, sizeof(pid_t));
+       append_to_buffer(buffer, position, &cookie->path, path_len);
+
+       for(i=0;i<cookie->permission_len;++i)
+               append_to_buffer(buffer, position, &cookie->permissions[i], sizeof(int));
+}
 
 /* Get all cookie info response *
  * packet format
@@ -89,10 +117,10 @@
  * |                                                               |
  * |                                                               |
  */
- unsigned char * get_all_cookie_info(cookie_list *list, int *size)
+unsigned char * get_all_cookie_info(cookie_list *list, int *size)
 {
        cookie_list *current = list;
-       int ptr, total_num, total_size, tempnum, i;
+       int ptr, total_num, total_size, path_len;
        unsigned char *buf = NULL, *tempptr = NULL;
        response_header hdr;
 
@@ -109,7 +137,8 @@
                        break;
 
                total_num++;
-               total_size += sizeof(int) + sizeof(int) + SECURITY_SERVER_COOKIE_LEN + sizeof(int) + current->path_len + (current->permission_len * sizeof(int));
+               path_len = current->path ? strlen(current->path) : 0;
+               total_size += sizeof(int) + sizeof(int) + SECURITY_SERVER_COOKIE_LEN + sizeof(pid_t) + path_len + (current->permission_len * sizeof(int));
                tempptr = realloc(buf, total_size);
                if(tempptr == NULL)
                {
@@ -118,26 +147,7 @@
                }
                buf = tempptr;
 
-               tempnum = current->path_len;
-               memcpy(buf+ptr, &tempnum, sizeof(int));
-               ptr += sizeof(int);
-               tempnum = current->permission_len;
-               memcpy(buf+ptr, &tempnum, sizeof(int));
-               ptr += sizeof(int);
-               memcpy(buf+ptr, current->cookie, SECURITY_SERVER_COOKIE_LEN);
-               ptr += SECURITY_SERVER_COOKIE_LEN;
-               tempnum = current->pid;
-               memcpy(buf+ptr, &tempnum, sizeof(int));
-               ptr += sizeof(int);
-               memcpy(buf+ptr, current->path, current->path_len);
-               ptr += current->path_len;
-
-               for(i=0;i<current->permission_len;i++)
-               {
-                       tempnum = current->permissions[i];
-                       memcpy(buf+ptr, &tempnum, sizeof(int));
-                       ptr += sizeof(int);
-               }
+               append_cookie(buf, &ptr, current);
                current = current->next;
        }
 
@@ -152,9 +162,11 @@
        hdr.basic_hdr.msg_id = SECURITY_SERVER_MSG_TYPE_GET_ALL_COOKIES_RESPONSE;
        hdr.basic_hdr.msg_len =(unsigned short)( total_size - sizeof(hdr));
        hdr.return_code = SECURITY_SERVER_RETURN_CODE_SUCCESS;
-       memcpy(buf, &hdr, sizeof(hdr));
-       tempnum = total_num;
-       memcpy(buf + sizeof(hdr), &tempnum, sizeof(int));
+
+       // reset buffer position to the beginning of buffer and insert header
+       ptr = 0;
+       append_to_buffer(buf, &ptr, &hdr, sizeof(hdr));
+       append_to_buffer(buf, &ptr, &total_num, sizeof(total_num));
        *size = total_size;
        return buf;
 }
@@ -214,9 +226,11 @@ int send_one_cookie_info(const cookie_list *list, int sockfd)
 {
        unsigned char *buf = NULL;
        response_header hdr;
-       int total_size, ptr = 0, tempnum, ret, i;
+       int total_size, ptr = 0, ret, path_len;
 
-       total_size = sizeof(hdr) + sizeof(int) + sizeof(int) + SECURITY_SERVER_COOKIE_LEN + sizeof(int) + list->path_len + (list->permission_len * sizeof(int));
+       path_len = list->path ? strlen(list->path) : 0;
+
+       total_size = sizeof(hdr) + sizeof(int) + sizeof(int) + SECURITY_SERVER_COOKIE_LEN + sizeof(pid_t) + path_len + (list->permission_len * sizeof(int));
        buf = malloc(total_size);
        if(buf == NULL)
        {
@@ -226,31 +240,13 @@ int send_one_cookie_info(const cookie_list *list, int sockfd)
 
        hdr.basic_hdr.version = SECURITY_SERVER_MSG_VERSION;
        hdr.basic_hdr.msg_id = SECURITY_SERVER_MSG_TYPE_GET_COOKIEINFO_RESPONSE;
-       hdr.basic_hdr.msg_len =sizeof(int) + sizeof(int) + SECURITY_SERVER_COOKIE_LEN + sizeof(int) + list->path_len + (list->permission_len * sizeof(int));
+       hdr.basic_hdr.msg_len =sizeof(int) + sizeof(int) + SECURITY_SERVER_COOKIE_LEN + sizeof(pid_t) + path_len + (list->permission_len * sizeof(int));
        hdr.return_code = SECURITY_SERVER_RETURN_CODE_SUCCESS;
-       memcpy(buf, &hdr, sizeof(hdr));
-       ptr += sizeof(hdr);
-
-       tempnum = list->path_len;
-       memcpy(buf+ptr, &tempnum, sizeof(int));
-       ptr += sizeof(int);
-       tempnum = list->permission_len;
-       memcpy(buf+ptr, &tempnum, sizeof(int));
-       ptr += sizeof(int);
-       memcpy(buf+ptr, list->cookie, SECURITY_SERVER_COOKIE_LEN);
-       ptr += SECURITY_SERVER_COOKIE_LEN;
-       tempnum = list->pid;
-       memcpy(buf+ptr, &tempnum, sizeof(int));
-       ptr += sizeof(int);
-       memcpy(buf+ptr, list->path, list->path_len);
-       ptr += list->path_len;
-
-       for(i=0;i<list->permission_len;i++)
-       {
-               tempnum = list->permissions[i];
-               memcpy(buf+ptr, &tempnum, sizeof(int));
-               ptr += sizeof(int);
-       }
+
+       // header
+       append_to_buffer(buf, &ptr, &hdr, sizeof(hdr));
+       // cookie
+       append_cookie(buf, &ptr, list);
 
        ret = check_socket_poll(sockfd, POLLOUT, SECURITY_SERVER_SOCKET_TIMEOUT_MILISECOND);
        if(ret == SECURITY_SERVER_ERROR_POLL)