From: Krzysztof Jackiewicz Date: Mon, 13 May 2013 15:17:47 +0000 (+0200) Subject: Cookie executable path logic fixed and refactored. X-Git-Tag: 2.2_release~4 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f0a1f03020eb178434822484eb274b692ffefc56;p=framework%2Fsecurity%2Fsecurity-server.git 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 --- 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 --- a/src/include/security-server-cookie.h +++ b/src/include/security-server-cookie.h @@ -24,7 +24,7 @@ #include "security-server-common.h" -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 --- 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 --- 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 --- 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;ipermission_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;ipermission_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;ipermission_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)