Merge pull request #3525 from DataDog/glopes/appsec-abstract-ns

Appsec: use abstract namespace on linux

Author: cataphract

.gitlab/generate-appsec.php

@@ -63,7 +63,7 @@
   before_script:
 <?php echo $ecrLoginSnippet, "\n"; ?>
 <?php dockerhub_login() ?>
-    - apt update && apt install -y default-jre
+    - apt update && apt install -y openjdk-17-jre
 
 "test appsec extension":
   stage: test
@@ -130,7 +130,7 @@
 <?php echo $ecrLoginSnippet, "\n"; ?>
 <?php dockerhub_login() ?>
   script:
-    - apt update && apt install -y default-jre
+    - apt update && apt install -y openjdk-17-jre
     - find "$CI_PROJECT_DIR"/appsec/tests/integration/build || true
     - |
       cd appsec/tests/integration

appsec/run-tests-internal.php

@@ -845,19 +845,19 @@ function write_information()
     $info_params = array();
     settings2array($ini_overwrites, $info_params);
     $info_params = settings2params($info_params);
-    $php_info = `$php $pass_options $info_params $no_file_cache "$info_file"`;
-    define('TESTED_PHP_VERSION', `$php -n -r "echo PHP_VERSION;"`);
+    $php_info = shell_exec("$php $pass_options $info_params $no_file_cache \"$info_file\"");
+    define('TESTED_PHP_VERSION', shell_exec("$php -n -r \"echo PHP_VERSION;\""));
 
     if ($php_cgi && $php != $php_cgi) {
-        $php_info_cgi = `$php_cgi $pass_options $info_params $no_file_cache -q "$info_file"`;
+        $php_info_cgi = shell_exec("$php_cgi $pass_options $info_params $no_file_cache -q \"$info_file\"");
         $php_info_sep = "\n---------------------------------------------------------------------";
         $php_cgi_info = "$php_info_sep\nPHP         : $php_cgi $php_info_cgi$php_info_sep";
     } else {
         $php_cgi_info = '';
     }
 
     if ($phpdbg) {
-        $phpdbg_info = `$phpdbg $pass_options $info_params $no_file_cache -qrr "$info_file"`;
+        $phpdbg_info = shell_exec("$phpdbg $pass_options $info_params $no_file_cache -qrr \"$info_file\"");
         $php_info_sep = "\n---------------------------------------------------------------------";
         $phpdbg_info = "$php_info_sep\nPHP         : $phpdbg $phpdbg_info$php_info_sep";
     } else {
@@ -872,7 +872,7 @@ function write_information()
     // load list of enabled extensions
     save_text($info_file,
         '<?php echo str_replace("Zend OPcache", "opcache", implode(",", get_loaded_extensions())); ?>');
-    $exts_to_test = explode(',', `$php $pass_options $info_params $no_file_cache "$info_file"`);
+    $exts_to_test = explode(',', shell_exec("$php $pass_options $info_params $no_file_cache \"$info_file\""));
     // check for extensions that need special handling and regenerate
     $info_params_ex = array(
         'session' => array('session.auto_start=0'),
@@ -2171,9 +2171,9 @@ function run_test($php, $file, array $env)
         $ext_params = array();
         settings2array($ini_overwrites, $ext_params);
         $ext_params = settings2params($ext_params);
-        $ext_dir = `$php $pass_options $extra_options $ext_params $no_file_cache -d display_errors=0 -r "echo ini_get('extension_dir');"`;
+        $ext_dir = shell_exec("$php $pass_options $extra_options $ext_params $no_file_cache -d display_errors=0 -r \"echo ini_get('extension_dir');\"");
         $extensions = preg_split("/[\n\r]+/", trim($section_text['EXTENSIONS']));
-        $loaded = explode(",", `$php $pass_options $extra_options $ext_params $no_file_cache -d display_errors=0 -r "echo implode(',', get_loaded_extensions());"`);
+        $loaded = explode(",", shell_exec("$php $pass_options $extra_options $ext_params $no_file_cache -d display_errors=0 -r \"echo implode(',', get_loaded_extensions());\""));
         $ext_prefix = IS_WINDOWS ? "php_" : "";
         foreach ($extensions as $req_ext) {
             if (!in_array($req_ext, $loaded)) {
@@ -3403,7 +3403,7 @@ function show_result(
     $tested,
     $tested_file,
     $extra = '',
-    array $temp_filenames = null
+    $temp_filenames = null
 ) {
     global $SHOW_ONLY_GROUPS, $colorize;
 

appsec/src/extension/configuration.h

@@ -29,7 +29,7 @@ extern bool runtime_config_first_init;
 #define DD_BASE(path) "/opt/datadog-php/"
 
 // clang-format off
-#define DD_CONFIGURATION \
+#define DD_CONFIGURATION_GENERAL \
     SYSCFG(BOOL, DD_APPSEC_ENABLED, "false")                                                                                          \
     SYSCFG(BOOL, DD_APPSEC_CLI_START_ON_RINIT, "false")                                                                               \
     SYSCFG(STRING, DD_APPSEC_RULES, "")                                                                                               \
@@ -50,7 +50,6 @@ extern bool runtime_config_first_init;
     SYSCFG(BOOL, DD_APPSEC_RASP_ENABLED , "true")                                                                                     \
     SYSCFG(INT, DD_APPSEC_MAX_STACK_TRACE_DEPTH, "32")                                                                                \
     SYSCFG(INT, DD_APPSEC_MAX_STACK_TRACES, "2")                                                                                      \
-    CONFIG(STRING, DD_APPSEC_HELPER_RUNTIME_PATH, "/tmp", .ini_change = dd_on_runtime_path_update)                                    \
     SYSCFG(STRING, DD_APPSEC_HELPER_LOG_FILE, "/dev/null")                                                                            \
     SYSCFG(STRING, DD_APPSEC_HELPER_LOG_LEVEL, "info")                                                                                \
     CONFIG(CUSTOM(SET), DD_EXTRA_SERVICES, "", .parser = _parse_list)                                                                 \
@@ -71,7 +70,17 @@ extern bool runtime_config_first_init;
     CONFIG(STRING, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, "")                                                                          \
     CONFIG(BOOL, DD_APM_TRACING_ENABLED, "true")                                                                                      \
     CONFIG(BOOL, DD_API_SECURITY_ENABLED, "true", .ini_change = zai_config_system_ini_change)                                         \
-    CONFIG(DOUBLE, DD_API_SECURITY_SAMPLE_DELAY, "30.0", .ini_change = zai_config_system_ini_change)                                  \
+    CONFIG(DOUBLE, DD_API_SECURITY_SAMPLE_DELAY, "30.0", .ini_change = zai_config_system_ini_change)
+
+#ifdef __linux__
+#define DD_CONFIGURATION \
+    DD_CONFIGURATION_GENERAL \
+    CONFIG(STRING, DD_APPSEC_HELPER_RUNTIME_PATH, "@/ddappsec", .ini_change = dd_on_runtime_path_update)
+#else
+#define DD_CONFIGURATION \
+    DD_CONFIGURATION_GENERAL \
+    CONFIG(STRING, DD_APPSEC_HELPER_RUNTIME_PATH, "/tmp", .ini_change = dd_on_runtime_path_update)
+#endif
 // clang-format on
 
 #define CALIAS CONFIG

appsec/src/extension/helper_process.c

@@ -41,8 +41,8 @@ typedef struct _dd_helper_mgr {
     bool connected_this_req;
     dd_helper_shared_state hss;
 
-    char *nonnull socket_path;
-    char *nonnull lock_path;
+    char *nonnull socket_path; // if abstract, starts with @
+    char *nonnull lock_path;   // set, but not used with abstract ns sockets
 } dd_helper_mgr;
 
 static _Atomic(dd_helper_shared_state) *_shared_state;

appsec/src/extension/network.c

@@ -49,7 +49,13 @@ static long _timespec_delta_ms(struct timespec *ts1, struct timespec *ts2);
 int dd_conn_init( // NOLINT(readability-function-cognitive-complexity)
     dd_conn *nonnull conn, const char *nonnull path, size_t path_len)
 {
-    if (path_len > sizeof(conn->addr.sun_path) - 1) {
+    if (path_len < 1) {
+        mlog(dd_log_error, "Socket path is too short");
+        return dd_error;
+    }
+    const bool is_abstract = path[0] == '@';
+
+    if (path_len > sizeof(conn->addr.sun_path) - (is_abstract ? 0 : 1)) {
         mlog(dd_log_error, "Socket path is too long");
         return dd_error;
     }
@@ -64,9 +70,19 @@ int dd_conn_init( // NOLINT(readability-function-cognitive-complexity)
 
     conn->addr.sun_family = AF_UNIX;
 
-    // NOLINTNEXTLINE
-    strncpy(conn->addr.sun_path, path, sizeof(conn->addr.sun_path) - 1);
-    conn->addr.sun_path[sizeof(conn->addr.sun_path) - 1] = '\0';
+    size_t addr_size;
+    if (is_abstract) {
+        // abstract namespace socket; replace @ with NUL byte
+        conn->addr.sun_path[0] = '\0';
+        strncpy(
+            conn->addr.sun_path + 1, path + 1, sizeof(conn->addr.sun_path) - 2);
+        conn->addr.sun_path[sizeof(conn->addr.sun_path) - 1] = '\0';
+        addr_size = path_len + offsetof(struct sockaddr_un, sun_path);
+    } else {
+        strncpy(conn->addr.sun_path, path, sizeof(conn->addr.sun_path) - 1);
+        conn->addr.sun_path[sizeof(conn->addr.sun_path) - 1] = '\0';
+        addr_size = sizeof(conn->addr);
+    }
 
     int flags_before = fcntl(conn->socket, F_GETFL, 0);
     if (flags_before == -1) {
@@ -82,19 +98,18 @@ int dd_conn_init( // NOLINT(readability-function-cognitive-complexity)
 
     mlog(dd_log_info, "Attempting to connect to UNIX socket %s", path);
     struct timespec deadline;
-    clock_gettime(CLOCK_MONOTONIC, &deadline);
+    UNUSED(clock_gettime(CLOCK_MONOTONIC, &deadline));
     _timespec_add_ms(&deadline, CONNECT_TIMEOUT);
 
 try_again:
-    res = connect(
-        conn->socket, (struct sockaddr *)&conn->addr, sizeof(conn->addr));
+    res = connect(conn->socket, (struct sockaddr *)&conn->addr, addr_size);
     if (res == -1) {
         int errno_copy = errno;
         if (errno_copy == EINPROGRESS) {
             struct pollfd pfds[] = {
                 {.fd = conn->socket, .events = POLLIN | POLLOUT}};
             struct timespec now;
-            clock_gettime(CLOCK_MONOTONIC, &now);
+            UNUSED(clock_gettime(CLOCK_MONOTONIC, &now));
             long time_left = _timespec_delta_ms(&deadline, &now);
             if (time_left <= 0) {
                 dd_conn_destroy(conn);
@@ -131,7 +146,7 @@ int dd_conn_init( // NOLINT(readability-function-cognitive-complexity)
             if (errno_copy == ENOENT || errno_copy == ECONNREFUSED) {
                 // the socket does not exist or is not being listened on. Retry
                 struct timespec now;
-                clock_gettime(CLOCK_MONOTONIC, &now);
+                UNUSED(clock_gettime(CLOCK_MONOTONIC, &now));
                 long time_left = _timespec_delta_ms(&deadline, &now);
                 if (time_left <= 0) {
                     dd_conn_destroy(conn);

appsec/src/helper/config.hpp

@@ -5,8 +5,6 @@
 // (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
 #pragma once
 
-#include <boost/lexical_cast.hpp>
-#include <chrono>
 #include <optional>
 #include <spdlog/spdlog.h>
 #include <string_view>
@@ -26,6 +24,15 @@ class config {
         return kv_.at(env_socket_file_path);
     }
 
+    [[nodiscard]] bool is_abstract_socket() const
+    {
+        std::string_view const sfp = socket_file_path();
+        if (sfp.empty()) {
+            return false;
+        }
+        return sfp[0] == '@';
+    }
+
     [[nodiscard]] std::string_view lock_file_path() const
     {
         return kv_.at(env_lock_file_path);

appsec/src/helper/main.cpp

@@ -25,8 +25,10 @@ extern "C" {
 #include <fcntl.h>
 #include <pthread.h>
 #include <sys/file.h>
+#include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <sys/un.h>
 }
 
 namespace {
@@ -60,7 +62,7 @@ void *pthread_wrapper(void *arg)
     return reinterpret_cast<void *>(ret);
 }
 
-bool ensure_unique(const std::string &lock_path)
+bool ensure_unique_lock(const std::string &lock_path)
 {
     // do not acquire the lock / assume we inherited it
     if (lock_path == "-") {
@@ -88,6 +90,44 @@ bool ensure_unique(const std::string &lock_path)
     return true;
 }
 
+bool ensure_unique_abstract_socket(std::string_view socket_path)
+{
+    if (socket_path.empty() || socket_path[0] != '@') {
+        return true;
+    }
+
+    // Try to connect to the socket. If successful, another helper is running
+    // NOLINTNEXTLINE(android-cloexec-socket)
+    int const sock = ::socket(AF_UNIX, SOCK_STREAM, 0);
+    if (sock == -1) {
+        SPDLOG_INFO("Failed to create test socket: errno {}", errno);
+        return false;
+    }
+
+    struct sockaddr_un addr {};
+    addr.sun_family = AF_UNIX;
+    addr.sun_path[0] = '\0';
+
+    std::size_t const size =
+        std::min(socket_path.size() - 1, sizeof(addr.sun_path) - 2);
+    std::copy_n(socket_path.data() + 1, size, &addr.sun_path[1]);
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index)
+    addr.sun_path[size + 1] = '\0';
+
+    int const res = ::connect(
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
+        sock, reinterpret_cast<struct sockaddr *>(&addr), sizeof(addr));
+    ::close(sock);
+
+    if (res == 0) {
+        SPDLOG_INFO("Another helper is already running on {}", socket_path);
+        return false;
+    }
+
+    SPDLOG_DEBUG("No helper running on socket {}, proceeding", socket_path);
+    return true;
+}
+
 int appsec_helper_main_impl()
 {
     dds::config::config const config{
@@ -124,10 +164,18 @@ int appsec_helper_main_impl()
 
     dds::waf::initialise_logging(level);
 
-    if (!ensure_unique(std::string{config.lock_file_path()})) {
-        logger->warn("helper launched, but not unique, exiting");
-        // There's another helper running
-        return 1;
+    if (config.is_abstract_socket()) {
+        if (!ensure_unique_abstract_socket(config.socket_file_path())) {
+            logger->warn("helper launched, but not unique (abstract socket "
+                         "exists), exiting");
+            return 1;
+        }
+    } else {
+        if (!ensure_unique_lock(std::string{config.lock_file_path()})) {
+            logger->warn(
+                "helper launched, but not unique (lock file exists), exiting");
+            return 1;
+        }
     }
 
     // block SIGUSR1 (only used to interrupt the runner)

appsec/src/helper/network/acceptor.cpp

@@ -29,42 +29,74 @@ acceptor::acceptor(const std::string_view &sv)
     }
 
     struct sockaddr_un addr {};
+    std::size_t addr_size;
     addr.sun_family = AF_UNIX;
-    if (sv.size() > sizeof(addr.sun_path) - 1) {
-        throw std::invalid_argument{"socket path too long"};
-    }
-    strcpy(static_cast<char *>(addr.sun_path), sv.data()); // NOLINT
+    bool const is_abstract = (!sv.empty() && sv[0] == '@');
 
-    // Remove the existing socket
-    int res = ::unlink(static_cast<char *>(addr.sun_path));
-    if (res == -1 && errno != ENOENT) {
-        SPDLOG_ERROR("Failed to unlink {}: errno {}", addr.sun_path, errno);
-        throw std::system_error(errno, std::generic_category());
+    if (is_abstract) {
+#ifdef __linux__
+        if (sv.size() > sizeof(addr.sun_path)) {
+            throw std::invalid_argument{"socket path too long"};
+        }
+        // Replace @ with null byte for abstract namespace
+        addr.sun_path[0] = '\0';
+        std::copy_n(sv.data() + 1, sv.size() - 1, &addr.sun_path[1]);
+        addr_size = sv.size() + offsetof(struct sockaddr_un, sun_path);
+#else
+        throw std::runtime_error{
+            "Abstract namespace sockets are only supported on Linux"};
+#endif
+    } else {
+        // Filesystem socket
+        if (sv.size() > sizeof(addr.sun_path) - 1) {
+            throw std::invalid_argument{"socket path too long"};
+        }
+        std::copy_n(sv.data(), sv.size(), &addr.sun_path[0]);
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index)
+        addr.sun_path[sv.size()] = '\0';
+        addr_size = sizeof(addr);
+
+        // Remove the existing socket
+        int const res = ::unlink(static_cast<char *>(addr.sun_path));
+        if (res == -1 && errno != ENOENT) {
+            SPDLOG_ERROR("Failed to unlink {}: errno {}", addr.sun_path, errno);
+            throw std::system_error(errno, std::generic_category());
+        }
+        SPDLOG_DEBUG("Unlinked {}", addr.sun_path);
     }
-    SPDLOG_DEBUG("Unlinked {}", addr.sun_path);
 
-    res =
+    int res = ::bind(
         // NOLINTNEXTLINE
-        ::bind(sock_.get(), reinterpret_cast<struct sockaddr *>(&addr),
-            sizeof(addr));
+        sock_.get(), reinterpret_cast<struct sockaddr *>(&addr), addr_size);
     if (res == -1) {
-        SPDLOG_ERROR(
-            "Failed to bind socket to {}: errno {}", addr.sun_path, errno);
+        if (is_abstract) {
+            SPDLOG_ERROR("Failed to bind abstract socket: errno {}", errno);
+        } else {
+            SPDLOG_ERROR(
+                "Failed to bind socket to {}: errno {}", addr.sun_path, errno);
+        }
         throw std::system_error(errno, std::generic_category());
     }
 
-    res = ::chmod(sv.data(), 0777); // NOLINT
-    if (res == -1) {
-        SPDLOG_ERROR(
-            "Failed to chmod socket {}: errno {}", addr.sun_path, errno);
-        throw std::system_error(errno, std::generic_category());
+    if (!is_abstract) {
+        res = ::chmod(sv.data(), 0777); // NOLINT
+        if (res == -1) {
+            SPDLOG_ERROR(
+                "Failed to chmod socket {}: errno {}", addr.sun_path, errno);
+            throw std::system_error(errno, std::generic_category());
+        }
     }
 
     static constexpr int backlog = 50;
     if (::listen(sock_.get(), backlog) == -1) {
         throw std::system_error(errno, std::generic_category());
     }
-    SPDLOG_INFO("Started listening on {}", sv);
+
+    if (is_abstract) {
+        SPDLOG_INFO("Started listening on abstract socket: {}", sv);
+    } else {
+        SPDLOG_INFO("Started listening on {}", sv);
+    }
 }
 
 void acceptor::set_accept_timeout(std::chrono::seconds timeout)