From be49d8a2a9fe44dd2c85b38c2411b694fcf75223 Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Fri, 16 Nov 2018 22:47:31 -0800 Subject: [PATCH 1/6] Add mDNS services for pairing and connect - ADB Secure Pairing - ADB Secure Connect Nothing else is implemented. Bug: 111434128, 119490749 Test: N/A Exempt-From-Owner-Approval: already approved Change-Id: I2e7873b62a3c7631451e47f6a301f8c4a5ffa2e2 --- adb/adb_mdns.h | 12 ++++++++++++ adb/daemon/mdns.cpp | 33 +++++++++++++++++++++------------ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/adb/adb_mdns.h b/adb/adb_mdns.h index 2e544d7fe..33e2e0884 100644 --- a/adb/adb_mdns.h +++ b/adb/adb_mdns.h @@ -17,6 +17,18 @@ #ifndef _ADB_MDNS_H_ #define _ADB_MDNS_H_ +#include + const char* kADBServiceType = "_adb._tcp"; +const char* kADBSecurePairingServiceType = "_adb_secure_pairing._tcp"; +const char* kADBSecureConnectServiceType = "_adb_secure_connect._tcp"; + +const char* kADBDNSServices[] = { + kADBServiceType, + kADBSecurePairingServiceType, + kADBSecureConnectServiceType, +}; + +const int kNumADBDNSServices = arraysize(kADBDNSServices); #endif diff --git a/adb/daemon/mdns.cpp b/adb/daemon/mdns.cpp index 3530f485e..c8faab42f 100644 --- a/adb/daemon/mdns.cpp +++ b/adb/daemon/mdns.cpp @@ -32,8 +32,8 @@ using namespace std::chrono_literals; static std::mutex& mdns_lock = *new std::mutex(); static int port; -static DNSServiceRef mdns_ref; -static bool mdns_registered = false; +static DNSServiceRef mdns_refs[kNumADBDNSServices]; +static bool mdns_registered[kNumADBDNSServices]; static void start_mdns() { if (android::base::GetProperty("init.svc.mdnsd", "") == "running") { @@ -67,24 +67,33 @@ static void setup_mdns_thread() { std::string hostname = "adb-"; hostname += android::base::GetProperty("ro.serialno", "unidentified"); - auto error = DNSServiceRegister(&mdns_ref, 0, 0, hostname.c_str(), - kADBServiceType, nullptr, nullptr, - htobe16((uint16_t)port), 0, nullptr, - mdns_callback, nullptr); + for (int i = 0; i < kNumADBDNSServices; i++) { + auto error = DNSServiceRegister(&mdns_refs[i], 0, 0, hostname.c_str(), kADBDNSServices[i], + nullptr, nullptr, htobe16((uint16_t)port), 0, nullptr, + mdns_callback, nullptr); - if (error != kDNSServiceErr_NoError) { - LOG(ERROR) << "Could not register mDNS service (" << error << ")."; - return; + if (error != kDNSServiceErr_NoError) { + LOG(ERROR) << "Could not register mDNS service " << kADBDNSServices[i] << ", error (" + << error << ")."; + mdns_registered[i] = false; + } + + mdns_registered[i] = true; } - mdns_registered = true; + for (int i = 0; i < kNumADBDNSServices; i++) { + LOG(INFO) << "adbd mDNS service " << kADBDNSServices[i] + << " registered: " << mdns_registered[i]; + } } static void teardown_mdns() { std::lock_guard lock(mdns_lock); - if (mdns_registered) { - DNSServiceRefDeallocate(mdns_ref); + for (int i = 0; i < kNumADBDNSServices; ++i) { + if (mdns_registered[i]) { + DNSServiceRefDeallocate(mdns_refs[i]); + } } } From c712f2db76b3de9fe2524492e6ff8f6d90bcd6bb Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Sat, 17 Nov 2018 10:14:29 -0800 Subject: [PATCH 2/6] make the client browse for appropriate mdns services This CL makes it so the client looks for the adb secure pairing and adb secure connect services. Nothing else should happen, but this should be useful to see if the right packet traffic for discoverability is happening. Bug: 111434128, 119490749 Test: N/A Exempt-From-Owner-Approval: already approved Change-Id: I266bdb8526cf39bbfa131344dca2b1bb14c14a7b --- adb/adb_mdns.h | 1 + adb/client/transport_mdns.cpp | 40 ++++++++++++++++++----------------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/adb/adb_mdns.h b/adb/adb_mdns.h index 33e2e0884..e223b1b2b 100644 --- a/adb/adb_mdns.h +++ b/adb/adb_mdns.h @@ -30,5 +30,6 @@ const char* kADBDNSServices[] = { }; const int kNumADBDNSServices = arraysize(kADBDNSServices); +const int kADBTransportServiceRefIndex = 0; #endif diff --git a/adb/client/transport_mdns.cpp b/adb/client/transport_mdns.cpp index 1a34384d2..20fa09efa 100644 --- a/adb/client/transport_mdns.cpp +++ b/adb/client/transport_mdns.cpp @@ -34,7 +34,7 @@ #include "fdevent/fdevent.h" #include "sysdeps.h" -static DNSServiceRef service_ref; +static DNSServiceRef service_refs[kNumADBDNSServices]; static fdevent* service_ref_fde; // Use adb_DNSServiceRefSockFD() instead of calling DNSServiceRefSockFD() @@ -239,14 +239,10 @@ static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, } } -static void DNSSD_API register_mdns_transport(DNSServiceRef sdRef, - DNSServiceFlags flags, - uint32_t interfaceIndex, - DNSServiceErrorType errorCode, - const char* serviceName, - const char* regtype, - const char* domain, - void* /*context*/) { +static void DNSSD_API on_service_browsed(DNSServiceRef sdRef, DNSServiceFlags flags, + uint32_t interfaceIndex, DNSServiceErrorType errorCode, + const char* serviceName, const char* regtype, + const char* domain, void* /*context*/) { D("Registering a transport."); if (errorCode != kDNSServiceErr_NoError) { D("Got error %d during mDNS browse.", errorCode); @@ -262,19 +258,25 @@ static void DNSSD_API register_mdns_transport(DNSServiceRef sdRef, } void init_mdns_transport_discovery_thread(void) { - DNSServiceErrorType errorCode = DNSServiceBrowse(&service_ref, 0, 0, kADBServiceType, nullptr, - register_mdns_transport, nullptr); + int errorCodes[kNumADBDNSServices]; - if (errorCode != kDNSServiceErr_NoError) { - D("Got %d initiating mDNS browse.", errorCode); - return; + for (int i = 0; i < kNumADBDNSServices; ++i) { + errorCodes[i] = DNSServiceBrowse(&service_refs[i], 0, 0, kADBDNSServices[i], nullptr, + on_service_browsed, nullptr); + + if (errorCodes[i] != kDNSServiceErr_NoError) { + D("Got %d browsing for mDNS service %s.", errorCodes[i], kADBDNSServices[i]); + } } - fdevent_run_on_main_thread([]() { - service_ref_fde = - fdevent_create(adb_DNSServiceRefSockFD(service_ref), pump_service_ref, &service_ref); - fdevent_set(service_ref_fde, FDE_READ); - }); + if (errorCodes[kADBTransportServiceRefIndex] == kDNSServiceErr_NoError) { + fdevent_run_on_main_thread([]() { + service_ref_fde = fdevent_create( + adb_DNSServiceRefSockFD(service_refs[kADBTransportServiceRefIndex]), + pump_service_ref, &service_refs[kADBTransportServiceRefIndex]); + fdevent_set(service_ref_fde, FDE_READ); + }); + } } void init_mdns_transport_discovery(void) { From 4b62bcde047ede5c928e7dacd790c8f9691d0072 Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Mon, 26 Nov 2018 14:19:55 -0800 Subject: [PATCH 3/6] get the ip address info for all DNS services This CL adds functionality to the class ResolvedService where the ip address associated with a resolved DNS service is recorded. It also avoids connecting to the device unless it is the Things-related DNS service. Next step is to add some kind of interface in other parts of adb code to retrieve these IP addresses. Bug: 111434128, 119490749 Test: N/A Exempt-From-Owner-Approval: already approved Change-Id: I46895a7a5bf5660f524c7323a9454f1e2c7d385c --- adb/client/transport_mdns.cpp | 118 ++++++++++++++++++++++------------ 1 file changed, 76 insertions(+), 42 deletions(-) diff --git a/adb/client/transport_mdns.cpp b/adb/client/transport_mdns.cpp index 20fa09efa..ebef711bd 100644 --- a/adb/client/transport_mdns.cpp +++ b/adb/client/transport_mdns.cpp @@ -35,7 +35,21 @@ #include "sysdeps.h" static DNSServiceRef service_refs[kNumADBDNSServices]; -static fdevent* service_ref_fde; +static fdevent* service_ref_fdes[kNumADBDNSServices]; + +static int adb_DNSServiceIndexByName(const char* regType) { + for (int i = 0; i < kNumADBDNSServices; ++i) { + if (!strncmp(regType, kADBDNSServices[i], strlen(kADBDNSServices[i]))) { + return i; + } + } + return -1; +} + +static bool adb_DNSServiceShouldConnect(const char* regType) { + int index = adb_DNSServiceIndexByName(regType); + return index == kADBTransportServiceRefIndex; +} // Use adb_DNSServiceRefSockFD() instead of calling DNSServiceRefSockFD() // directly so that the socket is put through the appropriate compatibility @@ -94,10 +108,15 @@ class ResolvedService : public AsyncServiceRef { public: virtual ~ResolvedService() = default; - ResolvedService(std::string name, uint32_t interfaceIndex, - const char* hosttarget, uint16_t port) : - name_(name), - port_(port) { + ResolvedService(std::string serviceName, std::string regType, uint32_t interfaceIndex, + const char* hosttarget, uint16_t port) + : serviceName_(serviceName), + regType_(regType), + hosttarget_(hosttarget), + port_(port), + sa_family_(0), + ip_addr_data_(NULL) { + memset(ip_addr_, 0, sizeof(ip_addr_)); /* TODO: We should be able to get IPv6 support by adding * kDNSServiceProtocol_IPv6 to the flags below. However, when we do @@ -119,40 +138,47 @@ class ResolvedService : public AsyncServiceRef { } void Connect(const sockaddr* address) { - char ip_addr[INET6_ADDRSTRLEN]; - const void* ip_addr_data; + sa_family_ = address->sa_family; const char* addr_format; - if (address->sa_family == AF_INET) { - ip_addr_data = - &reinterpret_cast(address)->sin_addr; + if (sa_family_ == AF_INET) { + ip_addr_data_ = &reinterpret_cast(address)->sin_addr; addr_format = "%s:%hu"; - } else if (address->sa_family == AF_INET6) { - ip_addr_data = - &reinterpret_cast(address)->sin6_addr; + } else if (sa_family_ == AF_INET6) { + ip_addr_data_ = &reinterpret_cast(address)->sin6_addr; addr_format = "[%s]:%hu"; - } else { // Should be impossible + } else { // Should be impossible D("mDNS resolved non-IP address."); return; } // Winsock version requires the const cast Because Microsoft. - if (!inet_ntop(address->sa_family, const_cast(ip_addr_data), - ip_addr, INET6_ADDRSTRLEN)) { + if (!inet_ntop(sa_family_, const_cast(ip_addr_data_), ip_addr_, sizeof(ip_addr_))) { D("Could not convert IP address to string."); return; } - std::string response; - connect_device(android::base::StringPrintf(addr_format, ip_addr, port_), - &response); - D("Connect to %s (%s:%hu) : %s", name_.c_str(), ip_addr, port_, - response.c_str()); + // adb secure service needs to do something different from just + // connecting here. + if (adb_DNSServiceShouldConnect(regType_.c_str())) { + std::string response; + connect_device(android::base::StringPrintf(addr_format, ip_addr_, port_), &response); + D("Connect to %s (%s:%hu) : %s", serviceName_.c_str(), ip_addr_, port_, + response.c_str()); + } else { + D("Not immediately connecting to serviceName=[%s], regtype=[%s] ipaddr=(%s:%hu)", + serviceName_.c_str(), regType_.c_str(), ip_addr_, port_); + } } private: - std::string name_; + std::string serviceName_; + std::string regType_; + std::string hosttarget_; const uint16_t port_; + int sa_family_; + const void* ip_addr_data_; + char ip_addr_[INET6_ADDRSTRLEN]; }; static void DNSSD_API register_service_ip(DNSServiceRef /*sdRef*/, @@ -182,18 +208,23 @@ static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, class DiscoveredService : public AsyncServiceRef { public: - DiscoveredService(uint32_t interfaceIndex, const char* serviceName, - const char* regtype, const char* domain) - : serviceName_(serviceName) { - + DiscoveredService(uint32_t interfaceIndex, const char* serviceName, const char* regtype, + const char* domain) + : serviceName_(serviceName), regType_(regtype) { DNSServiceErrorType ret = DNSServiceResolve(&sdRef_, 0, interfaceIndex, serviceName, regtype, domain, register_resolved_mdns_service, reinterpret_cast(this)); - if (ret != kDNSServiceErr_NoError) { - D("Got %d from DNSServiceResolve.", ret); - } else { + D("DNSServiceResolve for " + "interfaceIndex %u " + "serviceName %s " + "regtype %s " + "domain %s " + ": %d", + interfaceIndex, serviceName, regtype, domain, ret); + + if (ret == kDNSServiceErr_NoError) { Initialize(); } } @@ -202,8 +233,11 @@ class DiscoveredService : public AsyncServiceRef { return serviceName_.c_str(); } + const char* RegType() { return regType_.c_str(); } + private: std::string serviceName_; + std::string regType_; }; static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, @@ -225,10 +259,8 @@ static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, return; } - - auto resolved = - new ResolvedService(discovered->ServiceName(), - interfaceIndex, hosttarget, ntohs(port)); + auto resolved = new ResolvedService(discovered->ServiceName(), discovered->RegType(), + interfaceIndex, hosttarget, ntohs(port)); if (! resolved->Initialized()) { delete resolved; @@ -247,7 +279,10 @@ static void DNSSD_API on_service_browsed(DNSServiceRef sdRef, DNSServiceFlags fl if (errorCode != kDNSServiceErr_NoError) { D("Got error %d during mDNS browse.", errorCode); DNSServiceRefDeallocate(sdRef); - fdevent_destroy(service_ref_fde); + int serviceIndex = adb_DNSServiceIndexByName(regtype); + if (serviceIndex != -1) { + fdevent_destroy(service_ref_fdes[serviceIndex]); + } return; } @@ -267,15 +302,14 @@ void init_mdns_transport_discovery_thread(void) { if (errorCodes[i] != kDNSServiceErr_NoError) { D("Got %d browsing for mDNS service %s.", errorCodes[i], kADBDNSServices[i]); } - } - if (errorCodes[kADBTransportServiceRefIndex] == kDNSServiceErr_NoError) { - fdevent_run_on_main_thread([]() { - service_ref_fde = fdevent_create( - adb_DNSServiceRefSockFD(service_refs[kADBTransportServiceRefIndex]), - pump_service_ref, &service_refs[kADBTransportServiceRefIndex]); - fdevent_set(service_ref_fde, FDE_READ); - }); + if (errorCodes[i] == kDNSServiceErr_NoError) { + fdevent_run_on_main_thread([i]() { + service_ref_fdes[i] = fdevent_create(adb_DNSServiceRefSockFD(service_refs[i]), + pump_service_ref, &service_refs[i]); + fdevent_set(service_ref_fdes[i], FDE_READ); + }); + } } } From cef4ade8e1af48fbf3aac42bf4484bcc96f96b38 Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Mon, 26 Nov 2018 17:31:39 -0800 Subject: [PATCH 4/6] adb client interface for secure services This CL exposes a callback-based interface to perform some action for each resolved pairing/connect service as a function of their hostname, ip address, and port. The ADB client can then use this information to either set up secure connections directly, or to tell the adb host server to do so, or something. Bug: 111434128, 119490749 Test: N/A Exempt-From-Owner-Approval: already approved Change-Id: I2dd04c322df5b0597859f44703cfd2f3f29fd737 --- adb/adb_mdns.h | 5 +- adb/client/adb_client.h | 14 ++++++ adb/client/transport_mdns.cpp | 92 ++++++++++++++++++++++++++++++++++- 3 files changed, 108 insertions(+), 3 deletions(-) diff --git a/adb/adb_mdns.h b/adb/adb_mdns.h index e223b1b2b..cc2258146 100644 --- a/adb/adb_mdns.h +++ b/adb/adb_mdns.h @@ -23,6 +23,10 @@ const char* kADBServiceType = "_adb._tcp"; const char* kADBSecurePairingServiceType = "_adb_secure_pairing._tcp"; const char* kADBSecureConnectServiceType = "_adb_secure_connect._tcp"; +const int kADBTransportServiceRefIndex = 0; +const int kADBSecurePairingServiceRefIndex = 1; +const int kADBSecureConnectServiceRefIndex = 2; + const char* kADBDNSServices[] = { kADBServiceType, kADBSecurePairingServiceType, @@ -30,6 +34,5 @@ const char* kADBDNSServices[] = { }; const int kNumADBDNSServices = arraysize(kADBDNSServices); -const int kADBTransportServiceRefIndex = 0; #endif diff --git a/adb/client/adb_client.h b/adb/client/adb_client.h index ba530418d..758fcab42 100644 --- a/adb/client/adb_client.h +++ b/adb/client/adb_client.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -86,3 +87,16 @@ std::optional adb_get_server_executable_path(); // Globally acccesible argv/envp, for the purpose of re-execing adb. extern const char* _Nullable * _Nullable __adb_argv; extern const char* _Nullable * _Nullable __adb_envp; + +// ADB Secure DNS service interface. Used to query what ADB Secure DNS services have been +// resolved, and to run some kind of callback for each one. +using adb_secure_foreach_service_callback = std::function; + +// Queries pairing/connect services that have been discovered and resolved. +// If |host_name| is not null, run |cb| only for services +// matching |host_name|. Otherwise, run for all services. +void adb_secure_foreach_pairing_service(const char* _Nullable host_name, + adb_secure_foreach_service_callback cb); +void adb_secure_foreach_connect_service(const char* _Nullable host_name, + adb_secure_foreach_service_callback cb); diff --git a/adb/client/transport_mdns.cpp b/adb/client/transport_mdns.cpp index ebef711bd..0f88f786b 100644 --- a/adb/client/transport_mdns.cpp +++ b/adb/client/transport_mdns.cpp @@ -25,10 +25,12 @@ #endif #include +#include #include #include +#include "adb_client.h" #include "adb_mdns.h" #include "adb_trace.h" #include "fdevent/fdevent.h" @@ -163,14 +165,44 @@ class ResolvedService : public AsyncServiceRef { if (adb_DNSServiceShouldConnect(regType_.c_str())) { std::string response; connect_device(android::base::StringPrintf(addr_format, ip_addr_, port_), &response); - D("Connect to %s (%s:%hu) : %s", serviceName_.c_str(), ip_addr_, port_, - response.c_str()); + D("Connect to %s regtype %s (%s:%hu) : %s", serviceName_.c_str(), regType_.c_str(), + ip_addr_, port_, response.c_str()); } else { D("Not immediately connecting to serviceName=[%s], regtype=[%s] ipaddr=(%s:%hu)", serviceName_.c_str(), regType_.c_str(), ip_addr_, port_); } + + int adbSecureServiceType = serviceIndex(); + switch (adbSecureServiceType) { + case kADBSecurePairingServiceRefIndex: + sAdbSecurePairingServices->push_back(this); + break; + case kADBSecureConnectServiceRefIndex: + sAdbSecureConnectServices->push_back(this); + break; + default: + break; + } } + int serviceIndex() const { return adb_DNSServiceIndexByName(regType_.c_str()); } + + std::string hostTarget() const { return hosttarget_; } + + std::string ipAddress() const { return ip_addr_; } + + uint16_t port() const { return port_; } + + using ServiceRegistry = std::vector; + + static ServiceRegistry* sAdbSecurePairingServices; + static ServiceRegistry* sAdbSecureConnectServices; + + static void initAdbSecure(); + + static void forEachService(const ServiceRegistry& services, const std::string& hostname, + adb_secure_foreach_service_callback cb); + private: std::string serviceName_; std::string regType_; @@ -181,6 +213,55 @@ class ResolvedService : public AsyncServiceRef { char ip_addr_[INET6_ADDRSTRLEN]; }; +// static +std::vector* ResolvedService::sAdbSecurePairingServices = NULL; + +// static +std::vector* ResolvedService::sAdbSecureConnectServices = NULL; + +// static +void ResolvedService::initAdbSecure() { + if (!sAdbSecurePairingServices) { + sAdbSecurePairingServices = new ServiceRegistry; + } + if (!sAdbSecureConnectServices) { + sAdbSecureConnectServices = new ServiceRegistry; + } +} + +// static +void ResolvedService::forEachService(const ServiceRegistry& services, + const std::string& wanted_host, + adb_secure_foreach_service_callback cb) { + initAdbSecure(); + + for (auto service : services) { + auto hostname = service->hostTarget(); + auto ip = service->ipAddress(); + auto port = service->port(); + + if (wanted_host == "") { + cb(hostname.c_str(), ip.c_str(), port); + } else if (hostname == wanted_host) { + cb(hostname.c_str(), ip.c_str(), port); + } + } +} + +// static +void adb_secure_foreach_pairing_service(const char* host_name, + adb_secure_foreach_service_callback cb) { + ResolvedService::forEachService(*ResolvedService::sAdbSecurePairingServices, + host_name ? host_name : "", cb); +} + +// static +void adb_secure_foreach_connect_service(const char* host_name, + adb_secure_foreach_service_callback cb) { + ResolvedService::forEachService(*ResolvedService::sAdbSecureConnectServices, + host_name ? host_name : "", cb); +} + static void DNSSD_API register_service_ip(DNSServiceRef /*sdRef*/, DNSServiceFlags /*flags*/, uint32_t /*interfaceIndex*/, @@ -193,6 +274,12 @@ static void DNSSD_API register_service_ip(DNSServiceRef /*sdRef*/, std::unique_ptr data( reinterpret_cast(context)); data->Connect(address); + + // For ADB Secure services, keep those ResolvedService's around + // for later processing with secure connection establishment. + if (data->serviceIndex() != kADBTransportServiceRefIndex) { + data.release(); + } } static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, @@ -314,5 +401,6 @@ void init_mdns_transport_discovery_thread(void) { } void init_mdns_transport_discovery(void) { + ResolvedService::initAdbSecure(); std::thread(init_mdns_transport_discovery_thread).detach(); } From 615036f49445840ec1bf047688210936f3634b64 Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Wed, 28 Nov 2018 16:38:36 -0800 Subject: [PATCH 5/6] adbd: only register dnsservices via explicit API For privacy/security reasons, let's not broadcast the adb secure services on startup automatically, and instead leave that up to the rest of the adbd code somehow. Instead, this CL adds an API in daemon/mdns.h that lets the user control when registration happens, potentially only doing so if the developer option is enabled or something. Bug: 111434128, 119490749 Test: N/A Exempt-From-Owner-Approval: already approved Change-Id: Idc994a59ef9e5d48f08796c21989883497e19ef8 --- adb/daemon/mdns.cpp | 90 ++++++++++++++++++++++++++++++++++----------- adb/daemon/mdns.h | 8 ++++ 2 files changed, 76 insertions(+), 22 deletions(-) diff --git a/adb/daemon/mdns.cpp b/adb/daemon/mdns.cpp index c8faab42f..35e3d907e 100644 --- a/adb/daemon/mdns.cpp +++ b/adb/daemon/mdns.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include "mdns.h" #include "adb_mdns.h" #include "sysdeps.h" @@ -60,43 +61,58 @@ static void mdns_callback(DNSServiceRef /*ref*/, } } -static void setup_mdns_thread() { - start_mdns(); +static void register_mdns_service(int index, int port) { std::lock_guard lock(mdns_lock); std::string hostname = "adb-"; hostname += android::base::GetProperty("ro.serialno", "unidentified"); - for (int i = 0; i < kNumADBDNSServices; i++) { - auto error = DNSServiceRegister(&mdns_refs[i], 0, 0, hostname.c_str(), kADBDNSServices[i], - nullptr, nullptr, htobe16((uint16_t)port), 0, nullptr, - mdns_callback, nullptr); + auto error = DNSServiceRegister(&mdns_refs[index], 0, 0, hostname.c_str(), + kADBDNSServices[index], nullptr, nullptr, + htobe16((uint16_t)port), 0, nullptr, mdns_callback, nullptr); - if (error != kDNSServiceErr_NoError) { - LOG(ERROR) << "Could not register mDNS service " << kADBDNSServices[i] << ", error (" - << error << ")."; - mdns_registered[i] = false; - } - - mdns_registered[i] = true; + if (error != kDNSServiceErr_NoError) { + LOG(ERROR) << "Could not register mDNS service " << kADBDNSServices[index] << ", error (" + << error << ")."; + mdns_registered[index] = false; } - for (int i = 0; i < kNumADBDNSServices; i++) { - LOG(INFO) << "adbd mDNS service " << kADBDNSServices[i] - << " registered: " << mdns_registered[i]; - } + mdns_registered[index] = true; + + LOG(INFO) << "adbd mDNS service " << kADBDNSServices[index] + << " registered: " << mdns_registered[index]; } -static void teardown_mdns() { +static void unregister_mdns_service(int index) { std::lock_guard lock(mdns_lock); - for (int i = 0; i < kNumADBDNSServices; ++i) { - if (mdns_registered[i]) { - DNSServiceRefDeallocate(mdns_refs[i]); - } + if (mdns_registered[index]) { + DNSServiceRefDeallocate(mdns_refs[index]); } } +static void register_base_mdns_transport() { + register_mdns_service(kADBTransportServiceRefIndex, port); +} + +static void setup_mdns_thread() { + start_mdns(); + + // We will now only set up the normal transport mDNS service + // instead of registering all the adb secure mDNS services + // in the beginning. This is to provide more privacy/security. + register_base_mdns_transport(); +} + +// This also tears down any adb secure mDNS services, if they exist. +static void teardown_mdns() { + for (int i = 0; i < kNumADBDNSServices; ++i) { + unregister_mdns_service(i); + } +} + +// Public interface///////////////////////////////////////////////////////////// + void setup_mdns(int port_in) { port = port_in; std::thread(setup_mdns_thread).detach(); @@ -104,3 +120,33 @@ void setup_mdns(int port_in) { // TODO: Make this more robust against a hard kill. atexit(teardown_mdns); } + +void register_adb_secure_pairing_service(int port) { + std::thread([port]() { + register_mdns_service(kADBSecurePairingServiceRefIndex, port); + }).detach(); +} + +void unregister_adb_secure_pairing_service() { + std::thread([]() { unregister_mdns_service(kADBSecurePairingServiceRefIndex); }).detach(); +} + +bool is_adb_secure_pairing_service_registered() { + std::lock_guard lock(mdns_lock); + return mdns_registered[kADBSecurePairingServiceRefIndex]; +} + +void register_adb_secure_connect_service(int port) { + std::thread([port]() { + register_mdns_service(kADBSecureConnectServiceRefIndex, port); + }).detach(); +} + +void unregister_adb_secure_connect_service() { + std::thread([]() { unregister_mdns_service(kADBSecureConnectServiceRefIndex); }).detach(); +} + +bool is_adb_secure_connect_service_registered() { + std::lock_guard lock(mdns_lock); + return mdns_registered[kADBSecureConnectServiceRefIndex]; +} diff --git a/adb/daemon/mdns.h b/adb/daemon/mdns.h index 4c6b1ca02..a18093b50 100644 --- a/adb/daemon/mdns.h +++ b/adb/daemon/mdns.h @@ -19,4 +19,12 @@ void setup_mdns(int port); +void register_adb_secure_pairing_service(int port); +void unregister_adb_secure_pairing_service(int port); +bool is_adb_secure_pairing_service_registered(); + +void register_adb_secure_connect_service(int port); +void unregister_adb_secure_connect_service(int port); +bool is_adb_secure_connect_service_registered(); + #endif // _DAEMON_MDNS_H_ From 39e54b8c5b79b67def0401bc2da344bee207ca3e Mon Sep 17 00:00:00 2001 From: Lingfeng Yang Date: Thu, 24 Oct 2019 22:30:11 -0700 Subject: [PATCH 6/6] add a version TXT record to adb secure mdns services In the context of secure connect, allows adbd and host adb to reject each other based on incompatible versions without even having to actually connect (since it is a DNS TXT). Bug: 111434128, 119490749 Test: N/A Exempt-From-Owner-Approval: already approved Change-Id: I54312d8b67370c397ba81ecdbca1b27e3ee58572 --- adb/adb_mdns.h | 50 +++++++++++++++++++++++ adb/client/transport_mdns.cpp | 76 +++++++++++++++++++++++++++++------ adb/daemon/mdns.cpp | 34 ++++++++++++++-- 3 files changed, 144 insertions(+), 16 deletions(-) diff --git a/adb/adb_mdns.h b/adb/adb_mdns.h index cc2258146..6b373553a 100644 --- a/adb/adb_mdns.h +++ b/adb/adb_mdns.h @@ -27,12 +27,62 @@ const int kADBTransportServiceRefIndex = 0; const int kADBSecurePairingServiceRefIndex = 1; const int kADBSecureConnectServiceRefIndex = 2; +// Each ADB Secure service advertises with a TXT record indicating the version +// using a key/value pair per RFC 6763 (https://tools.ietf.org/html/rfc6763). +// +// The first key/value pair is always the version of the protocol. +// There may be more key/value pairs added after. +// +// The version is purposely represented as the single letter "v" due to the +// need to minimize DNS traffic. The version starts at 1. With each breaking +// protocol change, the version is incremented by 1. +// +// Newer adb clients/daemons need to recognize and either reject +// or be backward-compatible with older verseions if there is a mismatch. +// +// Relevant sections: +// +// """ +// 6.4. Rules for Keys in DNS-SD Key/Value Pairs +// +// The key MUST be at least one character. DNS-SD TXT record strings +// beginning with an '=' character (i.e., the key is missing) MUST be +// silently ignored. +// +// ... +// +// 6.5. Rules for Values in DNS-SD Key/Value Pairs +// +// If there is an '=' in a DNS-SD TXT record string, then everything +// after the first '=' to the end of the string is the value. The value +// can contain any eight-bit values including '='. +// """ + +#define ADB_SECURE_SERVICE_VERSION_TXT_RECORD(ver) ("v=" #ver) + +// Client/service versions are initially defined to be matching, +// but may go out of sync as different clients and services +// try to talk to each other. +#define ADB_SECURE_SERVICE_VERSION 1 +#define ADB_SECURE_CLIENT_VERSION ADB_SECURE_SERVICE_VERSION + +const char* kADBSecurePairingServiceTxtRecord = + ADB_SECURE_SERVICE_VERSION_TXT_RECORD(ADB_SECURE_SERVICE_VERSION); +const char* kADBSecureConnectServiceTxtRecord = + ADB_SECURE_SERVICE_VERSION_TXT_RECORD(ADB_SECURE_SERVICE_VERSION); + const char* kADBDNSServices[] = { kADBServiceType, kADBSecurePairingServiceType, kADBSecureConnectServiceType, }; +const char* kADBDNSServiceTxtRecords[] = { + nullptr, + kADBSecurePairingServiceTxtRecord, + kADBSecureConnectServiceTxtRecord, +}; + const int kNumADBDNSServices = arraysize(kADBDNSServices); #endif diff --git a/adb/client/transport_mdns.cpp b/adb/client/transport_mdns.cpp index 0f88f786b..f5811a4ca 100644 --- a/adb/client/transport_mdns.cpp +++ b/adb/client/transport_mdns.cpp @@ -111,13 +111,14 @@ class ResolvedService : public AsyncServiceRef { virtual ~ResolvedService() = default; ResolvedService(std::string serviceName, std::string regType, uint32_t interfaceIndex, - const char* hosttarget, uint16_t port) + const char* hosttarget, uint16_t port, int version) : serviceName_(serviceName), regType_(regType), hosttarget_(hosttarget), port_(port), sa_family_(0), - ip_addr_data_(NULL) { + ip_addr_data_(NULL), + serviceVersion_(version) { memset(ip_addr_, 0, sizeof(ip_addr_)); /* TODO: We should be able to get IPv6 support by adding @@ -137,6 +138,8 @@ class ResolvedService : public AsyncServiceRef { } else { Initialize(); } + + D("Client version: %d Service version: %d\n", clientVersion_, serviceVersion_); } void Connect(const sockaddr* address) { @@ -204,6 +207,7 @@ class ResolvedService : public AsyncServiceRef { adb_secure_foreach_service_callback cb); private: + int clientVersion_ = ADB_SECURE_CLIENT_VERSION; std::string serviceName_; std::string regType_; std::string hosttarget_; @@ -211,6 +215,7 @@ class ResolvedService : public AsyncServiceRef { int sa_family_; const void* ip_addr_data_; char ip_addr_[INET6_ADDRSTRLEN]; + int serviceVersion_; }; // static @@ -327,16 +332,55 @@ class DiscoveredService : public AsyncServiceRef { std::string regType_; }; -static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, - DNSServiceFlags flags, - uint32_t interfaceIndex, - DNSServiceErrorType errorCode, - const char* fullname, - const char* hosttarget, - uint16_t port, - uint16_t /*txtLen*/, - const unsigned char* /*txtRecord*/, - void* context) { +// Returns the version the device wanted to advertise, +// or -1 if parsing fails. +static int parse_version_from_txt_record(uint16_t txtLen, const unsigned char* txtRecord) { + if (!txtLen) return -1; + if (!txtRecord) return -1; + + // https://tools.ietf.org/html/rfc6763 + // """ + // 6.1. General Format Rules for DNS TXT Records + // + // A DNS TXT record can be up to 65535 (0xFFFF) bytes long. The total + // length is indicated by the length given in the resource record header + // in the DNS message. There is no way to tell directly from the data + // alone how long it is (e.g., there is no length count at the start, or + // terminating NULL byte at the end). + // """ + + // Let's trust the TXT record's length byte + // Worst case, it wastes 255 bytes + std::vector recordAsString(txtLen + 1, '\0'); + char* str = recordAsString.data(); + + memcpy(str, txtRecord + 1 /* skip the length byte */, txtLen); + + // Check if it's the version key + static const char* versionKey = "v="; + size_t versionKeyLen = strlen(versionKey); + + if (strncmp(versionKey, str, versionKeyLen)) return -1; + + auto valueStart = str + versionKeyLen; + + long parsedNumber = strtol(valueStart, 0, 10); + + // No valid conversion. Also, 0 + // is not a valid version. + if (!parsedNumber) return -1; + + // Outside bounds of long. + if (parsedNumber == LONG_MIN || parsedNumber == LONG_MAX) return -1; + + // Possibly valid version + return static_cast(parsedNumber); +} + +static void DNSSD_API register_resolved_mdns_service( + DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t interfaceIndex, + DNSServiceErrorType errorCode, const char* fullname, const char* hosttarget, uint16_t port, + uint16_t txtLen, const unsigned char* txtRecord, void* context) { D("Resolved a service."); std::unique_ptr discovered( reinterpret_cast(context)); @@ -346,8 +390,14 @@ static void DNSSD_API register_resolved_mdns_service(DNSServiceRef sdRef, return; } + // TODO: Reject certain combinations of invalid or mismatched client and + // service versions here before creating anything. + // At the moment, there is nothing to reject, so accept everything + // as an optimistic default. + auto serviceVersion = parse_version_from_txt_record(txtLen, txtRecord); + auto resolved = new ResolvedService(discovered->ServiceName(), discovered->RegType(), - interfaceIndex, hosttarget, ntohs(port)); + interfaceIndex, hosttarget, ntohs(port), serviceVersion); if (! resolved->Initialized()) { delete resolved; diff --git a/adb/daemon/mdns.cpp b/adb/daemon/mdns.cpp index 35e3d907e..fa98340b5 100644 --- a/adb/daemon/mdns.cpp +++ b/adb/daemon/mdns.cpp @@ -67,9 +67,37 @@ static void register_mdns_service(int index, int port) { std::string hostname = "adb-"; hostname += android::base::GetProperty("ro.serialno", "unidentified"); - auto error = DNSServiceRegister(&mdns_refs[index], 0, 0, hostname.c_str(), - kADBDNSServices[index], nullptr, nullptr, - htobe16((uint16_t)port), 0, nullptr, mdns_callback, nullptr); + // https://tools.ietf.org/html/rfc6763 + // """ + // The format of the data within a DNS TXT record is one or more + // strings, packed together in memory without any intervening gaps or + // padding bytes for word alignment. + // + // The format of each constituent string within the DNS TXT record is a + // single length byte, followed by 0-255 bytes of text data. + // """ + // + // Therefore: + // 1. Begin with the string length + // 2. No null termination + + std::vector txtRecord; + + if (kADBDNSServiceTxtRecords[index]) { + size_t txtRecordStringLength = strlen(kADBDNSServiceTxtRecords[index]); + + txtRecord.resize(1 + // length byte + txtRecordStringLength // string bytes + ); + + txtRecord[0] = (char)txtRecordStringLength; + memcpy(txtRecord.data() + 1, kADBDNSServiceTxtRecords[index], txtRecordStringLength); + } + + auto error = DNSServiceRegister( + &mdns_refs[index], 0, 0, hostname.c_str(), kADBDNSServices[index], nullptr, nullptr, + htobe16((uint16_t)port), (uint16_t)txtRecord.size(), + txtRecord.empty() ? nullptr : txtRecord.data(), mdns_callback, nullptr); if (error != kDNSServiceErr_NoError) { LOG(ERROR) << "Could not register mDNS service " << kADBDNSServices[index] << ", error ("