socksproxy: Fix localhost DNS and use-after-free crash (#10332)

* Produce a sorted list by metric * Tweak proxy DNS selection to suport link-local nameservers * Fix race condition when connection is destroyed before DNS finishes --------- Co-authored-by: Sebastian Streich <[email protected]>
mozilla-mobile · Mar 11, 2025 · d57e84b · d57e84b
1 parent e8c3554
commit d57e84b
Show file tree

Hide file tree

Showing 4 changed files with 109 additions and 72 deletions.
diff --git a/extension/socks5proxy/bin/windowsbypass.cpp b/extension/socks5proxy/bin/windowsbypass.cpp
@@ -220,34 +220,6 @@ void WindowsBypass::refreshAddresses() {
     }
   }
 
-  // Fetch the DNS resolvers too.
-  QByteArray gaaBuffer(4096, 0);
-  ULONG gaaBufferSize = gaaBuffer.size();
-  auto adapterAddrs = reinterpret_cast<PIP_ADAPTER_ADDRESSES>(gaaBuffer.data());
-  result = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX, nullptr,
-                                adapterAddrs, &gaaBufferSize);
-  if (result == ERROR_BUFFER_OVERFLOW) {
-    gaaBuffer.resize(gaaBufferSize);
-    adapterAddrs = reinterpret_cast<PIP_ADAPTER_ADDRESSES>(gaaBuffer.data());
-    result = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX, nullptr,
-                                  adapterAddrs, &gaaBufferSize);
-  }
-  for (auto i = adapterAddrs; result == NO_ERROR && i != nullptr; i = i->Next) {
-    // Ignore DNS servers on interfaces without routable addresses.
-    if (!data.contains(i->Luid.Value)) {
-      continue;
-    }
-    if (i->FirstDnsServerAddress == nullptr) {
-      continue;
-    }
-
-    // FIXME: Only using the first DNS server for now.
-    struct sockaddr* sa = i->FirstDnsServerAddress->Address.lpSockaddr;
-    data[i->Luid.Value].dnsAddr.setAddress(sa);
-    qDebug() << "Using" << data[i->Luid.Value].dnsAddr.toString()
-             << "for DNS server";
-  }
-
   // Swap the updated table into use.
   m_interfaceData.swap(data);
   updateNameserver();
