Skip to content

Commit

Permalink
Implement secure copying for the exo clipboard
Browse files Browse the repository at this point in the history
Currently, backgrounded crostini apps can copy to the clipboard. This
is a security concern as malicious apps are able to copy to the
clipboard which is likely to then be pasted into an address bar. Now,
we only allow crostini apps to make the copy if there is a foregrounded
crostini app with the same client.

Since data source objects cannot be mapped into surfaces, the most we
can do is check the request client and the foreground surfaces are the
same.

paste integration tests)

Bug: chromium:970114
Test: crostini.SecureCopyPaste.* and crostini.CopyPaste.* (all copy
Change-Id: I8a04993f4c697083c6bdaa5eae64c75925239f56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1962142
Commit-Queue: Matthew Chen <matterchen@google.com>
Reviewed-by: Fergus Dall <sidereal@google.com>
Reviewed-by: Daichi Hirono <hirono@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Nic Hollingum <hollingum@google.com>
Cr-Commit-Position: refs/heads/master@{#724994}
  • Loading branch information
matterchen authored and Commit Bot committed Dec 16, 2019
1 parent 374ca28 commit 4343147
Show file tree
Hide file tree
Showing 12 changed files with 43 additions and 8 deletions.
2 changes: 1 addition & 1 deletion components/exo/data_device_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class DataDeviceDelegate {

// This should return true if |surface| is a valid target for this data
// device. E.g. the surface is owned by the same client as the data device.
virtual bool CanAcceptDataEventsForSurface(Surface* surface) = 0;
virtual bool CanAcceptDataEventsForSurface(Surface* surface) const = 0;

protected:
virtual ~DataDeviceDelegate() {}
Expand Down
2 changes: 1 addition & 1 deletion components/exo/data_device_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class TestDataDeviceDelegate : public DataDeviceDelegate {
void OnSelection(const DataOffer& data_offer) override {
events_.push_back(DataEvent::kSelection);
}
bool CanAcceptDataEventsForSurface(Surface* surface) override {
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return can_accept_data_events_for_surface_;
}

Expand Down
4 changes: 4 additions & 0 deletions components/exo/data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -302,4 +302,8 @@ void DataSource::OnTextRead(ReadTextDataCallback callback,
std::move(callback).Run(mime_type, std::move(output));
}

bool DataSource::CanBeDataSourceForCopy(Surface *surface) const {
return delegate_->CanAcceptDataEventsForSurface(surface);
}

} // namespace exo
3 changes: 3 additions & 0 deletions components/exo/data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/containers/flat_set.h"
#include "base/macros.h"
#include "base/observer_list.h"
#include "components/exo/surface.h"

namespace exo {

Expand Down Expand Up @@ -78,6 +79,8 @@ class DataSource {
void ReadDataForTesting(const std::string& mime_type,
ReadDataCallback callback);

bool CanBeDataSourceForCopy(Surface *surface) const;

private:
// Reads data from the source. Then |callback| is invoked with read data. If
// Cancelled() is invoked or DataSource is destroyed before completion,
Expand Down
4 changes: 4 additions & 0 deletions components/exo/data_source_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ class DataSourceDelegate {
// Called when the compositor selects one drag and drop action.
virtual void OnAction(DndAction dnd_action) = 0;

// This should return true if |surface| is the source of this data source.
// E.g. the surface is owned by the same client as the data source.
virtual bool CanAcceptDataEventsForSurface(Surface* surface) const = 0;

protected:
virtual ~DataSourceDelegate() {}
};
Expand Down
3 changes: 3 additions & 0 deletions components/exo/data_source_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ class TestDataSourceDelegate : public DataSourceDelegate {
void OnDndDropPerformed() override {}
void OnDndFinished() override {}
void OnAction(DndAction dnd_action) override {}
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return true;
}
};

void CheckMimeType(const std::string& expected,
Expand Down
2 changes: 1 addition & 1 deletion components/exo/display_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class TestDataDeviceDelegate : public DataDeviceDelegate {
const gfx::PointF& location) override {}
void OnDrop() override {}
void OnSelection(const DataOffer& data_offer) override {}
bool CanAcceptDataEventsForSurface(Surface* surface) override {
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return false;
}
};
Expand Down
3 changes: 3 additions & 0 deletions components/exo/pointer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ class TestDataSourceDelegate : public DataSourceDelegate {
void OnDndDropPerformed() override {}
void OnDndFinished() override {}
void OnAction(DndAction dnd_action) override {}
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return true;
}

DISALLOW_COPY_AND_ASSIGN(TestDataSourceDelegate);
};
Expand Down
5 changes: 5 additions & 0 deletions components/exo/seat.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ void Seat::AbortPendingDragOperation() {
}

void Seat::SetSelection(DataSource* source) {
Surface* focused_surface = GetFocusedSurface();
if (source && !source->CanBeDataSourceForCopy(focused_surface)) {
return;
}

if (!source) {
ui::Clipboard::GetForCurrentThread()->Clear(
ui::ClipboardBuffer::kCopyPaste);
Expand Down
3 changes: 3 additions & 0 deletions components/exo/seat_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ class TestDataSourceDelegate : public DataSourceDelegate {
void OnDndDropPerformed() override {}
void OnDndFinished() override {}
void OnAction(DndAction dnd_action) override {}
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return true;
}

void SetData(std::vector<uint8_t> data) { data_ = std::move(data); }

Expand Down
3 changes: 3 additions & 0 deletions components/exo/touch_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class TestDataSourceDelegate : public DataSourceDelegate {
void OnDndDropPerformed() override {}
void OnDndFinished() override {}
void OnAction(DndAction dnd_action) override {}
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return true;
}

DISALLOW_COPY_AND_ASSIGN(TestDataSourceDelegate);
};
Expand Down
17 changes: 12 additions & 5 deletions components/exo/wayland/wl_data_device_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,17 @@ base::flat_set<DndAction> DataDeviceManagerDndActions(uint32_t value) {

class WaylandDataSourceDelegate : public DataSourceDelegate {
public:
explicit WaylandDataSourceDelegate(wl_resource* source)
: data_source_resource_(source) {}
explicit WaylandDataSourceDelegate(wl_client* client,
wl_resource* source)
: client_(client),
data_source_resource_(source) {}

// Overridden from DataSourceDelegate:
void OnDataSourceDestroying(DataSource* device) override { delete this; }
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return surface &&
wl_resource_get_client(GetSurfaceResource(surface)) == client_;
}
void OnTarget(const base::Optional<std::string>& mime_type) override {
wl_data_source_send_target(data_source_resource_,
mime_type ? mime_type->c_str() : nullptr);
Expand Down Expand Up @@ -122,6 +128,7 @@ class WaylandDataSourceDelegate : public DataSourceDelegate {
}

private:
wl_client* const client_;
wl_resource* const data_source_resource_;

DISALLOW_COPY_AND_ASSIGN(WaylandDataSourceDelegate);
Expand Down Expand Up @@ -240,7 +247,7 @@ class WaylandDataDeviceDelegate : public DataDeviceDelegate {

// Overridden from DataDeviceDelegate:
void OnDataDeviceDestroying(DataDevice* device) override { delete this; }
bool CanAcceptDataEventsForSurface(Surface* surface) override {
bool CanAcceptDataEventsForSurface(Surface* surface) const override {
return surface &&
wl_resource_get_client(GetSurfaceResource(surface)) == client_;
}
Expand Down Expand Up @@ -362,8 +369,8 @@ void data_device_manager_create_data_source(wl_client* client,
wl_resource* data_source_resource = wl_resource_create(
client, &wl_data_source_interface, wl_resource_get_version(resource), id);
SetImplementation(data_source_resource, &data_source_implementation,
std::make_unique<DataSource>(
new WaylandDataSourceDelegate(data_source_resource)));
std::make_unique<DataSource>(new WaylandDataSourceDelegate(
client, data_source_resource)));
}

void data_device_manager_get_data_device(wl_client* client,
Expand Down

0 comments on commit 4343147

Please sign in to comment.