From c33ba00b914c267d14f395a1127aca5dda17fee2 Mon Sep 17 00:00:00 2001 From: Agata Olender Date: Mon, 13 Jan 2020 17:51:24 +0100 Subject: Authentication support for Legacy mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change introduces new 'Mount' API argument - UNIX_FD for named pipe. This named pipe is utilized to securely send secret data over D-Bus. Currently data consists of null-terminated char buffers with username and password. Data on receiving side is encapsulated into classes whose role is to: - keep secret as short-lived as possible - erase secret from memory when it's not needed - pass secrets (and format them) to another secure container with above capabilities New classes: - Credentials: is a class encapsulating login and password. It zeroes them at destruction. - CredentialProvider: contains Credentials, specifies SecureBuffer, allows to store credentials in SecureBuffer - SecureBuffer: char vector which zeroes itself at destruction, used to provision secret data - VolatileFile: class creating temporary file with 'owner-only' permissions in /tmp; at destruction overwrites it's contents with '*' and removes it New behavior: - when UNIX_FD is provided over D-Bus it's treated as open unix pipe. Data is read from this pipe and stored securely into CredentialsProvider - credentials are stored in applications inside CredentialsProvider object, encapsulated by unique_ptr for as long as it's needed - strings containing secrets are zeroed immediately after use - VolatileFile is used to securely pass credentials to nbdkit curl plugin instead of command line parameters. Tested: Manual and automated tests on WilsonCity platform: - positive and negative tests for authentication on both CIFS and HTTPS resources - error injection (ill-formed data transfered over pipe, pipe broken etc.) Change-Id: I608ae0380b8ad57110bc0939f71eb48604e7dc99 Signed-off-by: Adrian Ambrożewicz Signed-off-by: Agata Olender --- virtual-media/src/smb.hpp | 23 +++- virtual-media/src/state_machine.hpp | 102 ++++++++++++++-- virtual-media/src/utils.hpp | 224 ++++++++++++++++++++++++++++++++++++ 3 files changed, 336 insertions(+), 13 deletions(-) create mode 100644 virtual-media/src/utils.hpp diff --git a/virtual-media/src/smb.hpp b/virtual-media/src/smb.hpp index 3189770..62c3a44 100644 --- a/virtual-media/src/smb.hpp +++ b/virtual-media/src/smb.hpp @@ -1,6 +1,7 @@ #pragma once #include "logger.hpp" +#include "utils.hpp" #include @@ -16,7 +17,8 @@ class SmbShare { } - bool mount(const fs::path& remote, bool rw) + bool mount(const fs::path& remote, bool rw, + const std::unique_ptr& credentials) { LogMsg(Logger::Debug, "Trying to mount remote : ", remote); @@ -25,8 +27,27 @@ class SmbShare auto options = params + "," + perm; LogMsg(Logger::Debug, "Mounting with options: ", options); + std::string credentialsOpt; + if (!credentials) + { + LogMsg(Logger::Info, "Mounting as Guest"); + credentialsOpt = "guest,user=OpenBmc"; + } + else + { + LogMsg(Logger::Info, "Authenticating as ", credentials->user()); + credentialsOpt = "user=" + credentials->user() + + ",password=" + credentials->password(); + } + + options += "," + credentialsOpt; + auto ec = ::mount(remote.c_str(), mountDir.c_str(), "cifs", 0, options.c_str()); + + utils::secureCleanup(options); + utils::secureCleanup(credentialsOpt); + if (ec) { LogMsg(Logger::Error, "Mount failed with ec = ", ec, diff --git a/virtual-media/src/state_machine.hpp b/virtual-media/src/state_machine.hpp index 9cba565..a788043 100644 --- a/virtual-media/src/state_machine.hpp +++ b/virtual-media/src/state_machine.hpp @@ -4,6 +4,7 @@ #include "logger.hpp" #include "smb.hpp" #include "system.hpp" +#include "utils.hpp" #include @@ -455,15 +456,68 @@ struct MountPointStateMachine // Mount specialization if (isLegacy) { + using sdbusplus::message::unix_fd; + using optional_fd = std::variant; + iface->register_method( - "Mount", [& machine = state.machine, this, - handleMount](boost::asio::yield_context yield, - std::string imgUrl, bool rw) { + "Mount", [& machine = state.machine, this, handleMount]( + boost::asio::yield_context yield, + std::string imgUrl, bool rw, optional_fd fd) { LogMsg(Logger::Info, "[App]: Mount called on ", getObjectPath(machine), machine.name); machine.target = {imgUrl, rw}; - return handleMount(yield, machine); + + if (std::holds_alternative(fd)) + { + LogMsg(Logger::Debug, "[App] Extra data available"); + + // Open pipe and prepare output buffer + boost::asio::posix::stream_descriptor secretPipe( + machine.ioc.get(), + dup(std::get(fd).fd)); + std::array buf; + + // Read data + auto size = secretPipe.async_read_some( + boost::asio::buffer(buf), yield); + + // Validate number of NULL delimiters, ensures + // further operations are safe + auto nullCount = std::count( + buf.begin(), buf.begin() + size, '\0'); + if (nullCount != 2) + { + throw sdbusplus::exception::SdBusError( + EINVAL, "Malformed extra data"); + } + + // First 'part' of payload + std::string user(buf.begin()); + // Second 'part', after NULL delimiter + std::string pass(buf.begin() + user.length() + 1); + + // Encapsulate credentials into safe buffer + machine.target->credentials = + std::make_unique( + std::move(user), std::move(pass)); + + // Cover the tracks + utils::secureCleanup(buf); + } + + try + { + auto ret = handleMount(yield, machine); + machine.target->credentials.reset(); + return ret; + } + catch (...) + { + machine.target->credentials.reset(); + throw; + return false; + } }); } else @@ -644,7 +698,8 @@ struct MountPointStateMachine "\n Remote parent: ", remoteParent, "\n Local file: ", localFile); - if (!smb.mount(remoteParent, state.machine.target->rw)) + if (!smb.mount(remoteParent, state.machine.target->rw, + state.machine.target->credentials)) { fs::remove_all(*mountDir); return ReadyState(state, std::errc::invalid_argument, @@ -684,6 +739,7 @@ struct MountPointStateMachine static std::shared_ptr spawnNbdKit(MountPointStateMachine& machine, + std::unique_ptr&& secret, const std::vector& params) { // Investigate @@ -740,7 +796,8 @@ struct MountPointStateMachine args.insert(args.end(), params.begin(), params.end()); if (!process->spawn( - args, [& machine = machine](int exitCode, bool isReady) { + args, [& machine = machine, secret = std::move(secret)]( + int exitCode, bool isReady) { LogMsg(Logger::Info, machine.name, " process ended."); machine.emitSubprocessStoppedEvent(); })) @@ -756,24 +813,43 @@ struct MountPointStateMachine static std::shared_ptr spawnNbdKit(MountPointStateMachine& machine, const fs::path& file) { - return spawnNbdKit(machine, {// Use file plugin ... - "file", - // ... to mount file at this location - "file=" + file.string()}); + return spawnNbdKit(machine, {}, + {// Use file plugin ... + "file", + // ... to mount file at this location + "file=" + file.string()}); } static std::shared_ptr spawnNbdKit(MountPointStateMachine& machine, const std::string& url) { + std::unique_ptr secret; std::vector params = { // Use curl plugin ... "curl", // ... to mount http resource at url "url=" + url}; - // TODO: Authorization support in future patch + // Authenticate if needed + if (machine.target->credentials) + { + // Pack password into buffer + utils::CredentialsProvider::SecureBuffer buff = + machine.target->credentials->pack( + [](const std::string& user, const std::string& pass, + std::vector& buff) { + std::copy(pass.begin(), pass.end(), + std::back_inserter(buff)); + }); + + // Prepare file to provide the password with + secret = std::make_unique(std::move(buff)); + + params.push_back("user=" + machine.target->credentials->user()); + params.push_back("password=+" + secret->path()); + } - return spawnNbdKit(machine, params); + return spawnNbdKit(machine, std::move(secret), params); } bool checkUrl(const std::string& urlScheme, const std::string& imageUrl) @@ -939,6 +1015,7 @@ struct MountPointStateMachine name = std::move(machine.name); ioc = machine.ioc; config = std::move(machine.config); + target = std::move(machine.target); } return *this; } @@ -999,6 +1076,7 @@ struct MountPointStateMachine std::string imgUrl; bool rw; std::optional mountDir; + std::unique_ptr credentials; }; std::reference_wrapper ioc; diff --git a/virtual-media/src/utils.hpp b/virtual-media/src/utils.hpp new file mode 100644 index 0000000..f4d2c02 --- /dev/null +++ b/virtual-media/src/utils.hpp @@ -0,0 +1,224 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +namespace fs = std::filesystem; + +namespace utils +{ +constexpr const size_t secretLimit = 1024; + +template +static void secureCleanup(T& value) +{ + auto raw = const_cast(&value[0]); + 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; + } + + private: + Credentials() = delete; + Credentials(const Credentials&) = delete; + Credentials& operator=(const Credentials&) = delete; + + std::string userBuf; + std::string passBuf; +}; + +class CredentialsProvider +{ + public: + template + struct Deleter + { + void operator()(T* buff) const + { + if (buff) + { + secureCleanup(*buff); + delete buff; + } + } + }; + + using Buffer = std::vector; + using SecureBuffer = std::unique_ptr>; + // Using explicit definition instead of std::function to avoid implicit + // conversions eg. stack copy instead of reference for parameters + 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(const FormatterFunc formatter) + { + SecureBuffer packed{new Buffer{}}; + if (formatter) + { + formatter(credentials.user(), credentials.password(), *packed); + } + return packed; + } + + private: + Credentials credentials; +}; + +// Wrapper for boost::async_pipe ensuring proper pipe cleanup +template +class NamedPipe +{ + public: + using unix_fd = sdbusplus::message::unix_fd; + + NamedPipe(boost::asio::io_context& io, const std::string name, + Buffer&& buffer) : + name(name), + impl(io, name), buffer{std::move(buffer)} + { + } + + ~NamedPipe() + { + // Named pipe needs to be explicitly removed + impl.close(); + ::unlink(name.c_str()); + } + + unix_fd fd() + { + return unix_fd{impl.native_sink()}; + } + + const std::string& file() + { + return name; + } + + template + void async_write(WriteHandler&& handler) + { + impl.async_write_some(data(), std::forward(handler)); + } + + private: + // Specialization for pointer types + template + typename std::enable_if::value, + boost::asio::const_buffer>::type + data() + { + return boost::asio::buffer(*buffer); + } + + template + typename std::enable_if::value, + boost::asio::const_buffer>::type + data() + { + return boost::asio::buffer(buffer); + } + + const std::string name; + boost::process::async_pipe impl; + Buffer buffer; +}; + +class VolatileFile +{ + using Buffer = CredentialsProvider::SecureBuffer; + + public: + VolatileFile(Buffer&& contents) : + filePath(fs::temp_directory_path() / std::tmpnam(nullptr)), + size(contents->size()) + { + auto data = std::move(contents); + create(filePath, data); + } + + ~VolatileFile() + { + // Purge file contents + std::array buf; + buf.fill('*'); + std::ofstream file(filePath); + std::size_t bytesWritten = 0, bytesToWrite = 0; + + while (bytesWritten < size) + { + bytesToWrite = std::min(secretLimit, (size - bytesWritten)); + file.write(buf.data(), bytesToWrite); + bytesWritten += bytesToWrite; + } + + // Remove leftover file + fs::remove(filePath); + } + + const std::string& path() + { + return filePath; + } + + private: + static void create(const std::string& filePath, const Buffer& data) + { + // Create file + std::ofstream file(filePath); + + // Limit permissions to owner only + fs::permissions(filePath, + fs::perms::owner_read | fs::perms::owner_write, + fs::perm_options::replace); + + // Write contents + file.write(data->data(), data->size()); + } + + const std::string filePath; + const std::size_t size; +}; +} // namespace utils -- cgit v1.2.3