Pull request 2357: 7555-fix-ra-packets

Merge in DNS/adguard-home from 7555-fix-ra-packets to master

* commit '0ed2cd04b22a7b447380ee863bc908603720877d':
  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

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

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
+	// raAllowSLAAC is used to determine if the ICMP Router Advertisement
-	raSLAACOnly      bool   // send RA packets with MO flags
+	// messages should be sent.
-	ipAddr           net.IP // source IP address (link-local-unicast)
+	//
-	dnsIPAddr        net.IP // IP address for DNS Server option
+	// If both raAllowSLAAC and raSLAACOnly are false, the Router Advertisement
-	prefixIPAddr     net.IP // IP address for Prefix option
+	// messages aren't sent.
-	ifaceName        string
+	raAllowSLAAC bool
-	iface            *net.Interface
-	packetSendPeriod time.Duration // how often RA packets are sent
 
-	conn *icmp.PacketConn // ICMPv6 socket
+	// raSLAACOnly is used to determine if the ICMP Router Advertisement
-	stop atomic.Value     // stop the packet sending loop
+	// 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
+// hwAddrToLinkLayerAddr clones the hardware address and returns it as a byte
-// RFC4861.  That is, a byte slice of length divisible by 8.
+// 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 {
+	return slices.Clone(hwa), nil
-		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
 }
 
 // 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..
+	// and make all constants named instead of all those comments.
-	data = make([]byte, 82+len(lla))
+	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                         // Type
-	data[i+1] = 1 // Length
+	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:
 

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)
+}

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)
-}