From f5951192c9e0bbaad200e55ea3d9208f22d96b11 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 18 Mar 2025 12:27:38 -0500 Subject: [PATCH] Stop advertising IPv4 addresses when IPv4 and IPv6 ports do not match. 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. --- src/app/server/Dnssd.cpp | 4 +-- src/app/server/Dnssd.h | 29 ++++++++++++++++--- src/app/server/Server.cpp | 16 +++++----- .../CHIPDeviceControllerFactory.cpp | 23 ++++++++++----- src/lib/dnssd/Discovery_ImplPlatform.cpp | 23 ++++++++++----- src/lib/dnssd/Discovery_ImplPlatform.h | 3 +- 6 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index a2bad90d8507d2..bce567f95f8d98 100644 --- 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 index 1fef479ca3eba4..1b82fb909653a7 100644 --- a/src/app/server/Dnssd.h +++ b/src/app/server/Dnssd.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -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 index 5aa7577606cc13..e9586c6a7c2775 100644 --- 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 index cbc29de5f86338..b537f01fe3ecd1 100644 --- 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(); // - // 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 index 0ecf30640d90a5..f43ded783f8bc5 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -527,7 +528,7 @@ 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, @@ -535,13 +536,13 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE 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)); @@ -549,11 +550,17 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE ? 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 index d51d5e758c0268..f57171a7bb9556 100644 --- 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 & addresses, CHIP_ERROR error);