diff --git a/appsec/src/extension/commands_helpers.c b/appsec/src/extension/commands_helpers.c index 9cbd505121..d1df77d2da 100644 --- a/appsec/src/extension/commands_helpers.c +++ b/appsec/src/extension/commands_helpers.c @@ -29,8 +29,6 @@ static inline ATTR_WARN_UNUSED mpack_error_t _omsg_finish( static inline void _omsg_destroy(dd_omsg *nonnull omsg); static inline dd_result _omsg_send( dd_conn *nonnull conn, dd_omsg *nonnull omsg); -static inline dd_result _omsg_send_cred( - dd_conn *nonnull conn, dd_omsg *nonnull omsg); static void _dump_in_msg( dd_log_level_t lvl, const char *nonnull data, size_t data_len); static void _dump_out_msg(dd_log_level_t lvl, zend_llist *iovecs); @@ -42,16 +40,15 @@ typedef struct _dd_imsg { mpack_node_t root; } dd_imsg; -// iif these two return success, _imsg_destroy must be called -static inline dd_result _imsg_recv( - dd_imsg *nonnull imsg, dd_conn *nonnull conn); -static inline ATTR_WARN_UNUSED dd_result _imsg_recv_cred( +// if and only if this returns success, _imsg_destroy must be called +static dd_result ATTR_WARN_UNUSED _imsg_recv( dd_imsg *nonnull imsg, dd_conn *nonnull conn); static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy( dd_imsg *nonnull imsg); +static void _imsg_cleanup(dd_imsg *nullable *imsg); -static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, +static dd_result _dd_command_exec(dd_conn *nonnull conn, const dd_command_spec *nonnull spec, void *unspecnull ctx) { #define NAME_L (int)spec->name_len, spec->name @@ -78,11 +75,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, return dd_error; } - if (check_cred) { - res = _omsg_send_cred(conn, &omsg); - } else { - res = _omsg_send(conn, &omsg); - } + res = _omsg_send(conn, &omsg); _dump_out_msg(dd_log_trace, &omsg.iovecs); _omsg_destroy(&omsg); if (res) { @@ -96,11 +89,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, dd_result res; { dd_imsg imsg = {0}; - if (check_cred) { - res = _imsg_recv_cred(&imsg, conn); - } else { - res = _imsg_recv(&imsg, conn); - } + res = _imsg_recv(&imsg, conn); if (res) { if (res != dd_helper_error) { mlog(dd_log_warning, @@ -110,20 +99,21 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, return res; } + // automatic cleanup of imsg on error branches + // set to NULL before calling _imsg_destroy + __attribute__((cleanup(_imsg_cleanup))) dd_imsg *nullable destroy_imsg = + &imsg; + mpack_node_t first_response = mpack_node_array_at(imsg.root, 0); mpack_error_t err = mpack_node_error(first_response); if (err != mpack_ok) { mlog(dd_log_error, "Array of responses could not be retrieved - %s", mpack_error_to_string(err)); - // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores) - err = _imsg_destroy(&imsg); return dd_error; } if (mpack_node_type(first_response) != mpack_type_array) { mlog(dd_log_error, "Invalid response. Expected array but got %s", mpack_type_to_string(mpack_node_type(first_response))); - // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores) - err = _imsg_destroy(&imsg); return dd_error; } mpack_node_t first_message = mpack_node_array_at(first_response, 1); @@ -139,16 +129,12 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, if (err != mpack_ok) { mlog(dd_log_error, "Response type could not be retrieved - %s", mpack_error_to_string(err)); - // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores) - err = _imsg_destroy(&imsg); return dd_error; } if (mpack_node_type(type) != mpack_type_str) { mlog(dd_log_error, "Unexpected type field. Expected string but got %s", mpack_type_to_string(mpack_node_type(type))); - // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores) - err = _imsg_destroy(&imsg); return dd_error; } if (dd_mpack_node_lstr_eq(type, "config_features")) { @@ -159,8 +145,6 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, mlog(dd_log_debug, "Received message for command %.*s unexpected: %.*s\n", NAME_L, (int)mpack_node_strlen(type), mpack_node_str(type)); - // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores) - err = _imsg_destroy(&imsg); return dd_error; } @@ -169,6 +153,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, err = imsg.root.tree->error; _dump_in_msg(err == mpack_ok ? dd_log_trace : dd_log_debug, imsg._data, imsg._size); + destroy_imsg = NULL; err = _imsg_destroy(&imsg); if (err != mpack_ok) { mlog(dd_log_warning, @@ -194,20 +179,25 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred, dd_result ATTR_WARN_UNUSED dd_command_exec(dd_conn *nonnull conn, const dd_command_spec *nonnull spec, void *unspecnull ctx) { - return _dd_command_exec(conn, false, spec, ctx); + return _dd_command_exec(conn, spec, ctx); } dd_result ATTR_WARN_UNUSED dd_command_exec_req_info(dd_conn *nonnull conn, const dd_command_spec *nonnull spec, struct req_info *nonnull ctx) { ctx->command_name = spec->name; - return _dd_command_exec(conn, false, spec, ctx); + return _dd_command_exec(conn, spec, ctx); } dd_result ATTR_WARN_UNUSED dd_command_exec_cred(dd_conn *nonnull conn, const dd_command_spec *nonnull spec, void *unspecnull ctx) { - return _dd_command_exec(conn, true, spec, ctx); + dd_result res = dd_conn_check_credentials(conn); + if (res) { + return res; + } + + return _dd_command_exec(conn, spec, ctx); } // outgoing @@ -247,24 +237,13 @@ static inline dd_result _omsg_send(dd_conn *nonnull conn, dd_omsg *nonnull omsg) return dd_conn_sendv(conn, &omsg->iovecs); } -static inline dd_result _omsg_send_cred( - dd_conn *nonnull conn, dd_omsg *nonnull omsg) -{ - return dd_conn_sendv_cred(conn, &omsg->iovecs); -} - // incoming -static inline dd_result _dd_imsg_recv( - dd_imsg *nonnull imsg, dd_conn *nonnull conn, bool check_cred) +static ATTR_WARN_UNUSED dd_result _imsg_recv( + dd_imsg *nonnull imsg, dd_conn *nonnull conn) { mlog(dd_log_debug, "Will receive response from helper"); - dd_result res; - if (check_cred) { - res = dd_conn_recv_cred(conn, &imsg->_data, &imsg->_size); - } else { - res = dd_conn_recv(conn, &imsg->_data, &imsg->_size); - } + dd_result res = dd_conn_recv(conn, &imsg->_data, &imsg->_size); if (res) { return res; } @@ -290,17 +269,6 @@ static inline dd_result _dd_imsg_recv( return dd_success; } -ATTR_WARN_UNUSED dd_result _imsg_recv( - dd_imsg *nonnull imsg, dd_conn *nonnull conn) -{ - return _dd_imsg_recv(imsg, conn, false); -} -ATTR_WARN_UNUSED dd_result _imsg_recv_cred( - dd_imsg *nonnull imsg, dd_conn *nonnull conn) -{ - return _dd_imsg_recv(imsg, conn, true); -} - static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy( dd_imsg *nonnull imsg) { @@ -310,6 +278,14 @@ static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy( return mpack_tree_destroy(&imsg->_tree); } +static void _imsg_cleanup(dd_imsg *nullable *imsg) +{ + dd_imsg **imsg_c = (dd_imsg * nullable * nonnull) imsg; + if (*imsg_c) { + UNUSED(_imsg_destroy(*imsg_c)); + } +} + /* Baked response */ static void _add_appsec_span_data_frag(mpack_node_t node); diff --git a/appsec/src/extension/network.c b/appsec/src/extension/network.c index 6f770be3ef..962353420c 100644 --- a/appsec/src/extension/network.c +++ b/appsec/src/extension/network.c @@ -21,6 +21,10 @@ #include #include +#ifdef __APPLE__ +# include +#endif + #define HELPER_PROCESS_C_INCLUDES #include "ddappsec.h" #include "dddefs.h" @@ -197,51 +201,54 @@ dd_result dd_conn_sendv(dd_conn *nonnull conn, zend_llist *nonnull iovecs) iovs[i] = *iov; } - size_t total = sizeof(dd_header) + data_len; + const size_t total = sizeof(dd_header) + data_len; mlog_g(dd_log_debug, "About to send %zu + %zu bytes to helper", sizeof(dd_header), data_len); - ssize_t sent_bytes = writev(conn->socket, iovs, (int)iovecs_count + 1); - efree(iovs); - if (sent_bytes == -1) { - mlog_err(dd_log_info, "Error writing %zu bytes to helper", total); - return dd_network; - } - mlog_g(dd_log_debug, "Wrote %zu bytes", (size_t)sent_bytes); + size_t sent_bytes = 0; + struct iovec *ciovs = iovs; + int ciovs_count = (int)iovecs_count + 1; + while (true) { + const ssize_t written = writev(conn->socket, ciovs, ciovs_count); + if (written == -1) { + if (errno == EINTR) { + mlog(dd_log_debug, "writev() call interrupted. Retrying"); + continue; + } + mlog_err(dd_log_info, "Error writing %zu bytes to helper", total); + efree(iovs); + return dd_network; + } else if (written == 0) { + mlog(dd_log_info, "writev() call returned zero"); + efree(iovs); + return dd_network; + } - if ((size_t)sent_bytes != total) { - mlog(dd_log_info, - "Could not send the desired number of bytes. Total sent was %zu, " - "wanted %zu", - (size_t)sent_bytes, total); - return dd_network; - } + mlog_g(dd_log_debug, "Wrote %zu bytes", (size_t)written); + sent_bytes += written; + if (sent_bytes == total) { + break; + } - return dd_success; -} -#ifdef SO_PASSCRED -dd_result dd_conn_sendv_cred(dd_conn *nonnull conn, zend_llist *nonnull iovecs) -{ - // set SO_PASSCRED before sending the message. This is to try to - // ensure that the helper does not send a response ahead of our having - // had the chance to set SO_PASSCRED before calling recvmsg(), resulting in - // the credentials received having the overflowuid - int res = setsockopt( - conn->socket, SOL_SOCKET, SO_PASSCRED, &(int){1}, sizeof(int)); - if (res == -1) { - mlog_err( - dd_log_warning, "Call to setsockopt to get credentials failed"); - return dd_error; + // adjust iovecs + size_t uwritten = (size_t)written; + for (int i = 0; i < ciovs_count; ++i) { + struct iovec *ciov = &ciovs[i]; + if (uwritten >= ciov->iov_len) { + assert(i < ciovs_count - 1); + uwritten -= ciov->iov_len; + } else { + ciov->iov_base = (char *)ciov->iov_base + uwritten; + ciov->iov_len -= uwritten; + ciovs += i; + break; + } + } } - return dd_conn_sendv(conn, iovecs); -} -#else // no SO_PASSCRED -dd_result dd_conn_sendv_cred(dd_conn *nonnull conn, zend_llist *nonnull iovecs) -{ - return dd_conn_sendv(conn, iovecs); + efree(iovs); + return dd_success; } -#endif static dd_result _recv_message_body(int sock, char *nullable *nonnull data, size_t *nonnull data_len, size_t expected_size); @@ -253,23 +260,35 @@ dd_result dd_conn_recv(dd_conn *nonnull conn, char *nullable *nonnull data, } dd_header h; - ssize_t recv_bytes = recv(conn->socket, (void *)&h, sizeof(dd_header), 0); - if (recv_bytes == -1) { - mlog_err(dd_log_info, "Error receiving the header"); - return dd_network; - } - if (recv_bytes != sizeof(dd_header)) { - mlog(dd_log_info, "Could not read the full header. Read %zd", - recv_bytes); - return dd_network; + char *writep = (char *)&h; + char *const endp = writep + sizeof(h); + while (writep < endp) { + ssize_t recv_bytes = recv(conn->socket, writep, endp - writep, 0); + if (recv_bytes == -1) { + if (errno == EINTR) { + mlog(dd_log_debug, "recv() call interrupted. Retrying"); + continue; + } + mlog_err(dd_log_info, "Error receiving the header"); + return dd_network; + } + if (recv_bytes == 0) { + mlog(dd_log_info, "recv() call receiving header yielded no data"); + return dd_network; + } + writep += recv_bytes; } - if (strncmp(h.code, "dds", 3) != 0) { - mlog(dd_log_warning, "Invalid message header from helper"); + if (memcmp(h.code, "dds", 4) != 0) { + mlog(dd_log_warning, + "Invalid message header from helper. First four bytes are " + "0x%02X%02X%02X%02x", + h.code[0], h.code[1], h.code[2], h.code[3]); // to force the connection closed. It may be we half-read a previous // message, so a reconnection can help return dd_network; } + // size is in machine order if (h.size > MAX_RECV_MESSAGE_SIZE) { mlog(dd_log_warning, @@ -298,6 +317,10 @@ static dd_result _recv_message_body(int sock, char *nullable *nonnull data, while (remaining_bytes > 0) { ssize_t recv_bytes = recv(sock, buffer, remaining_bytes, 0); if (recv_bytes == -1) { + if (errno == EINTR) { + mlog(dd_log_debug, "recv() call interrupted. Retrying"); + continue; + } mlog_err(dd_log_info, "Error receiving the body of a message"); goto error; } @@ -319,83 +342,34 @@ static dd_result _recv_message_body(int sock, char *nullable *nonnull data, return dd_network; } -#ifdef SO_PASSCRED -static dd_result _check_credentials(struct cmsghdr *cmsgp); -dd_result dd_conn_recv_cred(dd_conn *nonnull conn, char *nullable *nonnull data, - size_t *nonnull data_len) +dd_result dd_conn_check_credentials(dd_conn *nonnull conn) { - if (conn == NULL || conn->socket <= 0 || data == NULL) { - mlog(dd_log_warning, "Invalid arguments. Bug"); - return dd_error; - } - - union { - char buf[CMSG_SPACE(sizeof(struct ucred))]; - struct cmsghdr _align; - } control; - - dd_header h; - struct iovec iov = { - .iov_base = &h, - .iov_len = sizeof h, - }; - struct msghdr msgh = { - .msg_iov = &iov, - .msg_iovlen = 1, - .msg_control = control.buf, - .msg_controllen = CMSG_LEN(sizeof(struct ucred)), - }; - - ssize_t recv_bytes = recvmsg(conn->socket, &msgh, 0); - if (recv_bytes == -1) { - mlog_err(dd_log_info, "Error receviving data from helper"); - // will return after setsockopt() call - } - - setsockopt(conn->socket, SOL_SOCKET, SO_PASSCRED, &(int){0}, sizeof(int)); - - if (recv_bytes == 0) { - mlog(dd_log_info, "No data received"); - return dd_network; - } - if ((size_t)recv_bytes < sizeof(h)) { - mlog(dd_log_info, "Not enough data received for the header"); - return dd_network; - } - - // check credentials - if (msgh.msg_flags & MSG_CTRUNC) { // NOLINT - mlog(dd_log_info, "Truncated ancillary data"); - } - dd_result ddres = _check_credentials(CMSG_FIRSTHDR(&msgh)); - if (ddres) { - return ddres; - } - - if (strncmp(h.code, "dds", 3) != 0) { - mlog(dd_log_warning, "Invalid message header from helper"); - return dd_network; - } - - return _recv_message_body(conn->socket, data, data_len, h.size); -} -static dd_result _check_credentials(struct cmsghdr *cmsgp) -{ - if (!cmsgp || cmsgp->cmsg_len != CMSG_LEN(sizeof(struct ucred))) { - mlog(dd_log_warning, - "Helper credentials: no ancillary data or incorrect size"); - return dd_network; - } - if (cmsgp->cmsg_level != SOL_SOCKET || - cmsgp->cmsg_type != SCM_CREDENTIALS) { - mlog(dd_log_warning, "Unexpect type of ancillary data"); +#ifdef __APPLE__ + struct xucred creds; +#else + struct ucred creds; +#endif + socklen_t len = sizeof(creds); +#ifdef __APPLE__ + if (getsockopt(conn->socket, SOL_LOCAL, LOCAL_PEERCRED, &creds, &len) == + -1) { +#else + if (getsockopt(conn->socket, SOL_SOCKET, SO_PEERCRED, &creds, &len) == -1) { +#endif + mlog_err( + dd_log_warning, "Error getting credentials of the helper process"); return dd_network; } - struct ucred creds; - memcpy(&creds, CMSG_DATA(cmsgp), sizeof(struct ucred)); // NOLINT - mlog_g(dd_log_debug, "Credentials: pid %d, uid %d, gid %d", (int)creds.pid, + uid_t uid; +#ifdef __APPLE__ + uid = creds.cr_uid; + mlog_g(dd_log_debug, "credentials: pid %u", (unsigned)creds.cr_uid); +#else + uid = creds.uid; + mlog_g(dd_log_debug, "credentials: pid %d, uid %d, gid %d", (int)creds.pid, (int)creds.uid, (int)creds.gid); +#endif tsrm_env_lock(); char *use_zend_alloc = getenv("USE_ZEND_ALLOC"); // NOLINT @@ -406,24 +380,18 @@ static dd_result _check_credentials(struct cmsghdr *cmsgp) return dd_success; } - if (creds.uid != geteuid()) { + if (uid != geteuid()) { mlog(dd_log_error, "Mismatch of effective uid between helper and this process. " - "Helper's uid is %d, ours is %d", - (int)creds.uid, (int)geteuid()); + "Helper's uid is %u, ours is %u", + (unsigned)uid, (int)geteuid()); return dd_network; } - mlog(dd_log_debug, "Helper's process credentials are correct"); + mlog(dd_log_debug, "Helper's process credentials are correct (uid %u)", + (unsigned)uid); return dd_success; } -#else // no SO_PASSCRED -dd_result dd_conn_recv_cred(dd_conn *nonnull conn, char *nullable *nonnull data, - size_t *nonnull data_len) -{ - return dd_conn_recv(conn, data, data_len); -} -#endif int dd_conn_destroy(dd_conn *nonnull conn) { diff --git a/appsec/src/extension/network.h b/appsec/src/extension/network.h index 14989bdf5a..af02ed10fc 100644 --- a/appsec/src/extension/network.h +++ b/appsec/src/extension/network.h @@ -1,14 +1,14 @@ // Unless explicitly stated otherwise all files in this repository are // dual-licensed under the Apache-2.0 License or BSD-3-Clause License. // -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2021 Datadog, Inc. +// This product includes software developed at Datadog +// (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc. #ifndef DD_NETWORK_H #define DD_NETWORK_H #include -#include #include +#include #include #include #include @@ -27,10 +27,10 @@ enum comm_type { typedef struct _dd_conn dd_conn; +dd_result dd_conn_check_credentials(dd_conn *nonnull conn); dd_result dd_conn_sendv(dd_conn *nonnull conn, zend_llist *nonnull iovecs); -dd_result dd_conn_sendv_cred(dd_conn *nonnull conn, zend_llist *nonnull iovecs); -dd_result dd_conn_recv(dd_conn *nonnull conn, char *nullable *nonnull data, size_t *nonnull data_len); -dd_result dd_conn_recv_cred(dd_conn *nonnull conn, char *nullable *nonnull data, size_t *nonnull data_len); +dd_result dd_conn_recv(dd_conn *nonnull conn, char *nullable *nonnull data, + size_t *nonnull data_len); // for helper_process #ifdef HELPER_PROCESS_C_INCLUDES @@ -48,5 +48,4 @@ dd_result dd_conn_set_timeout( #endif - #endif // DD_NETWORK_H