185 lines
6.5 KiB
Diff
185 lines
6.5 KiB
Diff
From 13508c1c97da14a294c04e5097ea81c9ce7edf33 Mon Sep 17 00:00:00 2001
|
|
From: Shaju Mathew <shaju@google.com>
|
|
Date: Sat, 25 Jun 2022 14:57:31 +0000
|
|
Subject: [PATCH] Reject external connect: requests.
|
|
|
|
Steps:
|
|
1. Track forward:reverse config in a data-structure.
|
|
2. connect_to_remote() examines each socket transport and updates
|
|
this data-structure.
|
|
3. handle_packet() takes appropriate action
|
|
(abort) for an unknown connect: request originating from the device.
|
|
|
|
Bug:205286508
|
|
|
|
Test: treehugger
|
|
|
|
Signed-off-by: jmgao <jmgao@fb.com>
|
|
Change-Id: I0ec7d6f8e60afc2ee5d1be2b63bf90ca99443a52
|
|
---
|
|
adb.cpp | 11 +++++++++-
|
|
sockets.cpp | 6 +++++
|
|
transport.cpp | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-
|
|
transport.h | 12 ++++++++++
|
|
4 files changed, 88 insertions(+), 2 deletions(-)
|
|
|
|
diff --git a/adb.cpp b/adb.cpp
|
|
index 5d481295..f397da09 100644
|
|
--- a/adb.cpp
|
|
+++ b/vendor/adb/adb.cpp
|
|
@@ -485,7 +485,16 @@ void handle_packet(apacket *p, atransport *t)
|
|
// byte. The client sent strings with null termination, which post-string_view, start
|
|
// being interpreted as part of the string, unless we explicitly strip them.
|
|
address = StripTrailingNulls(address);
|
|
-
|
|
+#if ADB_HOST
|
|
+ // The incoming address (from the payload) might be some other
|
|
+ // target (e.g tcp:<ip>:8000), however we do not allow *any*
|
|
+ // such requests - namely, those from (a potentially compromised)
|
|
+ // adbd (reverse:forward: source) port transport.
|
|
+ if (!t->IsReverseConfigured(address.data())) {
|
|
+ LOG(FATAL) << __func__ << " disallowed connect to " << address << " from "
|
|
+ << t->serial_name();
|
|
+ }
|
|
+#endif
|
|
asocket* s = create_local_service_socket(address, t);
|
|
if (s == nullptr) {
|
|
send_close(0, p->msg.arg0, t);
|
|
diff --git a/sockets.cpp b/sockets.cpp
|
|
index 61a2d9d9..3cd43f92 100644
|
|
--- a/sockets.cpp
|
|
+++ b/vendor/adb/sockets.cpp
|
|
@@ -560,6 +560,12 @@ asocket* create_remote_socket(unsigned id, atransport* t) {
|
|
}
|
|
|
|
void connect_to_remote(asocket* s, std::string_view destination) {
|
|
+#if ADB_HOST
|
|
+ // Snoop reverse:forward: requests to track them so that an
|
|
+ // appropriate filter (to figure out whether the remote is
|
|
+ // allowed to connect locally) can be applied.
|
|
+ s->transport->UpdateReverseConfig(destination);
|
|
+#endif
|
|
D("Connect_to_remote call RS(%d) fd=%d", s->id, s->fd);
|
|
apacket* p = get_apacket();
|
|
|
|
diff --git a/transport.cpp b/transport.cpp
|
|
index 71771d31..f1cae405 100644
|
|
--- a/transport.cpp
|
|
+++ b/vendor/adb/transport.cpp
|
|
@@ -1219,8 +1219,10 @@ bool atransport::HandleRead(std::unique_ptr<apacket> p) {
|
|
VLOG(TRANSPORT) << dump_packet(serial.c_str(), "from remote", p.get());
|
|
apacket* packet = p.release();
|
|
|
|
- // TODO: Does this need to run on the main thread?
|
|
+ // This needs to run on the main thread since the associated fdevent
|
|
+ // message pump exists in that context.
|
|
fdevent_run_on_main_thread([packet, this]() { handle_packet(packet, this); });
|
|
+
|
|
return true;
|
|
}
|
|
|
|
@@ -1614,6 +1616,63 @@ void unregister_usb_transport(usb_handle* usb) {
|
|
return t->GetUsbHandle() == usb && t->GetConnectionState() == kCsNoPerm;
|
|
});
|
|
}
|
|
+
|
|
+// Track reverse:forward commands, so that info can be used to develop
|
|
+// an 'allow-list':
|
|
+// - adb reverse tcp:<device_port> localhost:<host_port> : responds with the
|
|
+// device_port
|
|
+// - adb reverse --remove tcp:<device_port> : responds OKAY
|
|
+// - adb reverse --remove-all : responds OKAY
|
|
+void atransport::UpdateReverseConfig(std::string_view service_addr) {
|
|
+ check_main_thread();
|
|
+ if (!android::base::ConsumePrefix(&service_addr, "reverse:")) {
|
|
+ return;
|
|
+ }
|
|
+
|
|
+ if (android::base::ConsumePrefix(&service_addr, "forward:")) {
|
|
+ // forward:[norebind:]<remote>;<local>
|
|
+ bool norebind = android::base::ConsumePrefix(&service_addr, "norebind:");
|
|
+ auto it = service_addr.find(';');
|
|
+ if (it == std::string::npos) {
|
|
+ return;
|
|
+ }
|
|
+ std::string remote(service_addr.substr(0, it));
|
|
+
|
|
+ if (norebind && reverse_forwards_.find(remote) != reverse_forwards_.end()) {
|
|
+ // This will fail, don't update the map.
|
|
+ LOG(DEBUG) << "ignoring reverse forward that will fail due to norebind";
|
|
+ return;
|
|
+ }
|
|
+
|
|
+ std::string local(service_addr.substr(it + 1));
|
|
+ reverse_forwards_[remote] = local;
|
|
+ } else if (android::base::ConsumePrefix(&service_addr, "killforward:")) {
|
|
+ // kill-forward:<remote>
|
|
+ auto it = service_addr.find(';');
|
|
+ if (it != std::string::npos) {
|
|
+ return;
|
|
+ }
|
|
+ reverse_forwards_.erase(std::string(service_addr));
|
|
+ } else if (service_addr == "killforward-all") {
|
|
+ reverse_forwards_.clear();
|
|
+ } else if (service_addr == "list-forward") {
|
|
+ LOG(DEBUG) << __func__ << " ignoring --list";
|
|
+ } else { // Anything else we need to know about?
|
|
+ LOG(FATAL) << "unhandled reverse service: " << service_addr;
|
|
+ }
|
|
+}
|
|
+
|
|
+// Is this an authorized :connect request?
|
|
+bool atransport::IsReverseConfigured(const std::string& local_addr) {
|
|
+ check_main_thread();
|
|
+ for (const auto& [remote, local] : reverse_forwards_) {
|
|
+ if (local == local_addr) {
|
|
+ return true;
|
|
+ }
|
|
+ }
|
|
+ return false;
|
|
+}
|
|
+
|
|
#endif
|
|
|
|
bool check_header(apacket* p, atransport* t) {
|
|
diff --git a/transport.h b/transport.h
|
|
index ed2cd81f..fc0e322d 100644
|
|
--- a/transport.h
|
|
+++ b/vendor/adb/transport.h
|
|
@@ -31,6 +31,7 @@
|
|
#include <string>
|
|
#include <string_view>
|
|
#include <thread>
|
|
+#include <unordered_map>
|
|
#include <vector>
|
|
|
|
#include <android-base/macros.h>
|
|
@@ -298,6 +299,10 @@ class atransport : public enable_weak_from_this<atransport> {
|
|
#if ADB_HOST
|
|
void SetUsbHandle(usb_handle* h) { usb_handle_ = h; }
|
|
usb_handle* GetUsbHandle() { return usb_handle_; }
|
|
+
|
|
+ // Interface for management/filter on forward:reverse: configuration.
|
|
+ void UpdateReverseConfig(std::string_view service_addr);
|
|
+ bool IsReverseConfigured(const std::string& local_addr);
|
|
#endif
|
|
|
|
const TransportId id;
|
|
@@ -427,6 +432,13 @@ class atransport : public enable_weak_from_this<atransport> {
|
|
|
|
bool delayed_ack_ = false;
|
|
|
|
+#if ADB_HOST
|
|
+ // Track remote addresses against local addresses (configured)
|
|
+ // through `adb reverse` commands.
|
|
+ // Access constrained to primary thread by virtue of check_main_thread().
|
|
+ std::unordered_map<std::string, std::string> reverse_forwards_;
|
|
+#endif
|
|
+
|
|
DISALLOW_COPY_AND_ASSIGN(atransport);
|
|
};
|
|
|
|
--
|
|
2.38.0
|
|
|