From f9c71c34f2840263da0bb9b71e53b615b267bafc Mon Sep 17 00:00:00 2001 From: Seonah Moon Date: Thu, 25 Mar 2021 16:54:27 +0900 Subject: [PATCH] Avoid to use stat() function Doing something after checking of the state of a file has a potential race condition bug. It is more effiecient that just using execption handling instead of checking to prevent TOCTOU. Change-Id: I2a2560d9d49ed9d52008557f0d2f89fa04bb130d --- packaging/download-provider.spec | 2 +- provider/download-provider-notify.c | 15 +++-- provider/download-provider-plugin-download-agent.c | 74 ++++++++++------------ 3 files changed, 45 insertions(+), 46 deletions(-) diff --git a/packaging/download-provider.spec b/packaging/download-provider.spec index 95639d0..e7f60aa 100755 --- a/packaging/download-provider.spec +++ b/packaging/download-provider.spec @@ -1,6 +1,6 @@ Name: download-provider Summary: Download the contents in background -Version: 2.2.4 +Version: 2.2.5 Release: 0 Group: Development/Libraries License: Apache-2.0 diff --git a/provider/download-provider-notify.c b/provider/download-provider-notify.c index e2d541f..62d2df7 100644 --- a/provider/download-provider-notify.c +++ b/provider/download-provider-notify.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -47,9 +48,10 @@ int dp_notify_init(pid_t pid) if (notify_fifo == NULL) return -1; int notify_fd = -1; - struct stat fifo_state; - if (stat(notify_fifo, &fifo_state) == 0) // found - unlink(notify_fifo); + + if (unlink(notify_fifo) < 0) + TRACE_ERROR("%s might not be exist. Ignored. errno(%d)", notify_fifo, errno); + if (mkfifo(notify_fifo, 0644/*-rwrr*/) < 0) TRACE_ERROR("failed to make fifo %s", notify_fifo); else @@ -64,9 +66,10 @@ void dp_notify_deinit(pid_t pid) char *notify_fifo = __dp_notify_get_path(pid); if (notify_fifo == NULL) return ; - struct stat fifo_state; - if (stat(notify_fifo, &fifo_state) == 0) // found - unlink(notify_fifo); + + if (unlink(notify_fifo) < 0) + TRACE_ERROR("%s might not be exist. Ignored. errno(%d)", notify_fifo, errno); + free(notify_fifo); } diff --git a/provider/download-provider-plugin-download-agent.c b/provider/download-provider-plugin-download-agent.c index 961baa5..76796f3 100755 --- a/provider/download-provider-plugin-download-agent.c +++ b/provider/download-provider-plugin-download-agent.c @@ -202,52 +202,48 @@ static int __precheck_request(dp_client_slots_fmt* slot, dp_request_fmt *request static int __set_file_permission_to_client(dp_client_slots_fmt *slot, dp_request_fmt *request, char *saved_path) { - if (slot == NULL || request == NULL) { - TRACE_ERROR("check address slot:%p request:%p id:%d agentid:%d", slot, request, (request == NULL ? 0 : request->id), (request == NULL ? 0 : request->agent_id)); - return DP_ERROR_INVALID_PARAMETER; - } else if (saved_path == NULL) { - TRACE_ERROR("check null saved path"); + if (slot == NULL || request == NULL || saved_path == NULL) { + TRACE_ERROR("slot:%p request:%p id:%d agentid:%d saved_path:%s", + slot, request, (request == NULL ? 0 : request->id), + (request == NULL ? 0 : request->agent_id), saved_path ? saved_path : ""); return DP_ERROR_INVALID_PARAMETER; } struct stat lstat_info; - struct stat fstat_info; int fd; - int errorcode = DP_ERROR_NONE; dp_credential cred = slot->credential; - if (lstat(saved_path, &lstat_info) != -1) { - fd = open(saved_path, O_RDONLY); - if (fd != -1) { - if (fstat(fd, &fstat_info) != -1) { - if (lstat_info.st_mode == fstat_info.st_mode && - lstat_info.st_ino == fstat_info.st_ino && - lstat_info.st_dev == fstat_info.st_dev) { - - if (strncmp(saved_path, "/opt/media/", 11) != 0) { - if (fchown(fd, cred.uid, cred.gid) != 0) { - TRACE_ERROR("[ERROR][%d] permission user:%d group:%d", - request->id, cred.uid, cred.gid); - errorcode = DP_ERROR_PERMISSION_DENIED; - } - } - } else { - TRACE_ERROR("fstat & lstat info have not matched"); - errorcode = DP_ERROR_PERMISSION_DENIED; - } - } else { - TRACE_ERROR("fstat call failed"); - errorcode = DP_ERROR_PERMISSION_DENIED; - } - close(fd); - } else { - TRACE_SECURE_ERROR("open failed for file : %s", saved_path); - errorcode = DP_ERROR_IO_ERROR; - } - } else { - TRACE_ERROR("lstat call failed"); - errorcode = DP_ERROR_PERMISSION_DENIED; + + if (lstat(saved_path, &lstat_info) == -1) { + TRACE_ERROR("Cannot access to %s", saved_path); + return DP_ERROR_PERMISSION_DENIED; } - return errorcode; + + if ((lstat_info.st_mode & S_IFMT) == S_IFLNK) { + TRACE_ERROR("%s is a symbolic link.", saved_path); + return DP_ERROR_PERMISSION_DENIED; + } + + fd = open(saved_path, O_RDONLY); + if (fd == -1) { + TRACE_SECURE_ERROR("open failed for file : %s", saved_path); + return DP_ERROR_IO_ERROR; + } + + // Don't need to change the owner and group of a file. + if (strncmp(saved_path, "/opt/media/", 11) != 0) { + close(fd); + return DP_ERROR_NONE; + } + + if (fchown(fd, cred.uid, cred.gid) != 0) { + TRACE_ERROR("[ERROR][%d] permission user:%d group:%d", + request->id, cred.uid, cred.gid); + close(fd); + return DP_ERROR_PERMISSION_DENIED; + } + + close(fd); + return DP_ERROR_NONE; } static void __finished_cb(finished_info_t *info, void *user_req_data, -- 2.7.4