diff options
author | erikchen <erikchen@chromium.org> | 2015-07-27 13:28:20 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-07-27 20:29:06 +0000 |
commit | c04ab34c84e272a6bd7234f14bcf9950a346c580 (patch) | |
tree | c860b9dee6015d90bed92cfd0943b1cf23d68cae /ipc | |
parent | ba53d6dc1d33f3be64defce3e72cf6c0c5cd855a (diff) | |
download | chromium_src-c04ab34c84e272a6bd7234f14bcf9950a346c580.zip chromium_src-c04ab34c84e272a6bd7234f14bcf9950a346c580.tar.gz chromium_src-c04ab34c84e272a6bd7234f14bcf9950a346c580.tar.bz2 |
ipc: Add attachment broker code for the privileged browser process.
No intended behavior change.
This CL adds the class AttachmentBrokerPrivilegedWin, a subclass of
AttachmentBroker intended for use in the privileged browser process on the
Windows platform. No brokerable attachments are made outside of tests, so this
code is not yet active.
This CL consists of several changes:
- The class AttachmentBrokerPrivilegedWin was created.
- Common logic between AttachmentBrokerPrivilegedWin and AttachmentBrokerWin
was moved to AttachmentBroker.
- ChannelWin was given a new member prelim_queue_. This queue is normally
empty, but in some circumstances messages are queued here before being
processed for delivery. See the documentation for a full explanation.
BUG=466437
Review URL: https://codereview.chromium.org/1246103006
Cr-Commit-Position: refs/heads/master@{#340548}
Diffstat (limited to 'ipc')
-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); |