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

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 a2a766b692fc2c1d4063a79e19548486bc19b83f..044571b4f9a005b5791b6db141e1c0330bd8afc5 100644 (file)
--- 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:<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/system/core/adb/sockets.cpp b/system/core/adb/sockets.cpp
index 423af67f1c542016e9753669aff829dd0516ccb6..c1ba0d4708dc665abce0c6d5f415ce929102094e 100644 (file)
--- 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 9dd6ec642424aa3798d8a8c0dbb9822525f8dfe1..4d05f0c8751ab47221c4d3a553521e742f324375 100644 (file)
--- 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:<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/system/core/adb/transport.h b/system/core/adb/transport.h
index 5a750eea1a9a267eb32bb075a43fb770d19e5386..a16c38a50058366d3aaa43dbdc0a2a648b2fb3b2 100644 (file)
--- a/system/core/adb/transport.h
+++ b/system/core/adb/transport.h
@@ -30,6 +30,7 @@
 #include <string>
 #include <string_view>
 #include <thread>
+#include <unordered_map>
 #include <unordered_set>
 
 #include <android-base/macros.h>
@@ -267,6 +268,10 @@ class atransport : public enable_weak_from_this<atransport> {
     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<atransport> {
 
     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<std::string, std::string> reverse_forwards_;
+#endif
+
     DISALLOW_COPY_AND_ASSIGN(atransport);
 };