diff options
| -rw-r--r-- | ipc/BUILD.gn | 3 | ||||
| -rw-r--r-- | ipc/attachment_broker.cc | 24 | ||||
| -rw-r--r-- | ipc/attachment_broker.h | 29 | ||||
| -rw-r--r-- | ipc/attachment_broker_messages.h | 8 | ||||
| -rw-r--r-- | ipc/attachment_broker_privileged_win.cc | 129 | ||||
| -rw-r--r-- | ipc/attachment_broker_privileged_win.h | 62 | ||||
| -rw-r--r-- | ipc/attachment_broker_privileged_win_unittest.cc | 397 | ||||
| -rw-r--r-- | ipc/attachment_broker_win.cc | 29 | ||||
| -rw-r--r-- | ipc/attachment_broker_win.h | 23 | ||||
| -rw-r--r-- | ipc/brokerable_attachment.cc | 6 | ||||
| -rw-r--r-- | ipc/handle_attachment_win.cc | 6 | ||||
| -rw-r--r-- | ipc/handle_attachment_win.h | 1 | ||||
| -rw-r--r-- | ipc/ipc.gyp | 1 | ||||
| -rw-r--r-- | ipc/ipc.gypi | 2 | ||||
| -rw-r--r-- | ipc/ipc_channel_reader_unittest.cc | 21 | ||||
| -rw-r--r-- | ipc/ipc_channel_win.cc | 47 | ||||
| -rw-r--r-- | ipc/ipc_channel_win.h | 21 | ||||
| -rw-r--r-- | ipc/ipc_test_base.cc | 2 | ||||
| -rw-r--r-- | ipc/ipc_test_base.h | 11 |
19 files changed, 733 insertions, 89 deletions
diff --git a/ipc/BUILD.gn b/ipc/BUILD.gn index e467649..75d0c62 100644 --- a/ipc/BUILD.gn +++ b/ipc/BUILD.gn @@ -9,6 +9,8 @@ component("ipc") { "attachment_broker.cc", "attachment_broker.h", "attachment_broker_messages.h", + "attachment_broker_privileged_win.cc", + "attachment_broker_privileged_win.h", "attachment_broker_win.cc", "attachment_broker_win.h", "brokerable_attachment.cc", @@ -117,6 +119,7 @@ if (!is_android) { test("ipc_tests") { sources = [ + "attachment_broker_privileged_win_unittest.cc", "attachment_broker_win_unittest.cc", "ipc_channel_posix_unittest.cc", "ipc_channel_reader_unittest.cc", diff --git a/ipc/attachment_broker.cc b/ipc/attachment_broker.cc index 9f811ce..7450ec3 100644 --- a/ipc/attachment_broker.cc +++ b/ipc/attachment_broker.cc @@ -8,9 +8,21 @@ namespace IPC { -AttachmentBroker::AttachmentBroker() { -} -AttachmentBroker::~AttachmentBroker() { +AttachmentBroker::AttachmentBroker() {} +AttachmentBroker::~AttachmentBroker() {} + +bool AttachmentBroker::GetAttachmentWithId( + BrokerableAttachment::AttachmentId id, + scoped_refptr<BrokerableAttachment>* out_attachment) { + for (AttachmentVector::iterator it = attachments_.begin(); + it != attachments_.end(); ++it) { + if ((*it)->GetIdentifier() == id) { + *out_attachment = *it; + attachments_.erase(it); + return true; + } + } + return false; } void AttachmentBroker::AddObserver(AttachmentBroker::Observer* observer) { @@ -25,6 +37,12 @@ void AttachmentBroker::RemoveObserver(AttachmentBroker::Observer* observer) { observers_.erase(it); } +void AttachmentBroker::HandleReceivedAttachment( + const scoped_refptr<BrokerableAttachment>& attachment) { + attachments_.push_back(attachment); + NotifyObservers(attachment->GetIdentifier()); +} + void AttachmentBroker::NotifyObservers( const BrokerableAttachment::AttachmentId& id) { // Make a copy of observers_ to avoid mutations during iteration. diff --git a/ipc/attachment_broker.h b/ipc/attachment_broker.h index a7bb0e9..9bd6114 100644 --- a/ipc/attachment_broker.h +++ b/ipc/attachment_broker.h @@ -5,6 +5,7 @@ #ifndef IPC_ATTACHMENT_BROKER_H_ #define IPC_ATTACHMENT_BROKER_H_ +#include "base/gtest_prod_util.h" #include "base/macros.h" #include "base/process/process_handle.h" #include "ipc/brokerable_attachment.h" @@ -61,21 +62,41 @@ class IPC_EXPORT AttachmentBroker : public Listener { // Returns whether the attachment was available. If the attachment was // available, populates the output parameter |attachment|. - virtual bool GetAttachmentWithId( - BrokerableAttachment::AttachmentId id, - scoped_refptr<BrokerableAttachment>* attachment) = 0; + bool GetAttachmentWithId(BrokerableAttachment::AttachmentId id, + scoped_refptr<BrokerableAttachment>* attachment); // Any given observer should only ever add itself once to the observer list. void AddObserver(Observer* observer); void RemoveObserver(Observer* observer); protected: + using AttachmentVector = std::vector<scoped_refptr<BrokerableAttachment>>; + + // Adds |attachment| to |attachments_|, and notifies the observers. + void HandleReceivedAttachment( + const scoped_refptr<BrokerableAttachment>& attachment); + + // Informs the observers that a new BrokerableAttachment has been received. void NotifyObservers(const BrokerableAttachment::AttachmentId& id); + // This method is exposed for testing only. + AttachmentVector* get_attachments() { return &attachments_; } + private: - DISALLOW_COPY_AND_ASSIGN(AttachmentBroker); +#if defined(OS_WIN) + FRIEND_TEST_ALL_PREFIXES(AttachmentBrokerWinTest, ReceiveValidMessage); + FRIEND_TEST_ALL_PREFIXES(AttachmentBrokerWinTest, ReceiveInvalidMessage); +#endif // defined(OS_WIN) + + // A vector of BrokerableAttachments that have been received, but not yet + // consumed. + // A std::vector is used instead of a std::map because this container is + // expected to have few elements, for which a std::vector is expected to have + // better performance. + AttachmentVector attachments_; std::vector<Observer*> observers_; + DISALLOW_COPY_AND_ASSIGN(AttachmentBroker); }; } // namespace IPC diff --git a/ipc/attachment_broker_messages.h b/ipc/attachment_broker_messages.h index eed2518..71b8986 100644 --- a/ipc/attachment_broker_messages.h +++ b/ipc/attachment_broker_messages.h @@ -5,6 +5,7 @@ // IPC messages used by the attachment broker. // Multiply-included message file, hence no include guard. +#include "base/process/process_handle.h" #include "ipc/brokerable_attachment.h" #include "ipc/ipc_export.h" #include "ipc/ipc_message_macros.h" @@ -50,7 +51,10 @@ IPC_MESSAGE_CONTROL1( // Sent from a non-broker to a broker to request the duplication of a HANDLE // into a different process (possibly the broker process, or even the original // process). -IPC_MESSAGE_CONTROL1( +// TODO(erikchen): https://code.google.com/p/chromium/issues/detail?id=513431 +// *sender_pid* should be unforgeably generated by the receiving channel. +IPC_MESSAGE_CONTROL2( AttachmentBrokerMsg_DuplicateWinHandle, - IPC::internal::HandleAttachmentWin::WireFormat /* wire_format */) + IPC::internal::HandleAttachmentWin::WireFormat /* wire_format */, + base::ProcessId /* sender_pid */) #endif // defined(OS_WIN) diff --git a/ipc/attachment_broker_privileged_win.cc b/ipc/attachment_broker_privileged_win.cc new file mode 100644 index 0000000..52918a3 --- /dev/null +++ b/ipc/attachment_broker_privileged_win.cc @@ -0,0 +1,129 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ipc/attachment_broker_privileged_win.h" + +#include <windows.h> + +#include "base/process/process.h" +#include "ipc/attachment_broker_messages.h" +#include "ipc/brokerable_attachment.h" +#include "ipc/handle_attachment_win.h" +#include "ipc/ipc_channel.h" + +namespace IPC { + +AttachmentBrokerPrivilegedWin::AttachmentBrokerPrivilegedWin() {} + +AttachmentBrokerPrivilegedWin::~AttachmentBrokerPrivilegedWin() {} + +bool AttachmentBrokerPrivilegedWin::SendAttachmentToProcess( + const BrokerableAttachment* attachment, + base::ProcessId destination_process) { + switch (attachment->GetBrokerableType()) { + case BrokerableAttachment::WIN_HANDLE: + const internal::HandleAttachmentWin* handle_attachment = + static_cast<const internal::HandleAttachmentWin*>(attachment); + HandleWireFormat wire_format = + handle_attachment->GetWireFormat(destination_process); + HandleWireFormat new_wire_format = + DuplicateWinHandle(wire_format, base::Process::Current().Pid()); + if (new_wire_format.handle == 0) + return false; + RouteDuplicatedHandle(new_wire_format); + return true; + } + return false; +} + +bool AttachmentBrokerPrivilegedWin::OnMessageReceived(const Message& msg) { + bool handled = true; + IPC_BEGIN_MESSAGE_MAP(AttachmentBrokerPrivilegedWin, msg) + IPC_MESSAGE_HANDLER(AttachmentBrokerMsg_DuplicateWinHandle, + OnDuplicateWinHandle) + IPC_MESSAGE_UNHANDLED(handled = false) + IPC_END_MESSAGE_MAP() + return handled; +} + +void AttachmentBrokerPrivilegedWin::RegisterCommunicationChannel( + Channel* channel) { + auto it = std::find(channels_.begin(), channels_.end(), channel); + DCHECK(channels_.end() == it); + channels_.push_back(channel); +} + +void AttachmentBrokerPrivilegedWin::DeregisterCommunicationChannel( + Channel* channel) { + auto it = std::find(channels_.begin(), channels_.end(), channel); + DCHECK(it != channels_.end()); + channels_.erase(it); +} + +void AttachmentBrokerPrivilegedWin::OnDuplicateWinHandle( + const HandleWireFormat& wire_format, + base::ProcessId source_pid) { + if (wire_format.destination_process == base::kNullProcessId) + return; + + HandleWireFormat new_wire_format = + DuplicateWinHandle(wire_format, source_pid); + RouteDuplicatedHandle(new_wire_format); +} + +void AttachmentBrokerPrivilegedWin::RouteDuplicatedHandle( + const HandleWireFormat& wire_format) { + // This process is the destination. + if (wire_format.destination_process == base::Process::Current().Pid()) { + scoped_refptr<BrokerableAttachment> attachment( + new internal::HandleAttachmentWin(wire_format)); + HandleReceivedAttachment(attachment); + return; + } + + // Another process is the destination. + base::ProcessId dest = wire_format.destination_process; + auto it = + std::find_if(channels_.begin(), channels_.end(), + [dest](Channel* c) { return c->GetPeerPID() == dest; }); + if (it == channels_.end()) { + // Assuming that this message was not sent from a malicious process, the + // channel endpoint that would have received this message will block + // forever. + LOG(ERROR) << "Failed to deliver brokerable attachment to process with id: " + << dest; + return; + } + + (*it)->Send(new AttachmentBrokerMsg_WinHandleHasBeenDuplicated(wire_format)); +} + +AttachmentBrokerPrivilegedWin::HandleWireFormat +AttachmentBrokerPrivilegedWin::DuplicateWinHandle( + const HandleWireFormat& wire_format, + base::ProcessId source_pid) { + HandleWireFormat new_wire_format; + new_wire_format.destination_process = wire_format.destination_process; + new_wire_format.attachment_id = wire_format.attachment_id; + + HANDLE original_handle = LongToHandle(wire_format.handle); + + base::Process source_process = + base::Process::OpenWithExtraPrivileges(source_pid); + base::Process dest_process = + base::Process::OpenWithExtraPrivileges(wire_format.destination_process); + if (source_process.Handle() && dest_process.Handle()) { + HANDLE new_handle; + DWORD result = ::DuplicateHandle(source_process.Handle(), original_handle, + dest_process.Handle(), &new_handle, 0, + FALSE, DUPLICATE_SAME_ACCESS); + + new_wire_format.handle = (result != 0) ? HandleToLong(new_handle) : 0; + } else { + new_wire_format.handle = 0; + } + return new_wire_format; +} + +} // namespace IPC diff --git a/ipc/attachment_broker_privileged_win.h b/ipc/attachment_broker_privileged_win.h new file mode 100644 index 0000000..3cb07d4 --- /dev/null +++ b/ipc/attachment_broker_privileged_win.h @@ -0,0 +1,62 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef IPC_ATTACHMENT_BROKER_PRIVILEGED_WIN_H_ +#define IPC_ATTACHMENT_BROKER_PRIVILEGED_WIN_H_ + +#include <vector> + +#include "ipc/attachment_broker.h" +#include "ipc/handle_attachment_win.h" +#include "ipc/ipc_export.h" + +namespace IPC { + +class Channel; + +// This class is an implementation of AttachmentBroker for a privileged process +// on the Windows platform. When unprivileged processes want to send +// attachments, the attachments get routed through the privileged process, and +// more specifically, an instance of this class. +class IPC_EXPORT AttachmentBrokerPrivilegedWin : public AttachmentBroker { + public: + AttachmentBrokerPrivilegedWin(); + ~AttachmentBrokerPrivilegedWin() override; + + // IPC::AttachmentBroker overrides. + bool SendAttachmentToProcess(const BrokerableAttachment* attachment, + base::ProcessId destination_process) override; + + // IPC::Listener overrides. + bool OnMessageReceived(const Message& message) override; + + // Each unprivileged process should have one IPC channel on which it + // communicates attachment information with this privileged process. These + // channels must be registered and deregistered with the Attachment Broker as + // they are created and destroyed. + void RegisterCommunicationChannel(Channel* channel); + void DeregisterCommunicationChannel(Channel* channel); + + private: + using HandleWireFormat = internal::HandleAttachmentWin::WireFormat; + // IPC message handlers. + void OnDuplicateWinHandle(const HandleWireFormat& wire_format, + base::ProcessId source_process); + + // Duplicates |wire_Format| from |source_process| into its destination + // process. + HandleWireFormat DuplicateWinHandle(const HandleWireFormat& wire_format, + base::ProcessId source_process); + + // If the HANDLE's destination is this process, queue it and notify the + // observers. Otherwise, send it in an IPC to its destination. + void RouteDuplicatedHandle(const HandleWireFormat& wire_format); + + std::vector<Channel*> channels_; + DISALLOW_COPY_AND_ASSIGN(AttachmentBrokerPrivilegedWin); +}; + +} // namespace IPC + +#endif // IPC_ATTACHMENT_BROKER_PRIVILEGED_WIN_H_ diff --git a/ipc/attachment_broker_privileged_win_unittest.cc b/ipc/attachment_broker_privileged_win_unittest.cc new file mode 100644 index 0000000..ea0c06d --- /dev/null +++ b/ipc/attachment_broker_privileged_win_unittest.cc @@ -0,0 +1,397 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "build/build_config.h" + +#include <windows.h> + +#include "base/files/file_path.h" +#include "base/files/file_util.h" +#include "base/files/scoped_temp_dir.h" +#include "base/memory/scoped_ptr.h" +#include "ipc/attachment_broker_privileged_win.h" +#include "ipc/attachment_broker_win.h" +#include "ipc/handle_attachment_win.h" +#include "ipc/ipc_listener.h" +#include "ipc/ipc_message.h" +#include "ipc/ipc_test_base.h" + +namespace { + +const char kDataBuffer[] = "This is some test data to write to the file."; + +// Returns the contents of the file represented by |h| as a std::string. +std::string ReadFromFile(HANDLE h) { + SetFilePointer(h, 0, nullptr, FILE_BEGIN); + char buffer[100]; + DWORD bytes_read; + BOOL success = ::ReadFile(h, buffer, static_cast<DWORD>(strlen(kDataBuffer)), + &bytes_read, nullptr); + return success ? std::string(buffer, bytes_read) : std::string(); +} + +HANDLE GetHandleFromBrokeredAttachment( + const scoped_refptr<IPC::BrokerableAttachment>& attachment) { + if (attachment->GetType() != + IPC::BrokerableAttachment::TYPE_BROKERABLE_ATTACHMENT) + return nullptr; + if (attachment->GetBrokerableType() != IPC::BrokerableAttachment::WIN_HANDLE) + return nullptr; + IPC::internal::HandleAttachmentWin* received_handle_attachment = + static_cast<IPC::internal::HandleAttachmentWin*>(attachment.get()); + return received_handle_attachment->get_handle(); +} + +// Returns true if |attachment| is a file HANDLE whose contents is +// |kDataBuffer|. +bool CheckContentsOfBrokeredAttachment( + const scoped_refptr<IPC::BrokerableAttachment>& attachment) { + HANDLE h = GetHandleFromBrokeredAttachment(attachment); + if (h == nullptr) + return false; + + std::string contents = ReadFromFile(h); + return contents == std::string(kDataBuffer); +} + +enum TestResult { + RESULT_UNKNOWN, + RESULT_SUCCESS, + RESULT_FAILURE, +}; + +// Once the test is finished, send a control message to the parent process with +// the result. The message may require the runloop to be run before its +// dispatched. +void SendControlMessage(IPC::Sender* sender, bool success) { + IPC::Message* message = new IPC::Message(0, 2, IPC::Message::PRIORITY_NORMAL); + TestResult result = success ? RESULT_SUCCESS : RESULT_FAILURE; + message->WriteInt(result); + sender->Send(message); +} + +class MockObserver : public IPC::AttachmentBroker::Observer { + public: + void ReceivedBrokerableAttachmentWithId( + const IPC::BrokerableAttachment::AttachmentId& id) override { + id_ = id; + } + IPC::BrokerableAttachment::AttachmentId* get_id() { return &id_; } + + private: + IPC::BrokerableAttachment::AttachmentId id_; +}; + +// Forwards all messages to |listener_|. Quits the message loop after a +// message is received, or the channel has an error. +class ProxyListener : public IPC::Listener { + public: + ProxyListener() : reason_(MESSAGE_RECEIVED) {} + ~ProxyListener() override {} + + // The reason for exiting the message loop. + enum Reason { MESSAGE_RECEIVED, CHANNEL_ERROR }; + + bool OnMessageReceived(const IPC::Message& message) override { + bool result = listener_->OnMessageReceived(message); + reason_ = MESSAGE_RECEIVED; + base::MessageLoop::current()->Quit(); + return result; + } + + void OnChannelError() override { + reason_ = CHANNEL_ERROR; + base::MessageLoop::current()->Quit(); + } + + void set_listener(IPC::Listener* listener) { listener_ = listener; } + Reason get_reason() { return reason_; } + + private: + IPC::Listener* listener_; + Reason reason_; +}; + +// Waits for a result to be sent over the channel. Quits the message loop +// after a message is received, or the channel has an error. +class ResultListener : public IPC::Listener { + public: + ResultListener() : result_(RESULT_UNKNOWN) {} + ~ResultListener() override {} + + bool OnMessageReceived(const IPC::Message& message) override { + base::PickleIterator iter(message); + + int result; + EXPECT_TRUE(iter.ReadInt(&result)); + result_ = static_cast<TestResult>(result); + return true; + } + + TestResult get_result() { return result_; } + + private: + TestResult result_; +}; + +// The parent process acts as an unprivileged process. The forked process acts +// as the privileged process. +class IPCAttachmentBrokerPrivilegedWinTest : public IPCTestBase { + public: + IPCAttachmentBrokerPrivilegedWinTest() : message_index_(0) {} + ~IPCAttachmentBrokerPrivilegedWinTest() override {} + + void SetUp() override { + IPCTestBase::SetUp(); + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_dir_.path(), &temp_path_)); + } + + void TearDown() override { IPCTestBase::TearDown(); } + + // Takes ownership of |broker|. Has no effect if called after CommonSetUp(). + void set_broker(IPC::AttachmentBrokerWin* broker) { broker_.reset(broker); } + + void CommonSetUp() { + if (!broker_.get()) + set_broker(new IPC::AttachmentBrokerWin); + broker_->AddObserver(&observer_); + set_attachment_broker(broker_.get()); + CreateChannel(&proxy_listener_); + broker_->set_sender(channel()); + ASSERT_TRUE(ConnectChannel()); + ASSERT_TRUE(StartClient()); + } + + void CommonTearDown() { + // Close the channel so the client's OnChannelError() gets fired. + channel()->Close(); + + EXPECT_TRUE(WaitForClientShutdown()); + DestroyChannel(); + broker_.reset(); + } + + HANDLE CreateTempFile() { + EXPECT_NE(-1, WriteFile(temp_path_, kDataBuffer, + static_cast<int>(strlen(kDataBuffer)))); + + HANDLE h = + CreateFile(temp_path_.value().c_str(), GENERIC_READ | GENERIC_WRITE, 0, + nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); + EXPECT_NE(h, INVALID_HANDLE_VALUE); + return h; + } + + void SendMessageWithAttachment(HANDLE h) { + IPC::Message* message = + new IPC::Message(0, 2, IPC::Message::PRIORITY_NORMAL); + message->WriteInt(message_index_++); + scoped_refptr<IPC::internal::HandleAttachmentWin> attachment( + new IPC::internal::HandleAttachmentWin(h)); + ASSERT_TRUE(message->WriteAttachment(attachment)); + sender()->Send(message); + } + + ProxyListener* get_proxy_listener() { return &proxy_listener_; } + IPC::AttachmentBrokerWin* get_broker() { return broker_.get(); } + MockObserver* get_observer() { return &observer_; } + + private: + base::ScopedTempDir temp_dir_; + base::FilePath temp_path_; + int message_index_; + ProxyListener proxy_listener_; + scoped_ptr<IPC::AttachmentBrokerWin> broker_; + MockObserver observer_; +}; + +// A broker which always sets the current process as the destination process +// for attachments. +class MockBroker : public IPC::AttachmentBrokerWin { + public: + MockBroker() {} + ~MockBroker() override {} + bool SendAttachmentToProcess(const IPC::BrokerableAttachment* attachment, + base::ProcessId destination_process) override { + return IPC::AttachmentBrokerWin::SendAttachmentToProcess( + attachment, base::Process::Current().Pid()); + } +}; + +// An unprivileged process makes a file HANDLE, and writes a string to it. The +// file HANDLE is sent to the privileged process using the attachment broker. +// The privileged process dups the HANDLE into its own HANDLE table. This test +// checks that the file has the same contents in the privileged process. +TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandle) { + Init("SendHandle"); + + CommonSetUp(); + ResultListener result_listener; + get_proxy_listener()->set_listener(&result_listener); + + HANDLE h = CreateTempFile(); + SendMessageWithAttachment(h); + base::MessageLoop::current()->Run(); + + // Check the result. + ASSERT_EQ(ProxyListener::MESSAGE_RECEIVED, + get_proxy_listener()->get_reason()); + ASSERT_EQ(result_listener.get_result(), RESULT_SUCCESS); + + CommonTearDown(); +} + +// Similar to SendHandle, except the file HANDLE attached to the message has +// neither read nor write permissions. +TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandleWithoutPermissions) { + Init("SendHandleWithoutPermissions"); + + CommonSetUp(); + ResultListener result_listener; + get_proxy_listener()->set_listener(&result_listener); + + HANDLE h = CreateTempFile(); + HANDLE h2; + BOOL result = ::DuplicateHandle(GetCurrentProcess(), h, GetCurrentProcess(), + &h2, 0, FALSE, DUPLICATE_CLOSE_SOURCE); + ASSERT_TRUE(result); + SendMessageWithAttachment(h2); + base::MessageLoop::current()->Run(); + + // Check the result. + ASSERT_EQ(ProxyListener::MESSAGE_RECEIVED, + get_proxy_listener()->get_reason()); + ASSERT_EQ(result_listener.get_result(), RESULT_SUCCESS); + + CommonTearDown(); +} + +// Similar to SendHandle, except the attachment's destination process is this +// process. This is an unrealistic scenario, but simulates an unprivileged +// process sending an attachment to another unprivileged process. +TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandleToSelf) { + Init("SendHandleToSelf"); + + set_broker(new MockBroker); + CommonSetUp(); + get_proxy_listener()->set_listener(get_broker()); + + HANDLE h = CreateTempFile(); + SendMessageWithAttachment(h); + base::MessageLoop::current()->Run(); + + // Get the received attachment. + IPC::BrokerableAttachment::AttachmentId* id = get_observer()->get_id(); + scoped_refptr<IPC::BrokerableAttachment> received_attachment; + get_broker()->GetAttachmentWithId(*id, &received_attachment); + ASSERT_NE(received_attachment.get(), nullptr); + + // Check that it's a new entry in the HANDLE table. + HANDLE h2 = GetHandleFromBrokeredAttachment(received_attachment); + EXPECT_NE(h2, h); + EXPECT_NE(h2, nullptr); + + // But it still points to the same file. + std::string contents = ReadFromFile(h); + EXPECT_EQ(contents, std::string(kDataBuffer)); + + CommonTearDown(); +} + +using OnMessageReceivedCallback = + void (*)(MockObserver* observer, + IPC::AttachmentBrokerPrivilegedWin* broker, + IPC::Sender* sender); + +int CommonPrivilegedProcessMain(OnMessageReceivedCallback callback, + const char* channel_name) { + base::MessageLoopForIO main_message_loop; + ProxyListener listener; + + // Set up IPC channel. + IPC::AttachmentBrokerPrivilegedWin broker; + listener.set_listener(&broker); + scoped_ptr<IPC::Channel> channel(IPC::Channel::CreateClient( + IPCTestBase::GetChannelName(channel_name), &listener, &broker)); + broker.RegisterCommunicationChannel(channel.get()); + CHECK(channel->Connect()); + + MockObserver observer; + broker.AddObserver(&observer); + + while (true) { + base::MessageLoop::current()->Run(); + ProxyListener::Reason reason = listener.get_reason(); + if (reason == ProxyListener::CHANNEL_ERROR) + break; + + callback(&observer, &broker, channel.get()); + } + + return 0; +} + +void SendHandleCallback(MockObserver* observer, + IPC::AttachmentBrokerPrivilegedWin* broker, + IPC::Sender* sender) { + IPC::BrokerableAttachment::AttachmentId* id = observer->get_id(); + scoped_refptr<IPC::BrokerableAttachment> received_attachment; + broker->GetAttachmentWithId(*id, &received_attachment); + + // Check that it's the expected handle. + bool success = CheckContentsOfBrokeredAttachment(received_attachment); + + SendControlMessage(sender, success); +} + +MULTIPROCESS_IPC_TEST_CLIENT_MAIN(SendHandle) { + return CommonPrivilegedProcessMain(&SendHandleCallback, "SendHandle"); +} + +void SendHandleWithoutPermissionsCallback( + MockObserver* observer, + IPC::AttachmentBrokerPrivilegedWin* broker, + IPC::Sender* sender) { + IPC::BrokerableAttachment::AttachmentId* id = observer->get_id(); + scoped_refptr<IPC::BrokerableAttachment> received_attachment; + broker->GetAttachmentWithId(*id, &received_attachment); + + // Check that it's the expected handle. + HANDLE h = GetHandleFromBrokeredAttachment(received_attachment); + if (h != nullptr) { + SetFilePointer(h, 0, nullptr, FILE_BEGIN); + + char buffer[100]; + DWORD bytes_read; + BOOL success = + ::ReadFile(h, buffer, static_cast<DWORD>(strlen(kDataBuffer)), + &bytes_read, nullptr); + if (!success && GetLastError() == ERROR_ACCESS_DENIED) { + SendControlMessage(sender, true); + return; + } + } + + SendControlMessage(sender, false); +} + +MULTIPROCESS_IPC_TEST_CLIENT_MAIN(SendHandleWithoutPermissions) { + return CommonPrivilegedProcessMain(&SendHandleWithoutPermissionsCallback, + "SendHandleWithoutPermissions"); +} + +void SendHandleToSelfCallback(MockObserver* observer, + IPC::AttachmentBrokerPrivilegedWin* broker, + IPC::Sender* sender) { + // Do nothing special. The default behavior already runs the + // AttachmentBrokerPrivilegedWin. +} + +MULTIPROCESS_IPC_TEST_CLIENT_MAIN(SendHandleToSelf) { + return CommonPrivilegedProcessMain(&SendHandleToSelfCallback, + "SendHandleToSelf"); +} + +} // namespace diff --git a/ipc/attachment_broker_win.cc b/ipc/attachment_broker_win.cc index 5d4b74d..a4cb067 100644 --- a/ipc/attachment_broker_win.cc +++ b/ipc/attachment_broker_win.cc @@ -18,12 +18,6 @@ AttachmentBrokerWin::AttachmentBrokerWin() { AttachmentBrokerWin::~AttachmentBrokerWin() { } -void AttachmentBrokerWin::OnReceiveDuplicatedHandle( - HANDLE, - BrokerableAttachment::AttachmentId id) { - // TODO(erikchen): Implement me. http://crbug.com/493414 -} - bool AttachmentBrokerWin::SendAttachmentToProcess( const BrokerableAttachment* attachment, base::ProcessId destination_process) { @@ -33,22 +27,10 @@ bool AttachmentBrokerWin::SendAttachmentToProcess( static_cast<const internal::HandleAttachmentWin*>(attachment); internal::HandleAttachmentWin::WireFormat format = handle_attachment->GetWireFormat(destination_process); - return sender_->Send( - new AttachmentBrokerMsg_DuplicateWinHandle(format)); - } - return false; -} - -bool AttachmentBrokerWin::GetAttachmentWithId( - BrokerableAttachment::AttachmentId id, - scoped_refptr<BrokerableAttachment>* out_attachment) { - for (AttachmentVector::iterator it = attachments_.begin(); - it != attachments_.end(); ++it) { - if ((*it)->GetIdentifier() == id) { - *out_attachment = *it; - attachments_.erase(it); - return true; - } + // TODO(erikchen): Replace the call to base::Process::Current().Pid() with + // a non-forgeable mechanism. http://crbug.com/513431. + return sender_->Send(new AttachmentBrokerMsg_DuplicateWinHandle( + format, base::Process::Current().Pid())); } return false; } @@ -71,8 +53,7 @@ void AttachmentBrokerWin::OnWinHandleHasBeenDuplicated( scoped_refptr<BrokerableAttachment> attachment( new IPC::internal::HandleAttachmentWin(wire_format)); - attachments_.push_back(attachment); - NotifyObservers(attachment->GetIdentifier()); + HandleReceivedAttachment(attachment); } } // namespace IPC diff --git a/ipc/attachment_broker_win.h b/ipc/attachment_broker_win.h index 1a81b09..dc6f8f7 100644 --- a/ipc/attachment_broker_win.h +++ b/ipc/attachment_broker_win.h @@ -7,7 +7,6 @@ #include <vector> -#include "base/gtest_prod_util.h" #include "base/memory/ref_counted.h" #include "ipc/attachment_broker.h" #include "ipc/brokerable_attachment.h" @@ -18,23 +17,16 @@ namespace IPC { class Sender; -// This class is an implementation of AttachmentBroker for the Windows platform. +// This class is an implementation of AttachmentBroker for the Windows platform +// for non-privileged processes. class IPC_EXPORT AttachmentBrokerWin : public IPC::AttachmentBroker { public: AttachmentBrokerWin(); ~AttachmentBrokerWin() override; - // In a non-broker process, the single instance of this class listens for - // an IPC from the broker process indicating that a new attachment has been - // duplicated. - void OnReceiveDuplicatedHandle(HANDLE, BrokerableAttachment::AttachmentId id); - // IPC::AttachmentBroker overrides. bool SendAttachmentToProcess(const BrokerableAttachment* attachment, base::ProcessId destination_process) override; - bool GetAttachmentWithId( - BrokerableAttachment::AttachmentId id, - scoped_refptr<BrokerableAttachment>* attachment) override; // IPC::Listener overrides. bool OnMessageReceived(const Message& message) override; @@ -44,21 +36,10 @@ class IPC_EXPORT AttachmentBrokerWin : public IPC::AttachmentBroker { void set_sender(IPC::Sender* sender) { sender_ = sender; } private: - FRIEND_TEST_ALL_PREFIXES(AttachmentBrokerWinTest, ReceiveValidMessage); - FRIEND_TEST_ALL_PREFIXES(AttachmentBrokerWinTest, ReceiveInvalidMessage); - using AttachmentVector = std::vector<scoped_refptr<BrokerableAttachment>>; - // IPC message handlers. void OnWinHandleHasBeenDuplicated( const IPC::internal::HandleAttachmentWin::WireFormat& wire_format); - // A vector of BrokerableAttachments that have been received, but not yet - // consumed. - // A std::vector is used instead of a std::map because this container is - // expected to have few elements, for which a std::vector is expected to have - // better performance. - AttachmentVector attachments_; - // |sender_| is used to send Messages to the broker. |sender_| must live at // least as long as this instance. IPC::Sender* sender_; diff --git a/ipc/brokerable_attachment.cc b/ipc/brokerable_attachment.cc index 9af40d9..f1cc9b2 100644 --- a/ipc/brokerable_attachment.cc +++ b/ipc/brokerable_attachment.cc @@ -22,13 +22,11 @@ BrokerableAttachment::AttachmentId GetRandomId() { } // namespace BrokerableAttachment::BrokerableAttachment() - : id_(GetRandomId()), needs_brokering_(false) { -} + : id_(GetRandomId()), needs_brokering_(false) {} BrokerableAttachment::BrokerableAttachment(const AttachmentId& id, bool needs_brokering) - : id_(id), needs_brokering_(needs_brokering) { -} + : id_(id), needs_brokering_(needs_brokering) {} BrokerableAttachment::~BrokerableAttachment() { } diff --git a/ipc/handle_attachment_win.cc b/ipc/handle_attachment_win.cc index fefaff6..994b22d 100644 --- a/ipc/handle_attachment_win.cc +++ b/ipc/handle_attachment_win.cc @@ -15,13 +15,11 @@ HandleAttachmentWin::HandleAttachmentWin(const HANDLE& handle) HandleAttachmentWin::HandleAttachmentWin(const WireFormat& wire_format) : BrokerableAttachment(wire_format.attachment_id, false), - handle_(LongToHandle(wire_format.handle)) { -} + handle_(LongToHandle(wire_format.handle)) {} HandleAttachmentWin::HandleAttachmentWin( const BrokerableAttachment::AttachmentId& id) - : BrokerableAttachment(id, true), handle_(INVALID_HANDLE_VALUE) { -} + : BrokerableAttachment(id, true), handle_(INVALID_HANDLE_VALUE) {} HandleAttachmentWin::~HandleAttachmentWin() { } diff --git a/ipc/handle_attachment_win.h b/ipc/handle_attachment_win.h index d7fe678..e164cc0 100644 --- a/ipc/handle_attachment_win.h +++ b/ipc/handle_attachment_win.h @@ -25,6 +25,7 @@ class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment { // void*, whose size varies between 32 and 64-bit processes. Using a // int32_t means that 64-bit processes will need to perform both up-casting // and down-casting. This is performed using the appropriate Windows apis. + // A value of 0 is equivalent to an invalid handle. int32_t handle; // The id of the destination process that the handle is duplicated into. base::ProcessId destination_process; diff --git a/ipc/ipc.gyp b/ipc/ipc.gyp index f8a34ad..c2f8745 100644 --- a/ipc/ipc.gyp +++ b/ipc/ipc.gyp @@ -45,6 +45,7 @@ '..' ], 'sources': [ + 'attachment_broker_privileged_win_unittest.cc', 'attachment_broker_win_unittest.cc', 'ipc_channel_posix_unittest.cc', 'ipc_channel_proxy_unittest.cc', diff --git a/ipc/ipc.gypi b/ipc/ipc.gypi index f332166..2cda257 100644 --- a/ipc/ipc.gypi +++ b/ipc/ipc.gypi @@ -14,6 +14,8 @@ 'attachment_broker.cc', 'attachment_broker.h', 'attachment_broker_messages.h', + 'attachment_broker_privileged_win.cc', + 'attachment_broker_privileged_win.h', 'attachment_broker_win.cc', 'attachment_broker_win.h', 'brokerable_attachment.cc', diff --git a/ipc/ipc_channel_reader_unittest.cc b/ipc/ipc_channel_reader_unittest.cc index 22300b5..e696809 100644 --- a/ipc/ipc_channel_reader_unittest.cc +++ b/ipc/ipc_channel_reader_unittest.cc @@ -54,29 +54,10 @@ class MockAttachmentBroker : public AttachmentBroker { bool OnMessageReceived(const Message& message) override { return false; } - bool GetAttachmentWithId( - BrokerableAttachment::AttachmentId id, - scoped_refptr<BrokerableAttachment>* attachment) override { - for (AttachmentSet::iterator it = attachments_.begin(); - it != attachments_.end(); ++it) { - if ((*it)->GetIdentifier() == id) { - *attachment = *it; - attachments_.erase(it); - return true; - } - } - return false; - } - void AddAttachment(scoped_refptr<BrokerableAttachment> attachment) { - attachments_.insert(attachment); + get_attachments()->push_back(attachment); NotifyObservers(attachment->GetIdentifier()); } - - private: - // A set of attachmetns that have been brokered and are available to - // consumers. - AttachmentSet attachments_; }; class MockChannelReader : public ChannelReader { diff --git a/ipc/ipc_channel_win.cc b/ipc/ipc_channel_win.cc index 0a905a8..a01a272 100644 --- a/ipc/ipc_channel_win.cc +++ b/ipc/ipc_channel_win.cc @@ -81,23 +81,38 @@ void ChannelWin::Close() { } bool ChannelWin::Send(Message* message) { - // TODO(erikchen): Remove this DCHECK once ChannelWin fully supports - // brokerable attachments. http://crbug.com/493414. - DCHECK(!message->HasAttachments()); DCHECK(thread_check_->CalledOnValidThread()); DVLOG(2) << "sending message @" << message << " on channel @" << this << " with type " << message->type() << " (" << output_queue_.size() << " in queue)"; + if (!prelim_queue_.empty()) { + prelim_queue_.push(message); + return true; + } + + if (message->HasBrokerableAttachments() && + peer_pid_ == base::kNullProcessId) { + prelim_queue_.push(message); + return true; + } + + return ProcessMessageForDelivery(message); +} + +bool ChannelWin::ProcessMessageForDelivery(Message* message) { // Sending a brokerable attachment requires a call to Channel::Send(), so - // Send() may be re-entrant. Brokered attachments must be sent before the - // Message itself. + // both Send() and ProcessMessageForDelivery() may be re-entrant. Brokered + // attachments must be sent before the Message itself. if (message->HasBrokerableAttachments()) { DCHECK(broker_); + DCHECK(peer_pid_ != base::kNullProcessId); for (const BrokerableAttachment* attachment : message->attachment_set()->PeekBrokerableAttachments()) { - if (!broker_->SendAttachmentToProcess(attachment, peer_pid_)) + if (!broker_->SendAttachmentToProcess(attachment, peer_pid_)) { + delete message; return false; + } } } @@ -106,6 +121,8 @@ bool ChannelWin::Send(Message* message) { #endif message->TraceMessageBegin(); + + // |output_queue_| takes ownership of |message|. output_queue_.push(message); // ensure waiting to write if (!waiting_connect_) { @@ -118,6 +135,21 @@ bool ChannelWin::Send(Message* message) { return true; } +void ChannelWin::FlushPrelimQueue() { + DCHECK_NE(peer_pid_, base::kNullProcessId); + + // Due to the possibly re-entrant nature of ProcessMessageForDelivery(), it + // is critical that |prelim_queue_| appears empty. + std::queue<Message*> prelim_queue; + prelim_queue_.swap(prelim_queue); + + while (!prelim_queue.empty()) { + Message* m = prelim_queue.front(); + ProcessMessageForDelivery(m); + prelim_queue.pop(); + } +} + AttachmentBroker* ChannelWin::GetAttachmentBroker() { return broker_; } @@ -205,6 +237,9 @@ void ChannelWin::HandleInternalMessage(const Message& msg) { peer_pid_ = claimed_pid; // Validation completed. validate_client_ = false; + + FlushPrelimQueue(); + listener()->OnChannelConnected(claimed_pid); } diff --git a/ipc/ipc_channel_win.h b/ipc/ipc_channel_win.h index ed1b0ea..aa1d1f00 100644 --- a/ipc/ipc_channel_win.h +++ b/ipc/ipc_channel_win.h @@ -61,6 +61,17 @@ class ChannelWin : public Channel, bool ProcessOutgoingMessages(base::MessageLoopForIO::IOContext* context, DWORD bytes_written); + // Returns |false| on channel error. + // If |message| has brokerable attachments, those attachments are passed to + // the AttachmentBroker (which in turn invokes Send()), so this method must + // be re-entrant. + // Adds |message| to |output_queue_| and calls ProcessOutgoingMessages(). + bool ProcessMessageForDelivery(Message* message); + + // Moves all messages from |prelim_queue_| to |output_queue_| by calling + // ProcessMessageForDelivery(). + void FlushPrelimQueue(); + // MessageLoop::IOHandler implementation. void OnIOCompleted(base::MessageLoopForIO::IOContext* context, DWORD bytes_transfered, @@ -81,6 +92,16 @@ class ChannelWin : public Channel, base::ProcessId peer_pid_; + // Messages not yet ready to be sent are queued here. Messages removed from + // this queue are placed in the output_queue_. The double queue is + // unfortunate, but is necessary because messages with brokerable attachments + // can generate multiple messages to be sent (possibly from other channels). + // Some of these generated messages cannot be sent until |peer_pid_| has been + // configured. + // As soon as |peer_pid| has been configured, there is no longer any need for + // |prelim_queue_|. All messages are flushed, and no new messages are added. + std::queue<Message*> prelim_queue_; + // Messages to be sent are queued here. std::queue<Message*> output_queue_; diff --git a/ipc/ipc_test_base.cc b/ipc/ipc_test_base.cc index 4de5543..53db30e 100644 --- a/ipc/ipc_test_base.cc +++ b/ipc/ipc_test_base.cc @@ -161,5 +161,5 @@ scoped_ptr<IPC::ChannelFactory> IPCTestBase::CreateChannelFactory( const IPC::ChannelHandle& handle, base::SequencedTaskRunner* runner) { return IPC::ChannelFactory::Create(handle, IPC::Channel::MODE_SERVER, - nullptr); + attachment_broker_); } diff --git a/ipc/ipc_test_base.h b/ipc/ipc_test_base.h index 2a45f72..03ac32f 100644 --- a/ipc/ipc_test_base.h +++ b/ipc/ipc_test_base.h @@ -20,6 +20,10 @@ namespace base { class MessageLoop; } +namespace IPC { +class AttachmentBroker; +} + // A test fixture for multiprocess IPC tests. Such tests include a "client" side // (running in a separate process). The same client may be shared between // several different tests. @@ -99,6 +103,9 @@ class IPCTestBase : public base::MultiProcessTest { IPC::Channel* channel() { return channel_.get(); } IPC::ChannelProxy* channel_proxy() { return channel_proxy_.get(); } + void set_attachment_broker(IPC::AttachmentBroker* broker) { + attachment_broker_ = broker; + } const base::Process& client_process() const { return client_process_; } scoped_refptr<base::SequencedTaskRunner> task_runner(); @@ -117,6 +124,10 @@ class IPCTestBase : public base::MultiProcessTest { scoped_ptr<IPC::Channel> channel_; scoped_ptr<IPC::ChannelProxy> channel_proxy_; + // The AttachmentBroker that should be passed to |channel_| when it is + // created. + IPC::AttachmentBroker* attachment_broker_; + base::Process client_process_; DISALLOW_COPY_AND_ASSIGN(IPCTestBase); |