Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ip6] filter host originated multicast packets #9735

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions etc/cmake/options.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ ot_option(OT_LINK_METRICS_INITIATOR OPENTHREAD_CONFIG_MLE_LINK_METRICS_INITIATOR
ot_option(OT_LINK_METRICS_MANAGER OPENTHREAD_CONFIG_LINK_METRICS_MANAGER_ENABLE "link metrics manager")
ot_option(OT_LINK_METRICS_SUBJECT OPENTHREAD_CONFIG_MLE_LINK_METRICS_SUBJECT_ENABLE "link metrics subject")
ot_option(OT_LINK_RAW OPENTHREAD_CONFIG_LINK_RAW_ENABLE "link raw service")
ot_option(OT_LOCAL_HOST_MULTICAST_FILTER OPENTHREAD_CONFIG_FILTER_LOCAL_ORIGINATED_MULTICAST_PACKETS "filter local originated multicast packet")
ot_option(OT_LOG_LEVEL_DYNAMIC OPENTHREAD_CONFIG_LOG_LEVEL_DYNAMIC_ENABLE "dynamic log level control")
ot_option(OT_MAC_FILTER OPENTHREAD_CONFIG_MAC_FILTER_ENABLE "mac filter")
ot_option(OT_MESH_DIAG OPENTHREAD_CONFIG_MESH_DIAG_ENABLE "mesh diag")
Expand Down
1 change: 1 addition & 0 deletions script/test
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ do_build_otbr_docker()
"-DOT_COVERAGE=ON"
"-DOT_DNS_CLIENT=ON"
"-DOT_DUA=ON"
"-DOT_LOCAL_HOST_MULTICAST_FILTER=ON"
"-DOT_MLR=ON"
"-DOT_NETDATA_PUBLISHER=ON"
"-DOT_SLAAC=ON"
Expand Down
15 changes: 15 additions & 0 deletions src/core/backbone_router/multicast_listeners_table.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,21 @@ Error MulticastListenersTable::GetNext(Listener::Iterator &aIterator, Listener::
return error;
}

bool MulticastListenersTable::HasListener(const Ip6::Address &aAddress) const
{
bool hasListener = false;

for (uint16_t i = 0; i < mNumValidListeners; i++)
{
if (mListeners[i].GetAddress() == aAddress)
{
hasListener = true;
break;
}
}
return hasListener;
}

} // namespace BackboneRouter

} // namespace ot
Expand Down
10 changes: 10 additions & 0 deletions src/core/backbone_router/multicast_listeners_table.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,16 @@ class MulticastListenersTable : public InstanceLocator, private NonCopyable
*/
Error GetNext(Listener::Iterator &aIterator, Listener::Info &aInfo);

/**
* Checks if a given address has a listener.
*
* @param[in] aAddress An Ipv6 address
*
* @retval true @p aAddress has a listener
* @retval false @p aAddress doesn't have a listener
*/
bool HasListener(const Ip6::Address &aAddress) const;

private:
static constexpr uint16_t kTableSize = OPENTHREAD_CONFIG_MAX_MULTICAST_LISTENERS;

Expand Down
24 changes: 24 additions & 0 deletions src/core/net/ip6.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

