Stop advertising IPv4 addresses when IPv4 and IPv6 ports do not match. (#38057)

bzbarsky-apple · web-flow · commit 4593aef400ba · 2025-03-19T15:31:22.000Z
Our advertising backends only support advertising one port.  In cases when the
IPv4 and IPv6 ports do not match, advertising the IPv6 port along with IPv4
addresses does not make sense: that port does not correspond to the Matter stack
on those addresses.

The fix is to only allow advertising IPv4 addresses when the port we are using
for IPv6 matches the port we are using for IPv4.
diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp
@@ -207,7 +207,7 @@ CHIP_ERROR DnssdServer::AdvertiseOperational()
                                        .SetPort(GetSecuredPort())
                                        .SetInterfaceId(GetInterfaceId())
                                        .SetLocalMRPConfig(GetLocalMRPConfig().std_optional())
-                                       .EnableIpV4(true);
+                                       .EnableIpV4(SecuredIPv4PortMatchesIPv6Port());
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
         AddICDKeyToAdvertisement(advertiseParameters);
@@ -234,7 +234,7 @@ CHIP_ERROR DnssdServer::Advertise(bool commissionableNode, chip::Dnssd::Commissi
     auto advertiseParameters = chip::Dnssd::CommissionAdvertisingParameters()
                                    .SetPort(commissionableNode ? GetSecuredPort() : GetUnsecuredPort())
                                    .SetInterfaceId(GetInterfaceId())
-                                   .EnableIpV4(true);
+                                   .EnableIpV4(!commissionableNode || SecuredIPv4PortMatchesIPv6Port());
     advertiseParameters.SetCommissionAdvertiseMode(commissionableNode ? chip::Dnssd::CommssionAdvertiseMode::kCommissionableNode
                                                                       : chip::Dnssd::CommssionAdvertiseMode::kCommissioner);
 
diff --git a/src/app/server/Dnssd.h b/src/app/server/Dnssd.h
@@ -24,6 +24,7 @@
 #include <app/icd/server/ICDStateObserver.h>
 #include <app/server/CommissioningModeProvider.h>
 #include <credentials/FabricTable.h>
+#include <inet/InetConfig.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/Optional.h>
 #include <lib/dnssd/Advertiser.h>
@@ -46,11 +47,16 @@ class DLL_EXPORT DnssdServer : public ICDStateObserver
         return instance;
     }
 
-    /// Sets the secure Matter port
-    void SetSecuredPort(uint16_t port) { mSecuredPort = port; }
+    /// Sets the secure Matter IPv6 port
+    void SetSecuredIPv6Port(uint16_t port) { mSecuredIPv6Port = port; }
+
+#if INET_CONFIG_ENABLE_IPV4
+    /// Sets the secure Matter IPv4 port.
+    void SetSecuredIPv4Port(uint16_t port) { mSecuredIPv4Port = port; }
+#endif // INET_CONFIG_ENABLE_IPV4
 
     /// Gets the secure Matter port
-    uint16_t GetSecuredPort() const { return mSecuredPort; }
+    uint16_t GetSecuredPort() const { return mSecuredIPv6Port; }
 
     /// Sets the unsecure Matter port
     void SetUnsecuredPort(uint16_t port) { mUnsecuredPort = port; }
@@ -176,6 +182,18 @@ class DLL_EXPORT DnssdServer : public ICDStateObserver
     //
     bool HaveOperationalCredentials();
 
+    // Check whether the secured IPv4 port matches the secured IPv6 port.  If it
+    // does not, we should not advertise our IPv4 bits, because we can only
+    // advertise one port number.
+    bool SecuredIPv4PortMatchesIPv6Port() const
+    {
+#if INET_CONFIG_ENABLE_IPV4
+        return mSecuredIPv4Port == mSecuredIPv6Port;
+#else
+        return false;
+#endif // INET_CONFIG_ENABLE_IPV4
+    }
+
     FabricTable * mFabricTable                             = nullptr;
     CommissioningModeProvider * mCommissioningModeProvider = nullptr;
 
@@ -187,7 +205,10 @@ class DLL_EXPORT DnssdServer : public ICDStateObserver
     bool mTCPServerEnabled = true;
 #endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
 
-    uint16_t mSecuredPort          = CHIP_PORT;
+    uint16_t mSecuredIPv6Port = CHIP_PORT;
+#if INET_CONFIG_ENABLE_IPV4
+    uint16_t mSecuredIPv4Port = CHIP_PORT;
+#endif // INET_CONFIG_ENABLE_IPV4
     uint16_t mUnsecuredPort        = CHIP_UDC_PORT;
     Inet::InterfaceId mInterfaceId = Inet::InterfaceId::Null();
 
diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp
@@ -216,12 +216,17 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
     }
 
     // Init transport before operations with secure session mgr.
