Pull request 2357: 7555-fix-ra-packets

Merge in DNS/adguard-home from 7555-fix-ra-packets to master * commit '0ed2cd04b2': dhcpd: imp docs all: fix changelog dhcpd: imp docs all: imp code & test, log changes Change comment message to match implementation dhcpd: Fix slaac configuration which was not working due to invalid icmp packet
2025-04-30 21:21:42 +03:00 · 2025-03-10 16:30:15 +03:00 · 2025-03-10 16:30:15 +03:00 · bf9be98c71
commit bf9be98c71
parent 66fba942c8 0ed2cd04b2
4 changed files with 133 additions and 73 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -24,11 +24,17 @@ NOTE: Add new changes BELOW THIS COMMENT.

 ### Fixed

+- Invalid ICMPv6 Router Advertisement messages ([#7547]).
+
 - Disabled button for autofilled login form.
+
 - Formatting of elapsed times less than one millisecond.
+
 - Changes to global upstream DNS settings not applying to custom client upstream configurations.
+
 - The formatting of large numbers in the clients tables on the *Client settings* page ([#7583]).

+[#7547]: https://github.com/AdguardTeam/AdGuardHome/issues/7547
 [#7583]: https://github.com/AdguardTeam/AdGuardHome/issues/7583

 [go-1.24.1]: https://groups.google.com/g/golang-announce/c/4t3lzH3I0eI
@ -56,6 +62,7 @@ See also the [v0.107.57 GitHub milestone][ms-v0.107.57].
 ### Fixed

 - The hostnames of DHCP clients not being shown in the *Top clients* table on the dashboard ([#7627]).
+
 - The formatting of large numbers in the upstream table and query log ([#7590]).

 [#7590]: https://github.com/AdguardTeam/AdGuardHome/issues/7590
--- a/internal/dhcpd/routeradv.go
+++ b/internal/dhcpd/routeradv.go
@ -4,6 +4,7 @@ import (
 	"encoding/binary"
 	"fmt"
 	"net"
+	"slices"
 	"sync/atomic"
 	"time"

@ -14,18 +15,48 @@ import (
 	"golang.org/x/net/ipv6"
 )

+// raCtx is a context for the Router Advertisement logic.
 type raCtx struct {
-	raAllowSLAAC     bool   // send RA packets without MO flags
-	raSLAACOnly      bool   // send RA packets with MO flags
-	ipAddr           net.IP // source IP address (link-local-unicast)
-	dnsIPAddr        net.IP // IP address for DNS Server option
-	prefixIPAddr     net.IP // IP address for Prefix option
-	ifaceName        string
-	iface            *net.Interface
-	packetSendPeriod time.Duration // how often RA packets are sent
+	// raAllowSLAAC is used to determine if the ICMP Router Advertisement
+	// messages should be sent.
+	//
+	// If both raAllowSLAAC and raSLAACOnly are false, the Router Advertisement
+	// messages aren't sent.
+	raAllowSLAAC bool

-	conn *icmp.PacketConn // ICMPv6 socket
-	stop atomic.Value     // stop the packet sending loop
+	// raSLAACOnly is used to determine if the ICMP Router Advertisement
+	// messages should set M and O flags, see RFC 4861, section 4.2.
+	//
+	// If both raAllowSLAAC and raSLAACOnly are false, the Router Advertisement
+	// messages aren't sent.
+	raSLAACOnly bool
+
+	// ipAddr is an IP address used within the Source Link-Layer Address option.
+	// See RFC 4861, section 4.6.1.
+	ipAddr net.IP
+
+	// dnsIPAddr is an IP address used within the DNS Server option.
+	dnsIPAddr net.IP
+
+	// prefixIPAddr is an IP address used within the Prefix Information option.
+	// See RFC 4861, section 4.6.2.
+	prefixIPAddr net.IP
+
+	// ifaceName is the name of the interface used as a scope of the IP
+	// addresses.
+	ifaceName string
+
+	// iface is the network interface used to send the ICMPv6 packets.
+	iface *net.Interface
+
+	// packetSendPeriod is the interval between sending the ICMPv6 packets.
+	packetSendPeriod time.Duration
+
+	// conn is the ICMPv6 socket.
+	conn *icmp.PacketConn
+
+	// stop is used to stop the packet sending loop.
+	stop atomic.Value
 }

 type icmpv6RA struct {
@ -38,10 +69,11 @@ type icmpv6RA struct {
 	mtu                         uint32
 }

-// hwAddrToLinkLayerAddr converts a hardware address into a form required by
-// RFC4861.  That is, a byte slice of length divisible by 8.
+// hwAddrToLinkLayerAddr clones the hardware address and returns it as a byte
+// slice suitable for the Source Link-Layer Address option in the ICMPv6
+// Router Advertisement packet.
 //
-// See https://tools.ietf.org/html/rfc4861#section-4.6.1.
+// TODO(e.burkov):  Check if it's safe to use the original slice.
 func hwAddrToLinkLayerAddr(hwa net.HardwareAddr) (lla []byte, err error) {
 	err = netutil.ValidateMAC(hwa)
 	if err != nil {
@ -50,19 +82,7 @@ func hwAddrToLinkLayerAddr(hwa net.HardwareAddr) (lla []byte, err error) {
 		return nil, err
 	}

-	if len(hwa) == 6 || len(hwa) == 8 {
-		lla = make([]byte, 8)
-		copy(lla, hwa)
-
-		return lla, nil
-	}
-
-	// Assume that netutil.ValidateMAC prevents lengths other than 20 by
-	// now.
-	lla = make([]byte, 24)
-	copy(lla, hwa)
-
-	return lla, nil
+	return slices.Clone(hwa), nil
 }

 // Create an ICMPv6.RouterAdvertisement packet with all necessary options.
@ -103,15 +123,24 @@ func hwAddrToLinkLayerAddr(hwa net.HardwareAddr) (lla []byte, err error) {
 //
 // TODO(a.garipov): Replace with an existing implementation from a dependency.
 func createICMPv6RAPacket(params icmpv6RA) (data []byte, err error) {
-	var lla []byte
-	lla, err = hwAddrToLinkLayerAddr(params.sourceLinkLayerAddress)
+	lla, err := hwAddrToLinkLayerAddr(params.sourceLinkLayerAddress)
 	if err != nil {
-		return nil, fmt.Errorf("converting source link layer address: %w", err)
+		return nil, fmt.Errorf("converting source link-layer address: %w", err)
 	}

+	// Calculate length of the source link-layer address option.  As per RFC
+	// 4861, section 4.6.1, the length should be in units of 8 octets, including
+	// the type and length fields.
+	//
+	// See https://datatracker.ietf.org/doc/html/rfc4861#section-4.6.1.
+	srcLLAOptLen := len(lla) + 2
+	// Make sure the value is rounded up to the nearest multiple of 8.
+	srcLLAOptLenValue := (srcLLAOptLen + 7) / 8
+	srcLLAPadLen := srcLLAOptLenValue*8 - srcLLAOptLen
+
 	// TODO(a.garipov): Don't use a magic constant here.  Refactor the code
-	// and make all constants named instead of all those comments..
-	data = make([]byte, 82+len(lla))
+	// and make all constants named instead of all those comments.
+	data = make([]byte, 80+srcLLAOptLen+srcLLAPadLen)
 	i := 0

 	// ICMPv6:
@ -175,12 +204,11 @@ func createICMPv6RAPacket(params icmpv6RA) (data []byte, err error) {

 	// Option=Source link-layer address:

-	data[i] = 1   // Type
-	data[i+1] = 1 // Length
+	data[i] = 1                         // Type
+	data[i+1] = byte(srcLLAOptLenValue) // Length
 	i += 2
-
 	copy(data[i:], lla) // Link-Layer Address[8/24]
-	i += len(lla)
+	i += len(lla) + srcLLAPadLen

 	// Option=Recursive DNS Server:

--- a/internal/dhcpd/routeradv_internal_test.go
+++ b/internal/dhcpd/routeradv_internal_test.go
@ -0,0 +1,63 @@
+package dhcpd
+
+import (
+	"net"
+	"testing"
+
+	"github.com/google/gopacket"
+	"github.com/google/gopacket/layers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestCreateICMPv6RAPacket(t *testing.T) {
+	raConf := icmpv6RA{
+		managedAddressConfiguration: false,
+		otherConfiguration:          true,
+		mtu:                         1500,
+		prefix:                      net.ParseIP("1234::"),
+		prefixLen:                   64,
+		recursiveDNSServer:          net.ParseIP("fe80::800:27ff:fe00:0"),
+		sourceLinkLayerAddress:      []byte{0x0A, 0x00, 0x27, 0x00, 0x00, 0x00},
+	}
+
+	pkt, err := createICMPv6RAPacket(raConf)
+	require.NoError(t, err)
+
+	icmpPkt := &layers.ICMPv6{}
+	err = icmpPkt.DecodeFromBytes(pkt, gopacket.NilDecodeFeedback)
+	require.NoError(t, err)
+
+	require.Equal(t, layers.LayerTypeICMPv6RouterAdvertisement, icmpPkt.NextLayerType())
+	raPkt := &layers.ICMPv6RouterAdvertisement{}
+	err = raPkt.DecodeFromBytes(icmpPkt.LayerPayload(), gopacket.NilDecodeFeedback)
+	require.NoError(t, err)
+
+	assert.Equal(t, raConf.managedAddressConfiguration, raPkt.ManagedAddressConfig())
+	assert.Equal(t, raConf.otherConfiguration, raPkt.OtherConfig())
+
+	wantOpts := layers.ICMPv6Options{{
+		Type: layers.ICMPv6OptPrefixInfo,
+		Data: []uint8{
+			0x40, 0xC0, 0x00, 0x00, 0x0E, 0x10, 0x00, 0x00,
+			0x0E, 0x10, 0x00, 0x00, 0x00, 0x00, 0x12, 0x34,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		},
+	}, {
+		Type: layers.ICMPv6OptMTU,
+		Data: []uint8{0x00, 0x00, 0x00, 0x00, 0x05, 0xDC},
+	}, {
+		Type: layers.ICMPv6OptSourceAddress,
+		Data: []uint8{0x0A, 0x00, 0x27, 0x00, 0x00, 0x0},
+	}, {
+		// Package layers declares no constant for Recursive DNS Server option.
+		Type: layers.ICMPv6Opt(25),
+		Data: []uint8{
+			0x00, 0x00, 0x00, 0x00, 0x0E, 0x10, 0xFE, 0x80,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00,
+			0x27, 0xFF, 0xFE, 0x00, 0x00, 0x00,
+		},
+	}}
+	assert.Equal(t, wantOpts, raPkt.Options)
+}
--- a/internal/dhcpd/routeradv_test.go
+++ b/internal/dhcpd/routeradv_test.go
@ -1,38 +0,0 @@
-package dhcpd
-
-import (
-	"net"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestCreateICMPv6RAPacket(t *testing.T) {
-	wantData := []byte{
-		0x86, 0x00, 0x00, 0x00, 0x40, 0x40, 0x07, 0x08,
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x03, 0x04, 0x40, 0xc0, 0x00, 0x00, 0x0e, 0x10,
-		0x00, 0x00, 0x0e, 0x10, 0x00, 0x00, 0x00, 0x00,
-		0x12, 0x34, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x05, 0x01, 0x00, 0x00, 0x00, 0x00, 0x05, 0xdc,
-		0x01, 0x01, 0x0a, 0x00, 0x27, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x19, 0x03, 0x00, 0x00, 0x00, 0x00,
-		0x0e, 0x10, 0xfe, 0x80, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x08, 0x00, 0x27, 0xff, 0xfe, 0x00,
-		0x00, 0x00,
-	}
-
-	gotData, err := createICMPv6RAPacket(icmpv6RA{
-		managedAddressConfiguration: false,
-		otherConfiguration:          true,
-		mtu:                         1500,
-		prefix:                      net.ParseIP("1234::"),
-		prefixLen:                   64,
-		recursiveDNSServer:          net.ParseIP("fe80::800:27ff:fe00:0"),
-		sourceLinkLayerAddress:      []byte{0x0a, 0x00, 0x27, 0x00, 0x00, 0x00},
-	})
-
-	assert.NoError(t, err)
-	assert.Equal(t, wantData, gotData)
-}