@@ -284,30 +256,56 @@ void WindowsBypass::interfaceChanged(quint64 luid) {
 }
 
 void WindowsBypass::updateNameserver() {
-  // Update the preferred DNS server.
-  QHostAddress dnsNameserver;
-  ULONG dnsMetric = ULONG_MAX;
-  for (auto i = m_interfaceData.constBegin(); i != m_interfaceData.constEnd();
-       i++) {
-    auto data = i.value();
-    if (data.dnsAddr.isNull()) {
+  // A list of DNS servers and their interface metrics.
+  struct keyed {
+    QHostAddress addr;
+    ulong metric;
+  };
+  QList<keyed> nameservers;
+
+  // Fetch the DNS resolvers.
+  QByteArray gaaBuffer(4096, 0);
+  ULONG gaaBufferSize = gaaBuffer.size();
+  auto adapterAddrs = reinterpret_cast<PIP_ADAPTER_ADDRESSES>(gaaBuffer.data());
+  DWORD result = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
+                                      nullptr, adapterAddrs, &gaaBufferSize);
+  if (result == ERROR_BUFFER_OVERFLOW) {
+    gaaBuffer.resize(gaaBufferSize);
+    adapterAddrs = reinterpret_cast<PIP_ADAPTER_ADDRESSES>(gaaBuffer.data());
+    result = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX, nullptr,
+                                  adapterAddrs, &gaaBufferSize);
+  }
+  for (auto i = adapterAddrs; result == NO_ERROR && i != nullptr; i = i->Next) {
+    // Ignore DNS servers on interfaces without routable addresses.
+    if (!m_interfaceData.contains(i->Luid.Value)) {
       continue;
     }
-    if (data.dnsAddr.protocol() == QAbstractSocket::IPv6Protocol) {
-      if (data.ipv6metric >= dnsMetric) {
-        continue;
-      }
-      dnsMetric = data.ipv6metric;
-    } else {
-      if (data.ipv4metric >= dnsMetric) {
-        continue;
+
+    for (auto dns = i->FirstDnsServerAddress; dns != nullptr; dns = dns->Next) {
+      struct sockaddr* sa = dns->Address.lpSockaddr;
+      QHostAddress addr{sa};
+      if (sa->sa_family == AF_INET) {
+        nameservers.append({addr, m_interfaceData[i->Luid.Value].ipv4metric});
+      } else {
+        if (addr.isLinkLocal()) {
+          addr.setScopeId(QString::number(i->Ipv6IfIndex));
+        }
+        nameservers.append({addr, m_interfaceData[i->Luid.Value].ipv6metric});
       }
-      dnsMetric = data.ipv4metric;
+      qDebug() << "Adding" << addr.toString() << "as DNS server for "
+               << i->Luid.Value;
     }
-    dnsNameserver = data.dnsAddr;
   }
-  qDebug() << "Setting nameserver:" << dnsNameserver.toString();
-  DNSResolver::instance()->setNameserver(dnsNameserver);
+
+  // Sort the nameservers by it's metric smallest first
+  std::sort(nameservers.begin(), nameservers.end(),
+            [](auto a, auto b) { return a.metric < b.metric; });
+  QList<QHostAddress> selectedNameServers(nameservers.length());
+  for (const auto& ns : qAsConst(nameservers)) {
+    selectedNameServers.append(ns.addr);
+  }
+
+  DNSResolver::instance()->setNameserver(selectedNameServers);
 }
 
 // In this function, we basically try our best to re-implement the Windows

diff --git a/extension/socks5proxy/bin/windowsbypass.h b/extension/socks5proxy/bin/windowsbypass.h
@@ -45,7 +45,6 @@ class WindowsBypass final : public QObject {
     unsigned long ipv6metric;
     QHostAddress ipv4addr;
     QHostAddress ipv6addr;
-    QHostAddress dnsAddr;
   };
 
   QHash<quint64, InterfaceData> m_interfaceData;

diff --git a/extension/socks5proxy/src/dnsresolver.cpp b/extension/socks5proxy/src/dnsresolver.cpp
@@ -4,6 +4,7 @@
 #include <ares.h>
 
 #include <QCoreApplication>
+#include <QMutexLocker>
 #include <QObject>
 
 #include "socks5connection.h"
@@ -33,10 +34,15 @@ DNSResolver::DNSResolver() {
 DNSResolver::~DNSResolver() { ares_destroy(mChannel); }
 
 /* Callback that is called when DNS query is finished */
-void DNSResolver::addressInfoCallback(void* arg, int status, int timeouts,
+void DNSResolver::addressInfoCallback(QObject* ctx, int status, int timeouts,
                                       struct ares_addrinfo* result) {
+  QMutexLocker locker(&m_requestLock);
+  if (!m_requests.contains(ctx)) {
+    qDebug() << "Connection destroyed before resolution completed";
+    return;
+  }
+
   // This should be our Socks5Connection
-  auto ctx = static_cast<QObject*>(arg);
   switch (status) {
     case ARES_ENOTIMP:
       qDebug() << "The ares library does not know how to find addresses of "
@@ -45,7 +51,7 @@ void DNSResolver::addressInfoCallback(void* arg, int status, int timeouts,
                                 Qt::QueuedConnection);
       return;
     case ARES_ENOTFOUND:
-      qDebug() << " The name was not found.";
+      qDebug() << "The name was not found.";
       QMetaObject::invokeMethod(ctx, "onHostnameNotFound",
                                 Qt::QueuedConnection);
       return;
@@ -78,23 +84,49 @@ void DNSResolver::addressInfoCallback(void* arg, int status, int timeouts,
   }
 }
 
-void DNSResolver::resolveAsync(const QString& hostname,
-                               Socks5Connection* parent) {
-  if (!m_nameserver.isNull()) {
-    auto serverString = m_nameserver.toString().toLocal8Bit();
-    int result = ares_set_servers_csv(mChannel, serverString.constData());
-    Q_ASSERT(result == ARES_SUCCESS);
-  }
+void DNSResolver::requestDestroyed() {
+  QMutexLocker locker(&m_requestLock);
+  m_requests.remove(QObject::sender());
+}
+
+void DNSResolver::resolveAsync(const QString& hostname, QObject* parent) {
+  // Store the requesting connections in a hash map.
+  // This allows us to detect when connections are destroyed, and prevents
+  // use-after-free bugs if the resolution completes after the connection
+  // is freed.
+  QObject::connect(parent, &QObject::destroyed, this,
+                   &DNSResolver::requestDestroyed);
+  m_requestLock.lock();
+  m_requests.insert(parent, hostname);
+  m_requestLock.unlock();
+
+  auto callback = [](void* arg, int status, int timeouts,
+                     struct ares_addrinfo* results) {
+    QObject* ctx = static_cast<QObject*>(arg);
+    auto instance = DNSResolver::instance();
+    instance->addressInfoCallback(ctx, status, timeouts, results);
+  };
 
   auto name = hostname.toStdString();
   struct ares_addrinfo_hints hints;
   memset(&hints, 0, sizeof(hints));
   hints.ai_family = AF_UNSPEC;
   hints.ai_flags = ARES_AI_CANONNAME;
-  ares_getaddrinfo(mChannel, name.c_str(), NULL, &hints, &addressInfoCallback,
-                   parent);
+  ares_getaddrinfo(mChannel, name.c_str(), NULL, &hints, callback, parent);
 }
 
-void DNSResolver::setNameserver(const QHostAddress& addr) {
-  m_nameserver = addr;
+void DNSResolver::setNameserver(const QList<QHostAddress>& dnsList) {
+  QString buffer{};
+
+  foreach (const QHostAddress& dns, qAsConst(dnsList)) {
+    if (dns.isNull()) {
+      continue;
+    }
+    buffer.append(dns.toString());
+    buffer.append(",");
+  }
+  auto serverString = buffer.toLocal8Bit();
+  qDebug() << "Setting nameservers:" << serverString;
+  int result = ares_set_servers_csv(mChannel, serverString.constData());
+  Q_ASSERT(result == ARES_SUCCESS);
 }
diff --git a/extension/socks5proxy/src/dnsresolver.h b/extension/socks5proxy/src/dnsresolver.h
@@ -5,14 +5,18 @@
 #pragma once
 
 #include <QGlobalStatic>
+#include <QHash>
 #include <QHostAddress>
-#include <mutex>
+#include <QMutex>
+#include <QObject>
 
 struct ares_channeldata;
 class ares_addrinfo;
 class Socks5Connection;
 
-class DNSResolver {
+class DNSResolver : public QObject {
+  Q_OBJECT
+
  public:
   DNSResolver();
   ~DNSResolver();
@@ -23,19 +27,23 @@ class DNSResolver {
    * @brief Queues up a DNS Query to get Resolved.
    *
    * @param hostname - The requested Hostname
-   * @param parent - The Socks5Connection to notify. Will call
-   * Socks5Connection::onHostnameResolved(QHostAddress) when done.
+   * @param parent - The QObject to notify. Will call the
+   * onHostnameResolved(QHostAddress) method when done.
    */
-  void resolveAsync(const QString& hostname, Socks5Connection* parent);
+  void resolveAsync(const QString& hostname, QObject* parent);
+
+  void setNameserver(const QList<QHostAddress>& addr);
 
-  void setNameserver(const QHostAddress& addr);
+ private slots:
+  void requestDestroyed();
 
  private:
-  static void addressInfoCallback(void* arg, int status, int timeouts,
-                                  struct ares_addrinfo* result);
+  void addressInfoCallback(QObject* ctx, int status, int timeouts,
+                           struct ares_addrinfo* result);
   void shutdownAres();
 
-  ares_channeldata* mChannel = nullptr;
+  QHash<QObject*, QString> m_requests;
+  QMutex m_requestLock;
 
-  QHostAddress m_nameserver;
+  ares_channeldata* mChannel = nullptr;
 };