diff options
-rw-r--r-- | include/credential_pipe.hpp | 52 | ||||
-rw-r--r-- | meson.build | 1 | ||||
-rw-r--r-- | redfish-core/lib/virtual_media.hpp | 189 | ||||
-rw-r--r-- | test/include/credential_pipe_test.cpp | 41 |
4 files changed, 118 insertions, 165 deletions
diff --git a/include/credential_pipe.hpp b/include/credential_pipe.hpp new file mode 100644 index 0000000000..169d47c6cb --- /dev/null +++ b/include/credential_pipe.hpp @@ -0,0 +1,52 @@ +#pragma once + +#include <boost/asio/buffer.hpp> +#include <boost/asio/io_context.hpp> +#include <boost/asio/write.hpp> +#include <boost/process/async_pipe.hpp> + +#include <array> +#include <string> + +// Wrapper for boost::async_pipe ensuring proper pipe cleanup +class CredentialsPipe +{ + public: + explicit CredentialsPipe(boost::asio::io_context& io) : impl(io) {} + + CredentialsPipe(const CredentialsPipe&) = delete; + CredentialsPipe(CredentialsPipe&&) = delete; + CredentialsPipe& operator=(const CredentialsPipe&) = delete; + CredentialsPipe& operator=(CredentialsPipe&&) = delete; + + ~CredentialsPipe() + { + explicit_bzero(user.data(), user.capacity()); + explicit_bzero(pass.data(), pass.capacity()); + } + + int fd() const + { + return impl.native_source(); + } + + template <typename WriteHandler> + void asyncWrite(std::string&& username, std::string&& password, + WriteHandler&& handler) + { + user = std::move(username); + pass = std::move(password); + + // Add +1 to ensure that the null terminator is included. + std::array<boost::asio::const_buffer, 2> buffer{ + {{user.data(), user.size() + 1}, {pass.data(), pass.size() + 1}}}; + boost::asio::async_write(impl, buffer, + std::forward<WriteHandler>(handler)); + } + + boost::process::async_pipe impl; + + private: + std::string user; + std::string pass; +}; diff --git a/meson.build b/meson.build index 9f22f23370..4aa7c330a9 100644 --- a/meson.build +++ b/meson.build @@ -389,6 +389,7 @@ srcfiles_unittest = files( 'test/include/http_utility_test.cpp', 'test/include/human_sort_test.cpp', 'test/include/async_resolve_test.cpp', + 'test/include/credential_pipe_test.cpp', 'test/include/ibm/configfile_test.cpp', 'test/include/ibm/lock_test.cpp', 'test/include/multipart_test.cpp', diff --git a/redfish-core/lib/virtual_media.hpp b/redfish-core/lib/virtual_media.hpp index f3113fa616..d854ee53b6 100644 --- a/redfish-core/lib/virtual_media.hpp +++ b/redfish-core/lib/virtual_media.hpp @@ -18,6 +18,7 @@ #include "account_service.hpp" #include "app.hpp" #include "async_resp.hpp" +#include "credential_pipe.hpp" #include "dbus_utility.hpp" #include "generated/enums/virtual_media.hpp" #include "query.hpp" @@ -442,140 +443,6 @@ struct InsertMediaActionParams std::optional<bool> inserted; }; -template <typename T> -static void secureCleanup(T& value) -{ - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) - auto raw = const_cast<typename T::value_type*>(value.data()); - explicit_bzero(raw, value.size() * sizeof(*raw)); -} - -class Credentials -{ - public: - Credentials(std::string&& user, std::string&& password) : - userBuf(std::move(user)), passBuf(std::move(password)) - {} - - ~Credentials() - { - secureCleanup(userBuf); - secureCleanup(passBuf); - } - - const std::string& user() - { - return userBuf; - } - - const std::string& password() - { - return passBuf; - } - - Credentials() = delete; - Credentials(const Credentials&) = delete; - Credentials& operator=(const Credentials&) = delete; - Credentials(Credentials&&) = delete; - Credentials& operator=(Credentials&&) = delete; - - private: - std::string userBuf; - std::string passBuf; -}; - -class CredentialsProvider -{ - public: - template <typename T> - struct Deleter - { - void operator()(T* buff) const - { - if (buff) - { - secureCleanup(*buff); - delete buff; - } - } - }; - - using Buffer = std::vector<char>; - using SecureBuffer = std::unique_ptr<Buffer, Deleter<Buffer>>; - // Using explicit definition instead of std::function to avoid implicit - // conversions eg. stack copy instead of reference - using FormatterFunc = void(const std::string& username, - const std::string& password, Buffer& dest); - - CredentialsProvider(std::string&& user, std::string&& password) : - credentials(std::move(user), std::move(password)) - {} - - const std::string& user() - { - return credentials.user(); - } - - const std::string& password() - { - return credentials.password(); - } - - SecureBuffer pack(FormatterFunc* formatter) - { - SecureBuffer packed{new Buffer{}}; - if (formatter != nullptr) - { - formatter(credentials.user(), credentials.password(), *packed); - } - - return packed; - } - - private: - Credentials credentials; -}; - -// Wrapper for boost::async_pipe ensuring proper pipe cleanup -class SecurePipe -{ - public: - using unix_fd = sdbusplus::message::unix_fd; - - SecurePipe(boost::asio::io_context& io, - CredentialsProvider::SecureBuffer&& bufferIn) : - impl(io), - buffer{std::move(bufferIn)} - {} - - ~SecurePipe() - { - // Named pipe needs to be explicitly removed - impl.close(); - } - - SecurePipe(const SecurePipe&) = delete; - SecurePipe(SecurePipe&&) = delete; - SecurePipe& operator=(const SecurePipe&) = delete; - SecurePipe& operator=(SecurePipe&&) = delete; - - unix_fd fd() const - { - return unix_fd{impl.native_source()}; - } - - template <typename WriteHandler> - void asyncWrite(WriteHandler&& handler) - { - impl.async_write_some(boost::asio::buffer(*buffer), - std::forward<WriteHandler>(handler)); - } - - const std::string name; - boost::process::async_pipe impl; - CredentialsProvider::SecureBuffer buffer; -}; - /** * @brief Function transceives data with dbus directly. * @@ -583,54 +450,46 @@ class SecurePipe */ inline void doMountVmLegacy(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, const std::string& service, const std::string& name, - const std::string& imageUrl, const bool rw, + const std::string& imageUrl, bool rw, std::string&& userName, std::string&& password) { - constexpr const size_t secretLimit = 1024; - - std::shared_ptr<SecurePipe> secretPipe; - dbus::utility::DbusVariantType unixFd = -1; - + int fd = -1; + std::shared_ptr<CredentialsPipe> secretPipe; if (!userName.empty() || !password.empty()) { - // Encapsulate in safe buffer - CredentialsProvider credentials(std::move(userName), - std::move(password)); - // Payload must contain data + NULL delimiters - if (credentials.user().size() + credentials.password().size() + 2 > - secretLimit) + constexpr const size_t secretLimit = 1024; + if (userName.size() + password.size() + 2 > secretLimit) { BMCWEB_LOG_ERROR("Credentials too long to handle"); messages::unrecognizedRequestBody(asyncResp->res); return; } - // Pack secret - auto secret = credentials.pack( - [](const auto& user, const auto& pass, auto& buff) { - std::ranges::copy(user, std::back_inserter(buff)); - buff.push_back('\0'); - std::ranges::copy(pass, std::back_inserter(buff)); - buff.push_back('\0'); - }); - // Open pipe - secretPipe = std::make_shared<SecurePipe>( - crow::connections::systemBus->get_io_context(), std::move(secret)); - unixFd = secretPipe->fd(); + secretPipe = std::make_shared<CredentialsPipe>( + crow::connections::systemBus->get_io_context()); + fd = secretPipe->fd(); // Pass secret over pipe secretPipe->asyncWrite( - [asyncResp](const boost::system::error_code& ec, std::size_t) { + std::move(userName), std::move(password), + [asyncResp, secretPipe](const boost::system::error_code& ec, + std::size_t) { if (ec) { BMCWEB_LOG_ERROR("Failed to pass secret: {}", ec); messages::internalError(asyncResp->res); } - }); + }); } + dbus::utility::DbusVariantType unixFd( + std::in_place_type<sdbusplus::message::unix_fd>, fd); + + sdbusplus::message::object_path path( + "/xyz/openbmc_project/VirtualMedia/Legacy"); + path /= name; crow::connections::systemBus->async_method_call( [asyncResp, secretPipe](const boost::system::error_code& ec, bool success) { @@ -638,16 +497,16 @@ inline void doMountVmLegacy(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, { BMCWEB_LOG_ERROR("Bad D-Bus request error: {}", ec); messages::internalError(asyncResp->res); + return; } - else if (!success) + if (!success) { BMCWEB_LOG_ERROR("Service responded with error"); - messages::generalError(asyncResp->res); + messages::internalError(asyncResp->res); } }, - service, "/xyz/openbmc_project/VirtualMedia/Legacy/" + name, - "xyz.openbmc_project.VirtualMedia.Legacy", "Mount", imageUrl, rw, - unixFd); + service, path.str, "xyz.openbmc_project.VirtualMedia.Legacy", "Mount", + imageUrl, rw, unixFd); } /** diff --git a/test/include/credential_pipe_test.cpp b/test/include/credential_pipe_test.cpp new file mode 100644 index 0000000000..c5b544bc0c --- /dev/null +++ b/test/include/credential_pipe_test.cpp @@ -0,0 +1,41 @@ +#include "credential_pipe.hpp" + +#include <boost/asio/io_context.hpp> +#include <boost/beast/core/file_posix.hpp> + +#include <string> + +#include <gmock/gmock.h> + +using ::testing::ElementsAre; + +static void handler(boost::asio::io_context& io, + const boost::system::error_code& ec, size_t sent) +{ + io.stop(); + EXPECT_FALSE(ec); + EXPECT_EQ(sent, 18); +} + +TEST(CredentialsPipe, BasicSend) +{ + boost::beast::file_posix file; + { + boost::asio::io_context io; + CredentialsPipe pipe(io); + file.native_handle(dup(pipe.fd())); + ASSERT_GT(file.native_handle(), 0); + pipe.asyncWrite("username", "password", + std::bind_front(handler, std::ref(io))); + io.run(); + } + std::array<char, 18> buff{}; + boost::system::error_code ec; + size_t r = file.read(buff.data(), buff.size(), ec); + ASSERT_FALSE(ec); + ASSERT_EQ(r, 18); + + EXPECT_THAT(buff, + ElementsAre('u', 's', 'e', 'r', 'n', 'a', 'm', 'e', '\0', 'p', + 'a', 's', 's', 'w', 'o', 'r', 'd', '\0')); +} |