+    //
+    // The logic below expects that the IPv6 transport is at index 0. Keep that logic in sync with
+    // this code.
     err = mTransports.Init(UdpListenParameters(DeviceLayer::UDPEndPointManager())
                                .SetAddressType(IPAddressType::kIPv6)
                                .SetListenPort(mOperationalServicePort)
                                .SetNativeParams(initParams.endpointNativeParams)
 #if INET_CONFIG_ENABLE_IPV4
                                ,
+                           // The logic below expects that the IPv4 transport, if enabled, is at
+                           // index 1. Keep that logic in sync with this code.
                            UdpListenParameters(DeviceLayer::UDPEndPointManager())
                                .SetAddressType(IPAddressType::kIPv4)
                                .SetListenPort(mOperationalServicePort)
@@ -318,13 +323,10 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
     SuccessOrExit(err);
 #endif
 
-    //
-    // We need to advertise the port that we're listening to for unsolicited messages over UDP. However, we have both a IPv4
-    // and IPv6 endpoint to pick from. Given that the listen port passed in may be set to 0 (which then has the kernel select
-    // a valid port at bind time), that will result in two possible ports being provided back from the resultant endpoint
-    // initializations. Since IPv6 is POR for Matter, let's go ahead and pick that port.
-    //
-    app::DnssdServer::Instance().SetSecuredPort(mTransports.GetTransport().GetImplAtIndex<0>().GetBoundPort());
+    app::DnssdServer::Instance().SetSecuredIPv6Port(mTransports.GetTransport().GetImplAtIndex<0>().GetBoundPort());
+#if INET_CONFIG_ENABLE_IPV4
+    app::DnssdServer::Instance().SetSecuredIPv4Port(mTransports.GetTransport().GetImplAtIndex<1>().GetBoundPort());
+#endif // INET_CONFIG_ENABLE_IPV4
 
     app::DnssdServer::Instance().SetUnsecuredPort(mUserDirectedCommissioningPort);
     app::DnssdServer::Instance().SetInterfaceId(mInterfaceId);
diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp
@@ -169,13 +169,18 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
     stateParams.transportMgr = chip::Platform::New<DeviceTransportMgr>();
 
     //
-    // The logic below expects IPv6 to be at index 0 of this tuple. Please do not alter that.
+    // The logic below expects IPv6 to be at index 0 of this tuple. Keep that logic in sync with
+    // this code.
     //
     ReturnErrorOnFailure(stateParams.transportMgr->Init(Transport::UdpListenParameters(stateParams.udpEndPointManager)
                                                             .SetAddressType(Inet::IPAddressType::kIPv6)
                                                             .SetListenPort(params.listenPort)
 #if INET_CONFIG_ENABLE_IPV4
                                                             ,
+                                                        //
+                                                        // The logic below expects IPv4 to be at index 1 of this tuple,
+                                                        // if it's enabled. Keep that logic in sync with this code.
+                                                        //
                                                         Transport::UdpListenParameters(stateParams.udpEndPointManager)
                                                             .SetAddressType(Inet::IPAddressType::kIPv4)
                                                             .SetListenPort(params.listenPort)
@@ -272,13 +277,15 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
             stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, sessionResumptionStorage,
             stateParams.certificateValidityPolicy, stateParams.groupDataProvider));
 
-        //
-        // We need to advertise the port that we're listening to for unsolicited messages over UDP. However, we have both a IPv4
-        // and IPv6 endpoint to pick from. Given that the listen port passed in may be set to 0 (which then has the kernel select
-        // a valid port at bind time), that will result in two possible ports being provided back from the resultant endpoint
-        // initializations. Since IPv6 is POR for Matter, let's go ahead and pick that port.
-        //
-        app::DnssdServer::Instance().SetSecuredPort(stateParams.transportMgr->GetTransport().GetImplAtIndex<0>().GetBoundPort());
+        // Our IPv6 transport is at index 0.
+        app::DnssdServer::Instance().SetSecuredIPv6Port(
+            stateParams.transportMgr->GetTransport().GetImplAtIndex<0>().GetBoundPort());
+
+#if INET_CONFIG_ENABLE_IPV4
+        // If enabled, our IPv4 transport is at index 1.
+        app::DnssdServer::Instance().SetSecuredIPv4Port(
+            stateParams.transportMgr->GetTransport().GetImplAtIndex<1>().GetBoundPort());
+#endif // INET_CONFIG_ENABLE_IPV4
 
         //
         // TODO: This is a hack to workaround the fact that we have a bi-polar stack that has controller and server modalities that
diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp
@@ -22,6 +22,7 @@
 
 #include <app/icd/server/ICDServerConfig.h>
 #include <crypto/RandUtils.h>
+#include <inet/InetConfig.h>
 #include <lib/core/CHIPConfig.h>
 #include <lib/core/CHIPSafeCasts.h>
 #include <lib/dnssd/IPAddressSorter.h>
@@ -527,33 +528,39 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE
                                                  const OperationalAdvertisingParameters & params)
 {
     return PublishService(serviceType, textEntries, textEntrySize, subTypes, subTypeSize, params.GetPort(), params.GetInterfaceId(),
-                          params.GetMac(), DnssdServiceProtocol::kDnssdProtocolTcp, params.GetPeerId());
+                          params.GetMac(), DnssdServiceProtocol::kDnssdProtocolTcp, params.GetPeerId(), params.IsIPv4Enabled());
 }
 
 CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextEntry * textEntries, size_t textEntrySize,
                                                  const char ** subTypes, size_t subTypeSize,
                                                  const CommissionAdvertisingParameters & params)
 {
     return PublishService(serviceType, textEntries, textEntrySize, subTypes, subTypeSize, params.GetPort(), params.GetInterfaceId(),
-                          params.GetMac(), DnssdServiceProtocol::kDnssdProtocolUdp, PeerId());
+                          params.GetMac(), DnssdServiceProtocol::kDnssdProtocolUdp, PeerId(), params.IsIPv4Enabled());
 }
 
 CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextEntry * textEntries, size_t textEntrySize,
                                                  const char ** subTypes, size_t subTypeSize, uint16_t port,
                                                  Inet::InterfaceId interfaceId, const chip::ByteSpan & mac,