#include "backbone_router/bbr_leader.hpp"
#include "backbone_router/bbr_local.hpp"
#include "backbone_router/multicast_listeners_table.hpp"
#include "backbone_router/ndproxy_table.hpp"
#include "common/code_utils.hpp"
#include "common/debug.hpp"
Expand Down Expand Up @@ -1048,6 +1049,9 @@ Error Ip6::SendRaw(OwnedPtr<Message> aMessagePtr)
{
Error error = kErrorNone;
Header header;
bool forwardThread;

OT_UNUSED_VARIABLE(forwardThread);

SuccessOrExit(error = header.ParseFrom(*aMessagePtr));
VerifyOrExit(!header.GetSource().IsMulticast(), error = kErrorInvalidSourceAddress);
Expand All @@ -1068,6 +1072,26 @@ Error Ip6::SendRaw(OwnedPtr<Message> aMessagePtr)

if (header.GetDestination().IsMulticast())
{
#if OPENTHREAD_CONFIG_FILTER_LOCAL_ORIGINATED_MULTICAST_PACKETS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a new API to control the behavior at runtime? I think this is a security feature, and should be controlled by upper layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update this PR to add API control for it.

// For multicast packet originating from untrusted source on host and passed to OT stack,
// only forward to thread network if destination is in the multicast listener table.
if (aMessagePtr->IsOriginHostUntrusted() && Get<ThreadNetif>().HasUnicastAddress(header.GetSource()))
{
#if OPENTHREAD_CONFIG_BACKBONE_ROUTER_MULTICAST_ROUTING_ENABLE && OPENTHREAD_FTD && \
OPENTHREAD_CONFIG_BACKBONE_ROUTER_ENABLE
forwardThread = (header.GetDestination().IsMulticastLargerThanRealmLocal() &&
Get<BackboneRouter::MulticastListenersTable>().HasListener(header.GetDestination()));
#else
forwardThread = false;
superwhd marked this conversation as resolved.
Show resolved Hide resolved
#endif
if (!forwardThread)
{
LogInfo("Dropping host originated multicast packet when destination is not MLR subscribed.");
}
VerifyOrExit(forwardThread, error = kErrorDrop);
}
#endif

Copy link
Member

@abtink abtink Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this feature we can decide not to forward this message over the thread mesh (to other nodes) which sounds reasonable. But the VerifyOrExit() check here, we also disallow it to be received by the device itself (since we skip calling HandleDatagram()).

What if this device itself is subscribed to the destination of the multicast message? Do we need to receive/process this multicast? Or should we drop it here as the desired behavior? (I don't have any answer/suggestion myself, just raising it so we consider and think about this situation)

Related to this, we have otMessageSetMulticastLoopEnabled() which allows caller to inidcate whether we want to allow multicast loops.

If the goal is to only disallow forwarding over thread mesh, we can consider moving this new code block to HandleDatagram() method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Letting otMessageSetMulticastLoopEnabled() decide if multicast loops are allowed for host originated packets sounds good to me, I'll update this PR to move the code to HandleDatagram().

SuccessOrExit(error = InsertMplOption(*aMessagePtr, header));
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
#!/usr/bin/env python3
#
# Copyright (c) 2023, The OpenThread Authors.
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are met:
# 1. Redistributions of source code must retain the above copyright
# notice, this list of conditions and the following disclaimer.
# 2. Redistributions in binary form must reproduce the above copyright
# notice, this list of conditions and the following disclaimer in the
# documentation and/or other materials provided with the distribution.
# 3. Neither the name of the copyright holder nor the
# names of its contributors may be used to endorse or promote products
# derived from this software without specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS 'AS IS'
# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
# POSSIBILITY OF SUCH DAMAGE.
#
import unittest

import thread_cert
import config
from pktverify.consts import ICMPV6_TYPE_DESTINATION_UNREACHABLE, MA1, MA2, MA5, MA6
from pktverify.packet_verifier import PacketVerifier

PBBR = 1
ROUTER = 2


# Test description:
# This test verifies that the multicast packet originated from BBR is only
# forwarded to thread network when the destination group address is in the
# multicast listener table.
#
# Topology:
#
# ------(eth)----------
# |
# PBBR---ROUTER
#
class TestMlrMulticastRoutingSelfOriginatedPacket(thread_cert.TestCase):
USE_MESSAGE_FACTORY = False

TOPOLOGY = {
PBBR: {
'name': 'PBBR',
'allowlist': [ROUTER],
'is_otbr': True,
'version': '1.2',
},
ROUTER: {
'name': 'ROUTER',
'allowlist': [PBBR],
'version': '1.2',
},
}

def test(self):
self.nodes[PBBR].start()
self.simulator.go(config.LEADER_STARTUP_DELAY)
self.assertEqual('leader', self.nodes[PBBR].get_state())
self.nodes[PBBR].enable_backbone_router()
self.simulator.go(3)
self.assertTrue(self.nodes[PBBR].is_primary_backbone_router)

self.nodes[ROUTER].start()
self.simulator.go(5)
self.assertEqual('router', self.nodes[ROUTER].get_state())

self.simulator.go(5)

self.collect_ipaddrs()

# Verify PBBR can send packets from host to MLR registered multicast address on Thread network
self.nodes[ROUTER].add_ipmaddr(MA1)
self.simulator.go(3)
self.assertTrue(self.nodes[PBBR].ping(MA1, backbone=True, ttl=10, interface=config.THREAD_IFNAME))

# Verify PBBR can't send packets from host to not registered multicast address on Thread network
self.assertFalse(self.nodes[PBBR].ping(MA2, backbone=True, ttl=10, interface=config.THREAD_IFNAME))

# Verify PBBR can't send packets from host to multicast address with scope <= realm-local to Thread network
self.assertFalse(self.nodes[PBBR].ping(MA5, backbone=True, ttl=10, interface=config.THREAD_IFNAME))
self.assertFalse(self.nodes[PBBR].ping(MA6, backbone=True, ttl=10, interface=config.THREAD_IFNAME))

def verify(self, pv: PacketVerifier):
pkts = pv.pkts
pv.add_common_vars()
pv.summary.show()

with pkts.save_index():
pv.verify_attached('ROUTER')

PBBR = pv.vars['PBBR']

# PBBR should send ping request to MA1 to Thread network
pkts.filter_wpan_src64(PBBR).filter_AMPLFMA().filter(
lambda p: p.ipv6inner.dst == MA1).filter_ping_request().must_next()

# PBBR should not send ping request to MA2 to Thread network
pkts.filter_wpan_src64(PBBR).filter_AMPLFMA().filter(
lambda p: p.ipv6inner.dst == MA2).filter_ping_request().must_not_next()

# PBBR should not send ping request to MA5 to Thread network
pkts.filter_wpan_src64(PBBR).filter_AMPLFMA().filter(
lambda p: p.ipv6inner.dst == MA5).filter_ping_request().must_not_next()

# PBBR should not send ping request to MA6 to Thread network
pkts.filter_wpan_src64(PBBR).filter_AMPLFMA().filter(
lambda p: p.ipv6inner.dst == MA6).filter_ping_request().must_not_next()


if __name__ == '__main__':
unittest.main()
Loading