diff --git a/Makefile-constants.mk b/Makefile-constants.mk index c37f6b69a1..d5f4e98dfd 100644 --- a/Makefile-constants.mk +++ b/Makefile-constants.mk @@ -13,7 +13,7 @@ endif USE_VALGRIND ?= false ADDRESS_SANITIZER ?= false -CMAKE_BUILD_TYPE ?= Release +CMAKE_BUILD_TYPE ?= Debug COLLECTOR_APPEND_CID ?= false PLATFORM ?= linux/amd64 TRACE_SINSP_EVENTS ?= false diff --git a/collector/Makefile b/collector/Makefile index 26770487d2..9f78f5c72d 100644 --- a/collector/Makefile +++ b/collector/Makefile @@ -37,6 +37,7 @@ container/bin/collector: cmake-build/collector mkdir -p container/bin cp "$(COLLECTOR_BIN_DIR)/collector" container/bin/collector cp "$(COLLECTOR_BIN_DIR)/self-checks" container/bin/self-checks + cp "$(COLLECTOR_BIN_DIR)/test/ProcessSignalFormatterTest" container/bin/ProcessSignalFormatterTest .PHONY: collector collector: container/bin/collector txt-files diff --git a/collector/container/Dockerfile b/collector/container/Dockerfile index 79a9e6fc50..ed3ed091e8 100644 --- a/collector/container/Dockerfile +++ b/collector/container/Dockerfile @@ -29,6 +29,7 @@ COPY container/THIRD_PARTY_NOTICES/ /THIRD_PARTY_NOTICES/ COPY kernel-modules /kernel-modules COPY container/bin/collector /usr/local/bin/ COPY container/bin/self-checks /usr/local/bin/self-checks +COPY container/bin/ProcessSignalFormatterTest /usr/local/bin/ProcessSignalFormatterTest COPY container/status-check.sh /usr/local/bin/status-check.sh RUN echo "${MODULE_VERSION}" > /kernel-modules/MODULE_VERSION.txt && \ diff --git a/collector/container/konflux.Dockerfile b/collector/container/konflux.Dockerfile index 0daa333930..4edda56414 100644 --- a/collector/container/konflux.Dockerfile +++ b/collector/container/konflux.Dockerfile @@ -52,7 +52,7 @@ ARG BUILD_DIR ARG SRC_ROOT_DIR=${BUILD_DIR} ARG CMAKE_BUILD_DIR # TODO(ROX-20240): CMAKE_BUILD_TYPE should probably not be Release for PR, normal branch builds -ARG CMAKE_BUILD_TYPE=Release +ARG CMAKE_BUILD_TYPE=Debug # Appends an argument to the driver download URL that is used for filtering alerts on missing kernels. # TODO(ROX-20240): This needs to be true on PRs only. ARG COLLECTOR_APPEND_CID=false diff --git a/collector/container/rhel/install.sh b/collector/container/rhel/install.sh index ee32ddae17..9dc88f153b 100755 --- a/collector/container/rhel/install.sh +++ b/collector/container/rhel/install.sh @@ -3,7 +3,7 @@ set -eo pipefail # UBI 9 requires confirmation with -y flag. microdnf upgrade -y --nobest -microdnf install -y kmod findutils elfutils-libelf +microdnf install -y kmod findutils elfutils-libelf gdb procps microdnf clean all rpm --query --all 'curl' '*rpm*' '*dnf*' '*libsolv*' '*hawkey*' 'yum*' 'findutils' | xargs -t rpm -e --nodeps diff --git a/collector/lib/CollectorConfig.cpp b/collector/lib/CollectorConfig.cpp index e033d8aaf5..1ea0d6cfc3 100644 --- a/collector/lib/CollectorConfig.cpp +++ b/collector/lib/CollectorConfig.cpp @@ -58,6 +58,8 @@ BoolEnvVar use_podman_ce("ROX_COLLECTOR_CE_USE_PODMAN", false); BoolEnvVar enable_introspection("ROX_COLLECTOR_INTROSPECTION_ENABLE", false); +BoolEnvVar disable_process_arguments("ROX_COLLECTOR_NO_PROCESS_ARGUMENTS", false); + } // namespace constexpr bool CollectorConfig::kTurnOffScrape; @@ -87,6 +89,7 @@ void CollectorConfig::InitCollectorConfig(CollectorArgs* args) { use_docker_ce_ = use_docker_ce.value(); use_podman_ce_ = use_podman_ce.value(); enable_introspection_ = enable_introspection.value(); + disable_process_arguments_ = disable_process_arguments.value(); for (const auto& syscall : kSyscalls) { syscalls_.push_back(syscall); diff --git a/collector/lib/CollectorConfig.h b/collector/lib/CollectorConfig.h index 3af15a77b5..841746f847 100644 --- a/collector/lib/CollectorConfig.h +++ b/collector/lib/CollectorConfig.h @@ -87,6 +87,7 @@ class CollectorConfig { unsigned int GetSinspBufferSize() const; unsigned int GetSinspTotalBufferSize() const { return sinsp_total_buffer_size_; } unsigned int GetSinspThreadCacheSize() const { return sinsp_thread_cache_size_; } + bool DisableProcessArguments() const { return disable_process_arguments_; } std::shared_ptr grpc_channel; @@ -122,6 +123,8 @@ class CollectorConfig { double connection_stats_error_; unsigned int connection_stats_window_; + bool disable_process_arguments_ = false; + // One ring buffer will be initialized for this many CPUs unsigned int sinsp_cpu_per_buffer_ = 0; // Size of one ring buffer, in bytes. diff --git a/collector/lib/ProcessSignalFormatter.cpp b/collector/lib/ProcessSignalFormatter.cpp index 108d4348e8..7522ca62f8 100644 --- a/collector/lib/ProcessSignalFormatter.cpp +++ b/collector/lib/ProcessSignalFormatter.cpp @@ -52,7 +52,7 @@ std::string extract_proc_args(sinsp_threadinfo* tinfo) { } // namespace -ProcessSignalFormatter::ProcessSignalFormatter(sinsp* inspector) : event_names_(EventNames::GetInstance()), event_extractor_(std::make_unique()), container_metadata_(inspector) { +ProcessSignalFormatter::ProcessSignalFormatter(sinsp* inspector, const CollectorConfig& config) : event_names_(EventNames::GetInstance()), event_extractor_(std::make_unique()), container_metadata_(inspector), config_(config) { event_extractor_->Init(inspector); } @@ -135,11 +135,13 @@ ProcessSignal* ProcessSignalFormatter::CreateProcessSignal(sinsp_evt* event) { signal->set_exec_file_path(name_sanitized ? *name_sanitized : *name); } - // set process arguments - if (const char* args = event_extractor_->get_proc_args(event)) { - std::string args_str = args; - auto args_sanitized = SanitizedUTF8(args_str); - signal->set_args(args_sanitized ? *args_sanitized : args_str); + // set process arguments, if not explicitely disabled + if (!config_.DisableProcessArguments()) { + if (const char* args = event_extractor_->get_proc_args(event)) { + std::string args_str = args; + auto args_sanitized = SanitizedUTF8(args_str); + signal->set_args(args_sanitized ? *args_sanitized : args_str); + } } // set pid diff --git a/collector/lib/ProcessSignalFormatter.h b/collector/lib/ProcessSignalFormatter.h index 1aa6a3e665..0911929d29 100644 --- a/collector/lib/ProcessSignalFormatter.h +++ b/collector/lib/ProcessSignalFormatter.h @@ -7,6 +7,7 @@ #include "internalapi/sensor/signal_iservice.pb.h" #include "storage/process_indicator.pb.h" +#include "CollectorConfig.h" #include "CollectorStats.h" #include "ContainerMetadata.h" #include "EventNames.h" @@ -25,7 +26,7 @@ namespace collector { class ProcessSignalFormatter : public ProtoSignalFormatter { public: - ProcessSignalFormatter(sinsp* inspector); + ProcessSignalFormatter(sinsp* inspector, const CollectorConfig& config); ~ProcessSignalFormatter(); using Signal = v1::Signal; @@ -52,6 +53,8 @@ class ProcessSignalFormatter : public ProtoSignalFormatter event_extractor_; ContainerMetadata container_metadata_; + + CollectorConfig config_; }; } // namespace collector diff --git a/collector/lib/ProcessSignalHandler.h b/collector/lib/ProcessSignalHandler.h index e6e16a6965..525098709d 100644 --- a/collector/lib/ProcessSignalHandler.h +++ b/collector/lib/ProcessSignalHandler.h @@ -5,6 +5,7 @@ #include +#include "CollectorConfig.h" #include "ProcessSignalFormatter.h" #include "RateLimit.h" #include "SignalHandler.h" @@ -19,8 +20,9 @@ namespace collector { class ProcessSignalHandler : public SignalHandler { public: - ProcessSignalHandler(sinsp* inspector, ISignalServiceClient* client, system_inspector::Stats* stats) - : client_(client), formatter_(inspector), stats_(stats) {} + ProcessSignalHandler(sinsp* inspector, ISignalServiceClient* client, system_inspector::Stats* stats, + const CollectorConfig& config) + : client_(client), formatter_(inspector, config), stats_(stats), config_(config) {} bool Start() override; bool Stop() override; @@ -34,6 +36,8 @@ class ProcessSignalHandler : public SignalHandler { ProcessSignalFormatter formatter_; system_inspector::Stats* stats_; RateLimitCache rate_limiter_; + + CollectorConfig config_; }; } // namespace collector diff --git a/collector/lib/system-inspector/Service.cpp b/collector/lib/system-inspector/Service.cpp index dbd191e1a1..5080feed1c 100644 --- a/collector/lib/system-inspector/Service.cpp +++ b/collector/lib/system-inspector/Service.cpp @@ -62,7 +62,8 @@ void Service::Init(const CollectorConfig& config, std::shared_ptr(inspector_.get(), signal_client_.get(), - &userspace_stats_)); + &userspace_stats_, + config)); if (signal_handlers_.size() == 2) { // self-check handlers do not count towards this check, because they diff --git a/collector/test/ProcessSignalFormatterTest.cpp b/collector/test/ProcessSignalFormatterTest.cpp index c95f91c9dc..e132380022 100644 --- a/collector/test/ProcessSignalFormatterTest.cpp +++ b/collector/test/ProcessSignalFormatterTest.cpp @@ -18,8 +18,9 @@ namespace { TEST(ProcessSignalFormatterTest, NoProcessTest) { sinsp* inspector = NULL; CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector); + ProcessSignalFormatter processSignalFormatter(inspector, config); sinsp_threadinfo* tinfo = NULL; std::vector lineage; @@ -42,8 +43,9 @@ TEST(ProcessSignalFormatterTest, NoProcessTest) { TEST(ProcessSignalFormatterTest, ProcessWithoutParentTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 0; @@ -76,8 +78,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithoutParentTest) { TEST(ProcessSignalFormatterTest, ProcessWithParentTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -120,8 +123,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithParentTest) { TEST(ProcessSignalFormatterTest, ProcessWithParentWithPid0Test) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 0; @@ -159,8 +163,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithParentWithPid0Test) { TEST(ProcessSignalFormatterTest, ProcessWithParentWithSameNameTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -203,8 +208,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithParentWithSameNameTest) { TEST(ProcessSignalFormatterTest, ProcessWithTwoParentsTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -261,8 +267,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithTwoParentsTest) { TEST(ProcessSignalFormatterTest, ProcessWithTwoParentsWithTheSameNameTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -316,8 +323,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithTwoParentsWithTheSameNameTest) { TEST(ProcessSignalFormatterTest, ProcessCollapseParentChildWithSameNameTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -380,8 +388,9 @@ TEST(ProcessSignalFormatterTest, ProcessCollapseParentChildWithSameNameTest) { TEST(ProcessSignalFormatterTest, ProcessCollapseParentChildWithSameName2Test) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -447,8 +456,9 @@ TEST(ProcessSignalFormatterTest, ProcessCollapseParentChildWithSameName2Test) { TEST(ProcessSignalFormatterTest, ProcessWithUnrelatedProcessTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -514,8 +524,9 @@ TEST(ProcessSignalFormatterTest, ProcessWithUnrelatedProcessTest) { TEST(ProcessSignalFormatterTest, CountTwoCounterCallsTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 1; @@ -561,8 +572,9 @@ TEST(ProcessSignalFormatterTest, CountTwoCounterCallsTest) { TEST(ProcessSignalFormatterTest, Rox3377ProcessLineageWithNoVPidTest) { std::unique_ptr inspector(new sinsp()); CollectorStats& collector_stats = CollectorStats::GetOrCreate(); + CollectorConfig config; - ProcessSignalFormatter processSignalFormatter(inspector.get()); + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); auto tinfo = inspector->build_threadinfo(); tinfo->m_pid = 3; @@ -616,6 +628,137 @@ TEST(ProcessSignalFormatterTest, Rox3377ProcessLineageWithNoVPidTest) { CollectorStats::Reset(); } +TEST(ProcessSignalFormatterTest, EmptyArgument) { + std::unique_ptr inspector(new sinsp()); + CollectorConfig config; + + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); + + auto tinfo = inspector->build_threadinfo(); + tinfo->m_pid = 3; + tinfo->m_tid = 3; + tinfo->m_ptid = -1; + tinfo->m_vpid = 0; + tinfo->m_user.set_uid(42); + tinfo->m_container_id = ""; + tinfo->m_exepath = "qwerty"; + + sinsp_evt* evt = new sinsp_evt(); + scap_evt* s_evt = new scap_evt(); + s_evt->type = PPME_SYSCALL_EXECVE_19_X; + + // An empty argument + tinfo->set_args("", 0); + evt->set_tinfo(tinfo.get()); + evt->set_scap_evt(s_evt); + + EXPECT_NO_FATAL_FAILURE(processSignalFormatter.ToProtoMessage(evt)) << "Empty argument failed"; +} + +TEST(ProcessSignalFormatterTest, LargeArgumentWithoutNull) { + std::unique_ptr inspector(new sinsp()); + CollectorConfig config; + char* test; + + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); + + auto tinfo = inspector->build_threadinfo(); + tinfo->m_pid = 3; + tinfo->m_tid = 3; + tinfo->m_ptid = -1; + tinfo->m_vpid = 0; + tinfo->m_user.set_uid(42); + tinfo->m_container_id = ""; + tinfo->m_exepath = "qwerty"; + + sinsp_evt* evt = new sinsp_evt(); + scap_evt* s_evt = new scap_evt(); + s_evt->type = PPME_SYSCALL_EXECVE_19_X; + + // A large argument without a null terminator + test = (char*)malloc(8192); + memset(test, 0xff, 8192); + + tinfo->set_args(test, 8192); + evt->set_tinfo(tinfo.get()); + evt->set_scap_evt(s_evt); + + EXPECT_NO_FATAL_FAILURE(processSignalFormatter.ToProtoMessage(evt)) << "Large argument without a null terminator failed"; +} + +TEST(ProcessSignalFormatterTest, SmallArgumentWithoutNull) { + std::unique_ptr inspector(new sinsp()); + CollectorConfig config; + char* test; + + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); + + auto tinfo = inspector->build_threadinfo(); + tinfo->m_pid = 3; + tinfo->m_tid = 3; + tinfo->m_ptid = -1; + tinfo->m_vpid = 0; + tinfo->m_user.set_uid(42); + tinfo->m_container_id = ""; + tinfo->m_exepath = "qwerty"; + + sinsp_evt* evt = new sinsp_evt(); + scap_evt* s_evt = new scap_evt(); + s_evt->type = PPME_SYSCALL_EXECVE_19_X; + + // A small argument without a null terminator + test = (char*)malloc(200); + memset(test, 0xff, 200); + + tinfo->set_args(test, 200); + evt->set_tinfo(tinfo.get()); + evt->set_scap_evt(s_evt); + + EXPECT_NO_FATAL_FAILURE(processSignalFormatter.ToProtoMessage(evt)) << "Small argument without a null terminator failed"; +} + +TEST(ProcessSignalFormatterTest, InvalidUTF8) { + std::unique_ptr inspector(new sinsp()); + CollectorConfig config; + char* test; + + ProcessSignalFormatter processSignalFormatter(inspector.get(), config); + + auto tinfo = inspector->build_threadinfo(); + tinfo->m_pid = 3; + tinfo->m_tid = 3; + tinfo->m_ptid = -1; + tinfo->m_vpid = 0; + tinfo->m_user.set_uid(42); + tinfo->m_container_id = ""; + tinfo->m_exepath = "qwerty"; + + sinsp_evt* evt = new sinsp_evt(); + scap_evt* s_evt = new scap_evt(); + s_evt->type = PPME_SYSCALL_EXECVE_19_X; + + // An argument with invalid UTF-8 sequence + test = (char*)malloc(200); + memset(test, 0xff, 200); + test[0] = 0xe2; + test[1] = 0xcf; + test[2] = 0xcf; + test[3] = 0xff; + test[4] = 0xff; + test[5] = 0xff; + test[6] = 0xff; + test[7] = 0xff; + test[8] = 0xff; + test[9] = 0xff; + test[10] = 0xff; + + tinfo->set_args(test, 200); + evt->set_tinfo(tinfo.get()); + evt->set_scap_evt(s_evt); + + EXPECT_NO_FATAL_FAILURE(processSignalFormatter.ToProtoMessage(evt)) << "Invalid UTF-8 argument failed"; +} + } // namespace } // namespace collector