From: Shaju Mathew Date: Sat, 25 Jun 2022 14:57:31 +0000 (+0000) Subject: Reject external connect: requests. X-Git-Tag: archive/raspbian/29.0.6-25+rpi1^2~1 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=e562b7cbbf645c1b19b82bfd9ccb2cfeed77ace1;p=android-platform-tools.git 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 Change-Id: I0ec7d6f8e60afc2ee5d1be2b63bf90ca99443a52 Gbp-Pq: Topic cve Gbp-Pq: Name CVE-2022-3168.patch --- diff --git a/system/core/adb/adb.cpp b/system/core/adb/adb.cpp index a2a766b6..044571b4 100644 --- a/system/core/adb/adb.cpp +++ b/system/core/adb/adb.cpp @@ -369,7 +369,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::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/system/core/adb/sockets.cpp b/system/core/adb/sockets.cpp index 423af67f..c1ba0d47 100644 --- a/system/core/adb/sockets.cpp +++ b/system/core/adb/sockets.cpp @@ -501,6 +501,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/system/core/adb/transport.cpp b/system/core/adb/transport.cpp index 9dd6ec64..4d05f0c8 100644 --- a/system/core/adb/transport.cpp +++ b/system/core/adb/transport.cpp @@ -1341,6 +1341,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: localhost: : responds with the +// device_port +// - adb reverse --remove tcp: : 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:]; + 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: + 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/system/core/adb/transport.h b/system/core/adb/transport.h index 5a750eea..a16c38a5 100644 --- a/system/core/adb/transport.h +++ b/system/core/adb/transport.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -267,6 +268,10 @@ class atransport : public enable_weak_from_this { 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); + const TransportId id; bool online = false; @@ -372,6 +377,13 @@ class atransport : public enable_weak_from_this { std::mutex mutex_; +#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 reverse_forwards_; +#endif + DISALLOW_COPY_AND_ASSIGN(atransport); };