-                                                 DnssdServiceProtocol protocol, PeerId peerId)
+                                                 DnssdServiceProtocol protocol, PeerId peerId, bool ipv4Enabled)
 {
     DnssdService service;
     ReturnErrorOnFailure(MakeHostName(service.mHostName, sizeof(service.mHostName), mac));
     ReturnErrorOnFailure(protocol == DnssdServiceProtocol::kDnssdProtocolTcp
                              ? MakeInstanceName(service.mName, sizeof(service.mName), peerId)
                              : GetCommissionableInstanceName(service.mName, sizeof(service.mName)));
     Platform::CopyString(service.mType, serviceType);
-#if INET_CONFIG_ENABLE_IPV4
-    service.mAddressType = Inet::IPAddressType::kAny;
-#else
-    service.mAddressType = Inet::IPAddressType::kIPv6;
-#endif
+#if !INET_CONFIG_ENABLE_IPV4
+    ipv4Enabled = false;
+#endif // INET_CONFIG_ENABLE_IPV4
+    if (ipv4Enabled)
+    {
+        service.mAddressType = Inet::IPAddressType::kAny;
+    }
+    else
+    {
+        service.mAddressType = Inet::IPAddressType::kIPv6;
+    }
     service.mInterface     = interfaceId;
     service.mProtocol      = protocol;
     service.mPort          = port;
diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h
@@ -85,9 +85,10 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
                               size_t subTypeSize, const OperationalAdvertisingParameters & params);
     CHIP_ERROR PublishService(const char * serviceType, TextEntry * textEntries, size_t textEntrySize, const char ** subTypes,
                               size_t subTypeSize, const CommissionAdvertisingParameters & params);
+    // ipv4Enabled will be ignored if we don't actually support IPv4.
     CHIP_ERROR PublishService(const char * serviceType, TextEntry * textEntries, size_t textEntrySize, const char ** subTypes,
                               size_t subTypeSize, uint16_t port, Inet::InterfaceId interfaceId, const chip::ByteSpan & mac,
-                              DnssdServiceProtocol procotol, PeerId peerId);
+                              DnssdServiceProtocol procotol, PeerId peerId, bool ipv4Enabled);
 
     static void HandleNodeIdResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & addresses,
                                     CHIP_ERROR error);
