Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 452f032d authored by Ken Chen's avatar Ken Chen
Browse files

Fix use-after-free in DNS64 discovery thread 

DNS64 discovery thread is detached from binder requesting thread. But
the discovery thread references resources not belongs to itself, which
can be destroyed in dnsresolver destruction.

Holds a strong pointer of Dns64Configuration in DNS64 discovery thread
so that the instance of Dns64Configuration will keep until the DNS64
thread is force terminated.

Ignore-AOSP-First: Fix security vulnerability
Bug: 278303745
Test: atest
Merged-In: Id74ea4e6f54a00805d3cc8a9d7e15e58a473b7d3
Change-Id: Id74ea4e6f54a00805d3cc8a9d7e15e58a473b7d3
(cherry picked from commit 25411558)
parent 5ceece50

Android.bp

+1 −0
Original line number Original line Diff line number Diff line
@@ -231,6 +231,7 @@ cc_library { 
        "libstatslog_resolv",

        "libstatslog_resolv",

        "libstatspush_compat",

        "libstatspush_compat",

        "libsysutils",

        "libsysutils",

        "libutils",

        "netd_event_listener_interface-lateststable-ndk",

        "netd_event_listener_interface-lateststable-ndk",

        "server_configurable_flags",

        "server_configurable_flags",

        "stats_proto",

        "stats_proto",

Dns64Configuration.cpp

+10 −7
Original line number Original line Diff line number Diff line
@@ -24,6 +24,7 @@ 
#include <netdutils/DumpWriter.h>

#include <netdutils/DumpWriter.h>

#include <netdutils/InternetAddresses.h>

#include <netdutils/InternetAddresses.h>

#include <netdutils/ThreadUtil.h>

#include <netdutils/ThreadUtil.h>

#include <utils/StrongPointer.h>

#include <thread>

#include <thread>

#include <utility>

#include <utility>
@@ -36,6 +37,7 @@ 




namespace android {

namespace android {





using android::sp;

using netdutils::DumpWriter;

using netdutils::DumpWriter;

using netdutils::IPAddress;

using netdutils::IPAddress;

using netdutils::IPPrefix;

using netdutils::IPPrefix;
@@ -61,8 +63,9 @@ void Dns64Configuration::startPrefixDiscovery(unsigned netId) { 
    // Emplace a copy of |cfg| in the map.

    // Emplace a copy of |cfg| in the map.

    mDns64Configs.emplace(std::make_pair(netId, cfg));

    mDns64Configs.emplace(std::make_pair(netId, cfg));





    const sp<Dns64Configuration> thiz = sp<Dns64Configuration>::fromExisting(this);

    // Note that capturing |cfg| in this lambda creates a copy.

    // Note that capturing |cfg| in this lambda creates a copy.

    std::thread discovery_thread([this, cfg, netId] {

    std::thread discovery_thread([thiz, cfg, netId] {

        setThreadName(fmt::format("Nat64Pfx_{}", netId));

        setThreadName(fmt::format("Nat64Pfx_{}", netId));





        // Make a mutable copy rather than mark the whole lambda mutable.

        // Make a mutable copy rather than mark the whole lambda mutable.
@@ -75,28 +78,28 @@ void Dns64Configuration::startPrefixDiscovery(unsigned netId) { 
                               .build();

                               .build();





        while (true) {

        while (true) {

            if (!this->shouldContinueDiscovery(evalCfg)) break;

            if (!thiz->shouldContinueDiscovery(evalCfg)) break;





            android_net_context netcontext{};

            android_net_context netcontext{};

            mGetNetworkContextCallback(evalCfg.netId, 0, &netcontext);

            thiz->mGetNetworkContextCallback(evalCfg.netId, 0, &netcontext);





            // Prefix discovery must bypass private DNS because in strict mode

            // Prefix discovery must bypass private DNS because in strict mode

            // the server generally won't know the NAT64 prefix.

            // the server generally won't know the NAT64 prefix.

            netcontext.flags |= NET_CONTEXT_FLAG_USE_LOCAL_NAMESERVERS;

            netcontext.flags |= NET_CONTEXT_FLAG_USE_LOCAL_NAMESERVERS;

            if (doRfc7050PrefixDiscovery(netcontext, &evalCfg)) {

            if (doRfc7050PrefixDiscovery(netcontext, &evalCfg)) {

                this->recordDns64Config(evalCfg);

                thiz->recordDns64Config(evalCfg);

                break;

                break;

            }

            }





            if (!this->shouldContinueDiscovery(evalCfg)) break;

            if (!thiz->shouldContinueDiscovery(evalCfg)) break;





            if (!backoff.hasNextTimeout()) break;

            if (!backoff.hasNextTimeout()) break;

            {

            {

                std::unique_lock<std::mutex> cvGuard(mMutex);

                std::unique_lock<std::mutex> cvGuard(thiz->mMutex);

                // TODO: Consider some chrono math, combined with wait_until()

                // TODO: Consider some chrono math, combined with wait_until()

                // perhaps, to prevent early re-resolves from the removal of

                // perhaps, to prevent early re-resolves from the removal of

                // other netids with IPv6-only nameservers.

                // other netids with IPv6-only nameservers.

                mCv.wait_for(cvGuard, backoff.getNextTimeout());

                thiz->mCv.wait_for(cvGuard, backoff.getNextTimeout());

            }

            }

        }

        }

    });

    });

Dns64Configuration.h

+2 −1
Original line number Original line Diff line number Diff line
@@ -26,6 +26,7 @@ 
#include <android-base/thread_annotations.h>

#include <android-base/thread_annotations.h>

#include <netdutils/DumpWriter.h>

#include <netdutils/DumpWriter.h>

#include <netdutils/InternetAddresses.h>

#include <netdutils/InternetAddresses.h>

#include <utils/RefBase.h>





struct android_net_context;

struct android_net_context;
@@ -47,7 +48,7 @@ namespace net { 
 * Thread-safety: All public methods in this class MUST be thread-safe.

 * Thread-safety: All public methods in this class MUST be thread-safe.

 * (In other words: this class handles all its locking privately.)

 * (In other words: this class handles all its locking privately.)

 */

 */

class Dns64Configuration {

class Dns64Configuration : virtual public RefBase {

  public:

  public:

    // Simple data struct for passing back packet NAT64 prefix event information to the

    // Simple data struct for passing back packet NAT64 prefix event information to the

    // Dns64PrefixCallback callback.

    // Dns64PrefixCallback callback.

ResolverController.cpp

+7 −7
Original line number Original line Diff line number Diff line
@@ -155,11 +155,11 @@ int getDnsInfo(unsigned netId, std::vector<std::string>* servers, std::vector<st 
}  // namespace

}  // namespace





ResolverController::ResolverController()

ResolverController::ResolverController()

    : mDns64Configuration(

    : mDns64Configuration(android::sp<Dns64Configuration>::make(

              [](uint32_t netId, uint32_t uid, android_net_context* netcontext) {

              [](uint32_t netId, uint32_t uid, android_net_context* netcontext) {

                  gResNetdCallbacks.get_network_context(netId, uid, netcontext);

                  gResNetdCallbacks.get_network_context(netId, uid, netcontext);

              },

              },

              std::bind(sendNat64PrefixEvent, std::placeholders::_1)) {}

              std::bind(sendNat64PrefixEvent, std::placeholders::_1))) {}





void ResolverController::destroyNetworkCache(unsigned netId) {

void ResolverController::destroyNetworkCache(unsigned netId) {

    LOG(VERBOSE) << __func__ << ": netId = " << netId;

    LOG(VERBOSE) << __func__ << ": netId = " << netId;
@@ -173,7 +173,7 @@ void ResolverController::destroyNetworkCache(unsigned netId) { 
                                     event.network_type(), event.private_dns_modes(), bytesField);

                                     event.network_type(), event.private_dns_modes(), bytesField);





    resolv_delete_cache_for_net(netId);

    resolv_delete_cache_for_net(netId);

    mDns64Configuration.stopPrefixDiscovery(netId);

    mDns64Configuration->stopPrefixDiscovery(netId);

    privateDnsConfiguration.clear(netId);

    privateDnsConfiguration.clear(netId);





    // Don't get this instance in PrivateDnsConfiguration. It's probe to deadlock.

    // Don't get this instance in PrivateDnsConfiguration. It's probe to deadlock.
@@ -276,16 +276,16 @@ int ResolverController::getResolverInfo(int32_t netId, std::vector<std::string>* 
}

}





void ResolverController::startPrefix64Discovery(int32_t netId) {

void ResolverController::startPrefix64Discovery(int32_t netId) {

    mDns64Configuration.startPrefixDiscovery(netId);

    mDns64Configuration->startPrefixDiscovery(netId);

}

}





void ResolverController::stopPrefix64Discovery(int32_t netId) {

void ResolverController::stopPrefix64Discovery(int32_t netId) {

    return mDns64Configuration.stopPrefixDiscovery(netId);

    return mDns64Configuration->stopPrefixDiscovery(netId);

}

}





// TODO: use StatusOr<T> to wrap the result.

// TODO: use StatusOr<T> to wrap the result.

int ResolverController::getPrefix64(unsigned netId, netdutils::IPPrefix* prefix) {

int ResolverController::getPrefix64(unsigned netId, netdutils::IPPrefix* prefix) {

    netdutils::IPPrefix p = mDns64Configuration.getPrefix64(netId);

    netdutils::IPPrefix p = mDns64Configuration->getPrefix64(netId);

    if (p.family() != AF_INET6 || p.length() == 0) {

    if (p.family() != AF_INET6 || p.length() == 0) {

        return -ENOENT;

        return -ENOENT;

    }

    }
@@ -352,7 +352,7 @@ void ResolverController::dump(DumpWriter& dw, unsigned netId) { 
                    params.max_samples, params.base_timeout_msec, params.retry_count);

                    params.max_samples, params.base_timeout_msec, params.retry_count);

        }

        }





        mDns64Configuration.dump(dw, netId);

        mDns64Configuration->dump(dw, netId);

        const auto privateDnsStatus = PrivateDnsConfiguration::getInstance().getStatus(netId);

        const auto privateDnsStatus = PrivateDnsConfiguration::getInstance().getStatus(netId);

        dw.println("Private DNS mode: %s", getPrivateDnsModeString(privateDnsStatus.mode));

        dw.println("Private DNS mode: %s", getPrivateDnsModeString(privateDnsStatus.mode));

        if (privateDnsStatus.dotServersMap.size() == 0) {

        if (privateDnsStatus.dotServersMap.size() == 0) {

ResolverController.h

+3 −3
Original line number Original line Diff line number Diff line
@@ -55,10 +55,10 @@ class ResolverController { 




    // Set or clear a NAT64 prefix discovered by other sources (e.g., RA).

    // Set or clear a NAT64 prefix discovered by other sources (e.g., RA).

    int setPrefix64(unsigned netId, const netdutils::IPPrefix& prefix) {

    int setPrefix64(unsigned netId, const netdutils::IPPrefix& prefix) {

        return mDns64Configuration.setPrefix64(netId, prefix);

        return mDns64Configuration->setPrefix64(netId, prefix);

    }

    }





    int clearPrefix64(unsigned netId) { return mDns64Configuration.clearPrefix64(netId); }

    int clearPrefix64(unsigned netId) { return mDns64Configuration->clearPrefix64(netId); }





    // Return the current NAT64 prefix network, regardless of how it was discovered.

    // Return the current NAT64 prefix network, regardless of how it was discovered.

    int getPrefix64(unsigned netId, netdutils::IPPrefix* prefix);

    int getPrefix64(unsigned netId, netdutils::IPPrefix* prefix);
@@ -66,7 +66,7 @@ class ResolverController { 
    void dump(netdutils::DumpWriter& dw, unsigned netId);

    void dump(netdutils::DumpWriter& dw, unsigned netId);





  private:

  private:

    Dns64Configuration mDns64Configuration;

    android::sp<Dns64Configuration> mDns64Configuration;

};

};

}  // namespace net

}  // namespace net

}  // namespace android

}  // namespace android