From 1d038a9cf5e154406710800c372631f5c7c3fd81 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 07:20:12 +0100 Subject: abrt-hook-python: add input sanitization and directory size guard Signed-off-by: Denys Vlasenko --- src/Hooks/abrt-hook-python.cpp | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) (limited to 'src/Hooks/abrt-hook-python.cpp') diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index 3f79d28..406cd82 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -24,12 +24,14 @@ /* We can easily get rid of abrtlib (libABRTUtils.so) usage in this file, * but DebugDump will pull it in anyway */ #include "abrtlib.h" +#include "hooklib.h" #include "DebugDump.h" #if HAVE_CONFIG_H # include #endif #define MAX_BT_SIZE (1024*1024) +#define MAX_BT_SIZE_STR "1 MB" static char *pid; static char *executable; @@ -74,9 +76,15 @@ int main(int argc, char** argv) ); } } - if (!pid) + if (!pid || !executable || !uuid) goto usage; -// is it really ok if other params aren't specified? abrtd might get confused... + + unsigned setting_MaxCrashReportsSize = 0; + parse_conf(NULL, &setting_MaxCrashReportsSize, NULL); + if (setting_MaxCrashReportsSize > 0) + { + check_free_space(setting_MaxCrashReportsSize); + } // Read the backtrace from stdin char *bt = (char*)xmalloc(MAX_BT_SIZE); @@ -88,35 +96,34 @@ int main(int argc, char** argv) bt[len] = '\0'; if (len == MAX_BT_SIZE-1) { - error_msg("Backtrace size limit exceeded, trimming to 1 MB"); + error_msg("Backtrace size limit exceeded, trimming to " MAX_BT_SIZE_STR); } + char *cmdline = get_cmdline(xatou(pid)); /* never NULL */ + // Create directory with the debug dump char path[PATH_MAX]; snprintf(path, sizeof(path), DEBUG_DUMPS_DIR"/pyhook-%ld-%s", (long)time(NULL), pid); - CDebugDump dd; dd.Create(path, geteuid()); - dd.SaveText(FILENAME_ANALYZER, "Python"); - if (executable) - dd.SaveText(FILENAME_EXECUTABLE, executable); - pid_t pidt = xatoi(pid); - char *cmdline = get_cmdline(pidt); + dd.SaveText(FILENAME_ANALYZER, "Python"); + dd.SaveText(FILENAME_EXECUTABLE, executable); + dd.SaveText("backtrace", bt); + free(bt); dd.SaveText("cmdline", cmdline); free(cmdline); - - if (uuid) - dd.SaveText("uuid", uuid); - + dd.SaveText("uuid", uuid); char uid[sizeof(int) * 3 + 2]; - sprintf(uid, "%d", (int)getuid()); + sprintf(uid, "%u", (unsigned)getuid()); dd.SaveText("uid", uid); - dd.SaveText("backtrace", bt); - free(bt); dd.Close(); + if (setting_MaxCrashReportsSize > 0) + { + trim_debug_dumps(setting_MaxCrashReportsSize, path); + } return 0; } -- cgit From b1c4304104910c4bc066cd43f9784fe2f3ddf1ad Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 07:21:31 +0100 Subject: *: cast pids and uigs to long, not int Signed-off-by: Denys Vlasenko --- src/Hooks/abrt-hook-python.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/Hooks/abrt-hook-python.cpp') diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index 406cd82..468c7ec 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -115,8 +115,8 @@ int main(int argc, char** argv) dd.SaveText("cmdline", cmdline); free(cmdline); dd.SaveText("uuid", uuid); - char uid[sizeof(int) * 3 + 2]; - sprintf(uid, "%u", (unsigned)getuid()); + char uid[sizeof(long) * 3 + 2]; + sprintf(uid, "%lu", (long)getuid()); dd.SaveText("uid", uid); dd.Close(); -- cgit From 14ef0cfe72faf6696df3ef8f42927e9458ccbeeb Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 07:22:13 +0100 Subject: *: misc fixes Signed-off-by: Denys Vlasenko --- src/Hooks/abrt-hook-python.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/Hooks/abrt-hook-python.cpp') diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index 468c7ec..1a7eace 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -79,6 +79,8 @@ int main(int argc, char** argv) if (!pid || !executable || !uuid) goto usage; +//TODO: sanitize uuid and executable (size, valid chars etc) + unsigned setting_MaxCrashReportsSize = 0; parse_conf(NULL, &setting_MaxCrashReportsSize, NULL); if (setting_MaxCrashReportsSize > 0) -- cgit From d037916adc56d384717ebd6b7a5963543febc170 Mon Sep 17 00:00:00 2001 From: Karel Klic Date: Mon, 11 Jan 2010 12:27:18 +0100 Subject: Catch and display ABRTException thrown by CDebugDump::Create --- src/Hooks/abrt-hook-python.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'src/Hooks/abrt-hook-python.cpp') diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index 1a7eace..d7fca67 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -26,6 +26,7 @@ #include "abrtlib.h" #include "hooklib.h" #include "DebugDump.h" +#include "ABRTException.h" #if HAVE_CONFIG_H # include #endif @@ -108,7 +109,12 @@ int main(int argc, char** argv) snprintf(path, sizeof(path), DEBUG_DUMPS_DIR"/pyhook-%ld-%s", (long)time(NULL), pid); CDebugDump dd; - dd.Create(path, geteuid()); + + try { + dd.Create(path, geteuid()); + } catch (CABRTException &e) { + error_msg_and_die("Error while creating debug dump: %s", e.what()); + } dd.SaveText(FILENAME_ANALYZER, "Python"); dd.SaveText(FILENAME_EXECUTABLE, executable); -- cgit From 71fb2d7e690640b391b76b5432f07b4a81351c8b Mon Sep 17 00:00:00 2001 From: Karel Klic Date: Tue, 12 Jan 2010 14:26:08 +0100 Subject: Fixing /var/cache/abrt/ permissions by allowing users to read, but not to change their crash data. Adds abrt user, changes abrt-hook-python to use suid instead of sgid bit (uid=abrt), sets /var/cache/abrt and every dump subdirectory to be owned by abrt user. Read access for users and their own crashes is provided by group (/var/cache/abrt/ccpp-xxxx-xx has user's group). --- src/Hooks/abrt-hook-python.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/Hooks/abrt-hook-python.cpp') diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index d7fca67..b921fba 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -111,7 +111,7 @@ int main(int argc, char** argv) CDebugDump dd; try { - dd.Create(path, geteuid()); + dd.Create(path, getuid()); } catch (CABRTException &e) { error_msg_and_die("Error while creating debug dump: %s", e.what()); } -- cgit From b85dabbbf338c8e5f4813f3a04e298ce3a8b319f Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Tue, 12 Jan 2010 17:07:01 +0100 Subject: abrt-hook-python: sanitize input more; log to syslog Signed-off-by: Denys Vlasenko --- src/Hooks/abrt-hook-python.cpp | 53 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 10 deletions(-) (limited to 'src/Hooks/abrt-hook-python.cpp') diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index b921fba..c8a25e3 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -20,7 +20,7 @@ */ #include -#include +#include /* We can easily get rid of abrtlib (libABRTUtils.so) usage in this file, * but DebugDump will pull it in anyway */ #include "abrtlib.h" @@ -38,12 +38,33 @@ static char *pid; static char *executable; static char *uuid; -int main(int argc, char** argv) +/* Note: "" will return false */ +static bool isxdigit_str(const char *str) { - // Error if daemon is not running. - if (!daemon_is_ok()) - error_msg_and_die("Daemon is not running."); + do { + if ((*str < '0' || *str > '9') /* not a digit */ + && ((*str | 0x20) < 'a' || (*str | 0x20) > 'f') /* not A-F or a-f */ + ) + { + return false; + } + str++; + } while (*str); + return true; +} + +static bool printable_str(const char *str) +{ + do { + if ((unsigned char)(*str) < ' ' || *str == 0x7f) + return false; + str++; + } while (*str); + return true; +} +int main(int argc, char** argv) +{ // Parse options static const struct option longopts[] = { // name , has_arg , flag, val @@ -79,8 +100,18 @@ int main(int argc, char** argv) } if (!pid || !executable || !uuid) goto usage; + if (strlen(uuid) > 128 || !isxdigit_str(uuid)) + goto usage; + if (strlen(executable) > PATH_MAX || !printable_str(executable)) + goto usage; + // pid string is sanitized later by xatou() -//TODO: sanitize uuid and executable (size, valid chars etc) + openlog("abrt", LOG_PID, LOG_DAEMON); + logmode = LOGMODE_SYSLOG; + + // Error if daemon is not running + if (!daemon_is_ok()) + error_msg_and_die("daemon is not running, python crash dump aborted"); unsigned setting_MaxCrashReportsSize = 0; parse_conf(NULL, &setting_MaxCrashReportsSize, NULL); @@ -94,14 +125,15 @@ int main(int argc, char** argv) ssize_t len = full_read(STDIN_FILENO, bt, MAX_BT_SIZE-1); if (len < 0) { - perror_msg_and_die("Read error"); + perror_msg_and_die("read error"); } bt[len] = '\0'; if (len == MAX_BT_SIZE-1) { - error_msg("Backtrace size limit exceeded, trimming to " MAX_BT_SIZE_STR); + error_msg("backtrace size limit exceeded, trimming to " MAX_BT_SIZE_STR); } + // This also checks that pid is a valid numeric string char *cmdline = get_cmdline(xatou(pid)); /* never NULL */ // Create directory with the debug dump @@ -109,11 +141,10 @@ int main(int argc, char** argv) snprintf(path, sizeof(path), DEBUG_DUMPS_DIR"/pyhook-%ld-%s", (long)time(NULL), pid); CDebugDump dd; - try { dd.Create(path, getuid()); } catch (CABRTException &e) { - error_msg_and_die("Error while creating debug dump: %s", e.what()); + error_msg_and_die("error while creating crash dump %s: %s", path, e.what()); } dd.SaveText(FILENAME_ANALYZER, "Python"); @@ -128,6 +159,8 @@ int main(int argc, char** argv) dd.SaveText("uid", uid); dd.Close(); + log("saved python crash dump of pid %s to %s", pid, path); + if (setting_MaxCrashReportsSize > 0) { trim_debug_dumps(setting_MaxCrashReportsSize, path); -- cgit