diff options
author | Denys Vlasenko <vda.linux@googlemail.com> | 2010-01-11 12:09:57 +0100 |
---|---|---|
committer | Denys Vlasenko <vda.linux@googlemail.com> | 2010-01-11 12:09:57 +0100 |
commit | edf6beb585dc38c365ccbdaae85756b2814e1329 (patch) | |
tree | c356fda7f3397c3b3427f56a5a1584cab7e513c5 | |
parent | 14ef0cfe72faf6696df3ef8f42927e9458ccbeeb (diff) | |
download | abrt-edf6beb585dc38c365ccbdaae85756b2814e1329.tar.gz abrt-edf6beb585dc38c365ccbdaae85756b2814e1329.tar.xz abrt-edf6beb585dc38c365ccbdaae85756b2814e1329.zip |
*: assorted fixes prompted by security analysis; more to come
Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
-rw-r--r-- | inc/abrtlib.h | 2 | ||||
-rw-r--r-- | lib/Plugins/CCpp.cpp | 12 | ||||
-rw-r--r-- | lib/Plugins/Catcut.cpp | 16 | ||||
-rw-r--r-- | lib/Plugins/FileTransfer.cpp | 11 | ||||
-rw-r--r-- | lib/Plugins/Firefox.cpp | 13 | ||||
-rw-r--r-- | lib/Plugins/KerneloopsScanner.cpp | 4 | ||||
-rw-r--r-- | lib/Plugins/Mailx.cpp | 33 | ||||
-rw-r--r-- | lib/Plugins/SQLite3.cpp | 8 | ||||
-rw-r--r-- | lib/Plugins/TicketUploader.cpp | 7 | ||||
-rw-r--r-- | lib/Utils/DebugDump.cpp | 2 | ||||
-rw-r--r-- | lib/Utils/abrt_dbus.h | 3 | ||||
-rw-r--r-- | lib/Utils/spawn.cpp | 1 | ||||
-rw-r--r-- | lib/Utils/xfuncs.cpp | 4 | ||||
-rw-r--r-- | src/Applet/CCApplet.cpp | 11 | ||||
-rw-r--r-- | src/Daemon/Daemon.cpp | 8 | ||||
-rw-r--r-- | src/Daemon/MiddleWare.cpp | 2 | ||||
-rw-r--r-- | src/Daemon/PluginManager.cpp | 2 | ||||
-rw-r--r-- | src/Daemon/RPM.cpp | 4 | ||||
-rw-r--r-- | src/Hooks/CCpp.cpp | 2 | ||||
-rw-r--r-- | src/Hooks/dumpoops.cpp | 1 |
20 files changed, 76 insertions, 70 deletions
diff --git a/inc/abrtlib.h b/inc/abrtlib.h index 8cb32d3c..ed54b528 100644 --- a/inc/abrtlib.h +++ b/inc/abrtlib.h @@ -161,7 +161,7 @@ off_t copy_file(const char *src_name, const char *dst_name, int mode); void xsetreuid(uid_t ruid, uid_t euid); -void xsetregid(gid_t rgid, uid_t egid); +void xsetregid(gid_t rgid, gid_t egid); enum { EXECFLG_INPUT = 1 << 0, EXECFLG_OUTPUT = 1 << 1, diff --git a/lib/Plugins/CCpp.cpp b/lib/Plugins/CCpp.cpp index 46c2f281..4bf1bc1f 100644 --- a/lib/Plugins/CCpp.cpp +++ b/lib/Plugins/CCpp.cpp @@ -353,6 +353,7 @@ static void InstallDebugInfos(const char *pDebugDumpDir, int pipeout[2]; //TODO: can we use ExecVP? xpipe(pipeout); + fflush(NULL); pid_t child = fork(); if (child < 0) { @@ -548,6 +549,8 @@ string CAnalyzerCCpp::GetGlobalUUID(const char *pDebugDumpDir) int pipeout[2]; xpipe(pipeout); /* stdout of abrt-backtrace */ + + fflush(NULL); pid_t child = fork(); if (child == -1) perror_msg_and_die("fork"); @@ -609,8 +612,9 @@ string CAnalyzerCCpp::GetGlobalUUID(const char *pDebugDumpDir) return CreateHash(hash_base.c_str()); } -static bool DebuginfoCheckPolkit(int uid) +static bool DebuginfoCheckPolkit(uid_t uid) { + fflush(NULL); int child_pid = fork(); if (child_pid < 0) { @@ -627,8 +631,10 @@ static bool DebuginfoCheckPolkit(int uid) //parent int status; - if (waitpid(child_pid, &status, 0) > 0 && WIFEXITED(status) && WEXITSTATUS(status) == 0) - { + if (waitpid(child_pid, &status, 0) > 0 + && WIFEXITED(status) + && WEXITSTATUS(status) == 0 + ) { return true; //authorization OK } log("UID %d is not authorized to install debuginfos", uid); diff --git a/lib/Plugins/Catcut.cpp b/lib/Plugins/Catcut.cpp index a56015d2..52efc573 100644 --- a/lib/Plugins/Catcut.cpp +++ b/lib/Plugins/Catcut.cpp @@ -13,7 +13,7 @@ using namespace std; static int -put_stream(const char *pURL, FILE* f, size_t content_length) +put_stream(const char *pURL, FILE* f, off_t content_length) { CURL* curl = xcurl_easy_init(); /* enable uploading */ @@ -22,8 +22,8 @@ put_stream(const char *pURL, FILE* f, size_t content_length) curl_easy_setopt(curl, CURLOPT_URL, pURL); /* file handle: passed to the default callback, it will fread() it */ curl_easy_setopt(curl, CURLOPT_READDATA, f); - /* get file size */ - curl_easy_setopt(curl, CURLOPT_INFILESIZE, content_length); + /* file size */ + curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, (curl_off_t)content_length); /* everything is done here; result 0 means success */ int result = curl_easy_perform(curl); /* goodbye */ @@ -43,9 +43,9 @@ send_string(const char *pURL, return; } + size_t content_length = strlen(pContent); while (1) { - int content_length = strlen(pContent); FILE* f = fmemopen((void*)pContent, content_length, "r"); if (!f) { @@ -53,7 +53,7 @@ send_string(const char *pURL, } int result = put_stream(pURL, f, content_length); fclose(f); - if (!result) + if (result == 0) return; update_client(_("Sending failed, try it again: %s"), curl_easy_strerror((CURLcode)result)); if (--retryCount <= 0) @@ -88,10 +88,9 @@ send_file(const char *pURL, } struct stat buf; fstat(fileno(f), &buf); /* can't fail */ - int content_length = buf.st_size; - int result = put_stream(pURL, f, content_length); + int result = put_stream(pURL, f, buf.st_size); fclose(f); - if (!result) + if (result == 0) return; update_client(_("Sending failed, try it again: %s"), curl_easy_strerror((CURLcode)result)); if (--retryCount <= 0) @@ -122,6 +121,7 @@ resolve_relative_url(const char *url, const char *base) } const char *end_of_protocol = strchr(base, ':'); +//TODO: why is this safe?!! string protocol(base, end_of_protocol - base); end_of_protocol += 3; /* skip "://" */ diff --git a/lib/Plugins/FileTransfer.cpp b/lib/Plugins/FileTransfer.cpp index 9d7a59af..b08ecd51 100644 --- a/lib/Plugins/FileTransfer.cpp +++ b/lib/Plugins/FileTransfer.cpp @@ -80,11 +80,7 @@ void CFileTransfer::SendFile(const char *pURL, const char *pFilename) { throw CABRTException(EXCEP_PLUGIN, "Can't open archive file '%s'", pFilename); } - if (fstat(fileno(f), &buf) == -1) - { - fclose(f); - throw CABRTException(EXCEP_PLUGIN, "Can't stat archive file '%s'", pFilename); - } + fstat(fileno(f), &buf); /* never fails */ curl = xcurl_easy_init(); /* enable uploading */ curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L); @@ -92,7 +88,7 @@ void CFileTransfer::SendFile(const char *pURL, const char *pFilename) curl_easy_setopt(curl, CURLOPT_URL, wholeURL.c_str()); /* FILE handle: passed to the default callback, it will fread() it */ curl_easy_setopt(curl, CURLOPT_READDATA, f); - curl_easy_setopt(curl, CURLOPT_INFILESIZE, buf.st_size); + curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, (curl_off_t)buf.st_size); /* everything is done here; result 0 means success */ int result = curl_easy_perform(curl); curl_easy_cleanup(curl); @@ -181,6 +177,7 @@ static void create_targz(const char * archive_name, const char * directory) f = fopen(name_without_gz, "r"); if (f == NULL) { +//TODO: we leak uncompressed tar file on disk?? free(name_without_gz); return; } @@ -226,6 +223,7 @@ static void create_tarbz2(const char * archive_name, const char * directory) f = fopen(archive_name, "w"); if (f == NULL) { +//TODO: we leak uncompressed tar file on disk?? close(tarFD); free(name_without_bz2); return; @@ -312,6 +310,7 @@ void CFileTransfer::Run(const char *pActionDir, const char *pArgs) else if (strcmp(pArgs, "one") == 0) { /* just send one archive */ +//TODO: where are we creating it??!! In cwd, which may well be / ??!!! string archivename = ssprintf("%s-%s%s", hostname, DirBase(pActionDir).c_str(), m_sArchiveType.c_str()); try { diff --git a/lib/Plugins/Firefox.cpp b/lib/Plugins/Firefox.cpp index cd02a152..d9807556 100644 --- a/lib/Plugins/Firefox.cpp +++ b/lib/Plugins/Firefox.cpp @@ -99,6 +99,8 @@ static pid_t ExecVP(char** pArgs, uid_t uid, std::string& pOutput) pid_t child; xpipe(pipeout); + + fflush(NULL); child = fork(); if (child == -1) { @@ -549,6 +551,7 @@ static void InstallDebugInfos(const char *pDebugDumpDir, std::string& build_ids) xpipe(pipein); xpipe(pipeout); + fflush(NULL); pid_t child = fork(); if (child < 0) { @@ -673,6 +676,7 @@ static void InstallDebugInfos(const char *pDebugDumpDir, std::string& build_ids) int pipeout[2]; //TODO: can we use ExecVP? xpipe(pipeout); + fflush(NULL); pid_t child = fork(); if (child < 0) { @@ -839,8 +843,9 @@ std::string CAnalyzerFirefox::GetGlobalUUID(const char *pDebugDumpDir) return CreateHash(package + executable + independentBacktrace); } -static bool DebuginfoCheckPolkit(int uid) +static bool DebuginfoCheckPolkit(uid_t uid) { + fflush(NULL); int child_pid = fork(); if (child_pid < 0) { @@ -857,8 +862,10 @@ static bool DebuginfoCheckPolkit(int uid) //parent int status; - if (waitpid(child_pid, &status, 0) > 0 && WEXITSTATUS(status) == 0) - { + if (waitpid(child_pid, &status, 0) > 0 + && WIFEXITED(status) + && WEXITSTATUS(status) == 0 + ) { return true; //authorization OK } log("UID %d is not authorized to install debuginfos", uid); diff --git a/lib/Plugins/KerneloopsScanner.cpp b/lib/Plugins/KerneloopsScanner.cpp index 128e0837..f05f0d82 100644 --- a/lib/Plugins/KerneloopsScanner.cpp +++ b/lib/Plugins/KerneloopsScanner.cpp @@ -147,8 +147,10 @@ int CKerneloopsScanner::ScanSysLogFile(const char *filename) if (fd < 0) return 0; statb.st_size = 0; /* paranoia */ - if (fstat(fd, &statb) != 0 || statb.st_size < 1) + if (fstat(fd, &statb) != 0 || statb.st_size < 1) { + close(fd); return 0; + } /* * in theory there's a race here, since someone could spew diff --git a/lib/Plugins/Mailx.cpp b/lib/Plugins/Mailx.cpp index 70eddb8e..df33e84c 100644 --- a/lib/Plugins/Mailx.cpp +++ b/lib/Plugins/Mailx.cpp @@ -39,33 +39,16 @@ CMailx::CMailx() : static void exec_and_feed_input(uid_t uid, const char* pText, char **pArgs) { int pipein[2]; - pid_t child; - xpipe(pipein); - child = fork(); - if (child == -1) - { - close(pipein[0]); - close(pipein[1]); - throw CABRTException(EXCEP_PLUGIN, "Can't fork"); - } - if (child == 0) - { - close(pipein[1]); - xmove_fd(pipein[0], STDIN_FILENO); - - struct passwd* pw = getpwuid(uid); - gid_t gid = pw ? pw->pw_gid : uid; - setgroups(1, &gid); - xsetregid(gid, gid); - xsetreuid(uid, uid); - - execvp(pArgs[0], pArgs); - exit(1); /* exec failed */ - } + pid_t child = fork_execv_on_steroids( + EXECFLG_INPUT | EXECFLG_QUIET | EXECFLG_SETGUID, + pArgs, + pipein, + /*unsetenv_vec:*/ NULL, + /*dir:*/ NULL, + uid); - close(pipein[0]); - safe_write(pipein[1], pText, strlen(pText)); + full_write(pipein[1], pText, strlen(pText)); close(pipein[1]); waitpid(child, NULL, 0); /* wait for command completion */ diff --git a/lib/Plugins/SQLite3.cpp b/lib/Plugins/SQLite3.cpp index 09634561..ffcf05fe 100644 --- a/lib/Plugins/SQLite3.cpp +++ b/lib/Plugins/SQLite3.cpp @@ -375,15 +375,15 @@ void CSQLite3::DeleteRow(const char *pUUID, const char *pUID) execute_sql(m_pDB, "DELETE FROM "ABRT_TABLE" " "WHERE "COL_UUID" = '%s';", - pUUID, pUID + pUUID ); } else if (exists_uuid_uid(m_pDB, pUUID, pUID)) { execute_sql(m_pDB, "DELETE FROM "ABRT_TABLE" " - "WHERE "COL_UUID" = '%s' " - "AND ("COL_UID" = '%s' OR "COL_UID" = '-1');", - pUUID, pUID + "WHERE "COL_UUID" = '%s' " + "AND ("COL_UID" = '%s' OR "COL_UID" = '-1');", + pUUID, pUID ); } else diff --git a/lib/Plugins/TicketUploader.cpp b/lib/Plugins/TicketUploader.cpp index de197912..3fb7bcd5 100644 --- a/lib/Plugins/TicketUploader.cpp +++ b/lib/Plugins/TicketUploader.cpp @@ -120,17 +120,14 @@ void CTicketUploader::SendFile(const char *pURL, const char *pFilename) throw CABRTException(EXCEP_PLUGIN, "Can't open archive file '%s'", pFilename); } struct stat buf; - if (fstat(fileno(f), &buf) == -1) /* TODO: never fails */ - { - throw CABRTException(EXCEP_PLUGIN, "Can't stat archive file '%s'", pFilename); - } + fstat(fileno(f), &buf); /* never fails */ CURL* curl = xcurl_easy_init(); /* enable uploading */ curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L); /* specify target */ curl_easy_setopt(curl, CURLOPT_URL, wholeURL.c_str()); curl_easy_setopt(curl, CURLOPT_READDATA, f); - curl_easy_setopt(curl, CURLOPT_INFILESIZE, buf.st_size); + curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, (curl_off_t)buf.st_size); /* everything is done here; result 0 means success */ result = curl_easy_perform(curl); /* goodbye */ diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index dd992aab..cd45493e 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -307,7 +307,7 @@ static void DeleteFileDir(const char *pDir) } } closedir(dir); - if (remove(pDir) == -1) + if (rmdir(pDir) == -1) { throw CABRTException(EXCEP_DD_DELETE, "Can't remove dir %s", pDir); } diff --git a/lib/Utils/abrt_dbus.h b/lib/Utils/abrt_dbus.h index bc9889a7..25e099ea 100644 --- a/lib/Utils/abrt_dbus.h +++ b/lib/Utils/abrt_dbus.h @@ -154,6 +154,9 @@ enum { ABRT_DBUS_ERROR = -1, ABRT_DBUS_LAST_FIELD = 0, ABRT_DBUS_MORE_FIELDS = 1, + /* note that dbus_message_iter_next() returns FALSE on last field + * and TRUE if there are more fields. + * It maps exactly on the above constants. */ }; /* Checks type, loads data, advances to the next arg. * Returns TRUE if next arg exists. diff --git a/lib/Utils/spawn.cpp b/lib/Utils/spawn.cpp index 4b0eecbf..3aeb682c 100644 --- a/lib/Utils/spawn.cpp +++ b/lib/Utils/spawn.cpp @@ -27,6 +27,7 @@ pid_t fork_execv_on_steroids(int flags, if (flags & EXECFLG_OUTPUT) xpipe(pipe_fm_child); + fflush(NULL); child = fork(); if (child == -1) { perror_msg_and_die("fork"); diff --git a/lib/Utils/xfuncs.cpp b/lib/Utils/xfuncs.cpp index 7ffbb2d1..f360d33b 100644 --- a/lib/Utils/xfuncs.cpp +++ b/lib/Utils/xfuncs.cpp @@ -158,8 +158,8 @@ char* xvasprintf(const char *format, va_list p) #else // Bloat for systems that haven't got the GNU extension. va_list p2; - r = vsnprintf(NULL, 0, format, p); va_copy(p2, p); + r = vsnprintf(NULL, 0, format, p); string_ptr = xmalloc(r+1); r = vsnprintf(string_ptr, r+1, format, p2); va_end(p2); @@ -374,7 +374,7 @@ void xsetreuid(uid_t ruid, uid_t euid) perror_msg_and_die("can't set %cid %lu", 'u', (long)ruid); } -void xsetregid(gid_t rgid, uid_t egid) +void xsetregid(gid_t rgid, gid_t egid) { if (setregid(rgid, egid) != 0) perror_msg_and_die("can't set %cid %lu", 'g', (long)rgid); diff --git a/src/Applet/CCApplet.cpp b/src/Applet/CCApplet.cpp index 770915a0..302fe0bf 100644 --- a/src/Applet/CCApplet.cpp +++ b/src/Applet/CCApplet.cpp @@ -192,20 +192,21 @@ void CApplet::SetIconTooltip(const char *format, ...) void CApplet::CrashNotify(const char *format, ...) { va_list args; - char *buf; - int n; - GError *err = NULL; va_start(args, format); - buf = NULL; - n = vasprintf(&buf, format, args); + char *buf = xvasprintf(format, args); va_end(args); notify_notification_update(m_pNotification, _("Warning"), buf, NULL); + + GError *err = NULL; if (gtk_status_icon_is_embedded(m_pStatusIcon)) notify_notification_show(m_pNotification, &err); if (err != NULL) + { error_msg("%s", err->message); + g_error_free(err); + } } void CApplet::OnAppletActivate_CB(GtkStatusIcon *status_icon, gpointer user_data) diff --git a/src/Daemon/Daemon.cpp b/src/Daemon/Daemon.cpp index 9e1aa0df..0f9c6228 100644 --- a/src/Daemon/Daemon.cpp +++ b/src/Daemon/Daemon.cpp @@ -203,10 +203,12 @@ static int SetUpCron() int nM = -1; int nS = -1; +//TODO: rewrite using good old sscanf? + if (pos != std::string::npos) { - std::string sH = ""; - std::string sM = ""; + std::string sH; + std::string sM; sH = it_c->first.substr(0, pos); nH = xatou(sH.c_str()); @@ -221,7 +223,7 @@ static int SetUpCron() } else { - std::string sS = ""; + std::string sS; sS = it_c->first; nS = xatou(sS.c_str()); diff --git a/src/Daemon/MiddleWare.cpp b/src/Daemon/MiddleWare.cpp index 3656060a..a348a92d 100644 --- a/src/Daemon/MiddleWare.cpp +++ b/src/Daemon/MiddleWare.cpp @@ -548,6 +548,8 @@ report_status_t Report(const map_crash_report_t& pCrashReport, static bool IsDebugDumpSaved(const char *pUID, const char *pDebugDumpDir) { + /* TODO: use database query instead of dumping all rows and searching in them */ + CDatabase* database = g_pPluginManager->GetDatabase(g_settings_sDatabase.c_str()); database->Connect(); vector_database_rows_t rows = database->GetUIDData(pUID); diff --git a/src/Daemon/PluginManager.cpp b/src/Daemon/PluginManager.cpp index 697b9646..a6550e74 100644 --- a/src/Daemon/PluginManager.cpp +++ b/src/Daemon/PluginManager.cpp @@ -138,6 +138,8 @@ void CPluginManager::LoadPlugins() if (!ext || strcmp(ext + 1, PLUGINS_LIB_EXTENSION) != 0) continue; *ext = '\0'; + if (strncmp(dent->d_name, PLUGINS_LIB_PREFIX, sizeof(PLUGINS_LIB_PREFIX)-1) != 0) + continue; LoadPlugin(dent->d_name + sizeof(PLUGINS_LIB_PREFIX)-1, /*enabled_only:*/ true); } closedir(dir); diff --git a/src/Daemon/RPM.cpp b/src/Daemon/RPM.cpp index 6f05c0b9..6cc0ba68 100644 --- a/src/Daemon/RPM.cpp +++ b/src/Daemon/RPM.cpp @@ -4,8 +4,8 @@ CRPM::CRPM() { - char *argv[] = { (char*)"" }; - m_poptContext = rpmcliInit(0, argv, NULL); + static const char *const argv[] = { "", NULL }; + m_poptContext = rpmcliInit(1, (char**)argv, NULL); } CRPM::~CRPM() diff --git a/src/Hooks/CCpp.cpp b/src/Hooks/CCpp.cpp index b5bfff77..ea08baeb 100644 --- a/src/Hooks/CCpp.cpp +++ b/src/Hooks/CCpp.cpp @@ -139,7 +139,7 @@ int main(int argc, char** argv) * if they happen too often. Else, write new marker value. */ snprintf(path, sizeof(path), "%s/last-ccpp", dddir); - fd = open(path, O_RDWR | O_CREAT, 0666); + fd = open(path, O_RDWR | O_CREAT, 0600); if (fd >= 0) { int sz; diff --git a/src/Hooks/dumpoops.cpp b/src/Hooks/dumpoops.cpp index 4b6778d0..01e65c42 100644 --- a/src/Hooks/dumpoops.cpp +++ b/src/Hooks/dumpoops.cpp @@ -83,6 +83,7 @@ int main(int argc, char **argv) void *handle; errno = 0; +//TODO: use it directly, not via dlopen? handle = dlopen(PLUGINS_LIB_DIR"/libKerneloopsScanner.so", RTLD_NOW); if (!handle) perror_msg_and_die("can't load %s", PLUGINS_LIB_DIR"/libKerneloopsScanner.so"); |