Pull request: dhcpd: do not assume mac addrs of 6 bytes

Closes AdguardTeam#2828.

Squashed commit of the following:

commit 26c6cf8
Author: Ainar Garipov <[email protected]>
Date:   Tue Mar 30 17:43:53 2021 +0300

    dhcpd: do not assume mac addrs of 6 bytes

CHANGELOG.md

@@ -33,6 +33,7 @@ and this project adheres to
 
 ### Fixed
 
+- Assumption that MAC addresses always have the length of 6 octets ([#2828]).
 - Support for more than one `/24` subnet in DHCP ([#2541]).
 - Invalid filenames in the `mobileconfig` API responses ([#2835]).
 
@@ -47,6 +48,7 @@ and this project adheres to
 [#2498]: https://github.com/AdguardTeam/AdGuardHome/issues/2498
 [#2533]: https://github.com/AdguardTeam/AdGuardHome/issues/2533
 [#2541]: https://github.com/AdguardTeam/AdGuardHome/issues/2541
+[#2828]: https://github.com/AdguardTeam/AdGuardHome/issues/2828
 [#2835]: https://github.com/AdguardTeam/AdGuardHome/issues/2835
 [#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
 

internal/aghnet/addr.go

@@ -0,0 +1,23 @@
+package aghnet
+
+import (
+	"fmt"
+	"net"
+
+	"github.com/AdguardTeam/AdGuardHome/internal/agherr"
+)
+
+// ValidateHardwareAddress returns an error if hwa is not a valid EUI-48,
+// EUI-64, or 20-octet InfiniBand link-layer address.
+func ValidateHardwareAddress(hwa net.HardwareAddr) (err error) {
+	defer agherr.Annotate("validating hardware address %q: %w", &err, hwa)
+
+	switch l := len(hwa); l {
+	case 0:
+		return agherr.Error("address is empty")
+	case 6, 8, 20:
+		return nil
+	default:
+		return fmt.Errorf("bad len: %d", l)
+	}
+}

internal/aghnet/addr_test.go

@@ -0,0 +1,57 @@
+package aghnet
+
+import (
+	"net"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateHardwareAddress(t *testing.T) {
+	testCases := []struct {
+		name       string
+		wantErrMsg string
+		in         net.HardwareAddr
+	}{{
+		name:       "success_eui_48",
+		wantErrMsg: "",
+		in:         net.HardwareAddr{0x00, 0x01, 0x02, 0x03, 0x04, 0x05},
+	}, {
+		name:       "success_eui_64",
+		wantErrMsg: "",
+		in:         net.HardwareAddr{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07},
+	}, {
+		name:       "success_infiniband",
+		wantErrMsg: "",
+		in: net.HardwareAddr{
+			0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+			0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
+			0x10, 0x11, 0x12, 0x13,
+		},
+	}, {
+		name:       "error_nil",
+		wantErrMsg: `validating hardware address "": address is empty`,
+		in:         nil,
+	}, {
+		name:       "error_empty",
+		wantErrMsg: `validating hardware address "": address is empty`,
+		in:         net.HardwareAddr{},
+	}, {
+		name:       "error_bad",
+		wantErrMsg: `validating hardware address "00:01:02:03": bad len: 4`,
+		in:         net.HardwareAddr{0x00, 0x01, 0x02, 0x03},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidateHardwareAddress(tc.in)
+			if tc.wantErrMsg == "" {
+				assert.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				assert.Equal(t, tc.wantErrMsg, err.Error())
+			}
+		})
+	}
+}

internal/dhcpd/routeradv.go

@@ -7,6 +7,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/golibs/log"
 	"golang.org/x/net/icmp"
 	"golang.org/x/net/ipv6"
@@ -36,6 +37,33 @@ 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.
+//
+// See https://tools.ietf.org/html/rfc4861#section-4.6.1.
+func hwAddrToLinkLayerAddr(hwa net.HardwareAddr) (lla []byte, err error) {
+	err = aghnet.ValidateHardwareAddress(hwa)
+	if err != nil {
+		// Don't wrap the error, because it already contains enough
+		// context.
+		return nil, err
+	}
+
+	if len(hwa) == 6 || len(hwa) == 8 {
+		lla = make([]byte, 8)
+		copy(lla, hwa)
+
+		return lla, nil
+	}
+
+	// Assume that aghnet.ValidateHardwareAddress 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.
 //
 // ICMPv6:
@@ -63,15 +91,23 @@ type icmpv6RA struct {
 //     Reserved[2]
 //     MTU[4]
 //   Option=Source link-layer address(1):
-//     Link-Layer Address[6]
+//     Link-Layer Address[8/24]
 //   Option=Recursive DNS Server(25):
 //     Type[1]
 //     Length * 8bytes[1]
 //     Reserved[2]
 //     Lifetime[4]
 //     Addresses of IPv6 Recursive DNS Servers[16]
-func createICMPv6RAPacket(params icmpv6RA) []byte {
-	data := make([]byte, 88)
+func createICMPv6RAPacket(params icmpv6RA) (data []byte, err error) {
+	var lla []byte
+	lla, err = hwAddrToLinkLayerAddr(params.sourceLinkLayerAddress)
+	if err != nil {
+		return nil, fmt.Errorf("converting source link layer address: %w", err)
+	}
+
+	// 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))
 	i := 0
 
 	// ICMPv6:
@@ -138,8 +174,9 @@ func createICMPv6RAPacket(params icmpv6RA) []byte {
 	data[i] = 1   // Type
 	data[i+1] = 1 // Length
 	i += 2
-	copy(data[i:], params.sourceLinkLayerAddress) // Link-Layer Address[6]
-	i += 6
+
+	copy(data[i:], lla) // Link-Layer Address[8/24]
+	i += len(lla)
 
 	// Option=Recursive DNS Server:
 
@@ -152,11 +189,11 @@ func createICMPv6RAPacket(params icmpv6RA) []byte {
 	i += 4
 	copy(data[i:], params.recursiveDNSServer) // Addresses of IPv6 Recursive DNS Servers[16]
 
-	return data
+	return data, nil
 }
 
 // Init - initialize RA module
-func (ra *raCtx) Init() error {
+func (ra *raCtx) Init() (err error) {
 	ra.stop.Store(0)
 	ra.conn = nil
 	if !(ra.raAllowSLAAC || ra.raSLAACOnly) {
@@ -177,9 +214,12 @@ func (ra *raCtx) Init() error {
 	params.prefix = make([]byte, 16)
 	copy(params.prefix, ra.prefixIPAddr[:8]) // /64
 
-	data := createICMPv6RAPacket(params)
+	var data []byte
+	data, err = createICMPv6RAPacket(params)
+	if err != nil {
+		return fmt.Errorf("creating packet: %w", err)
+	}
 
-	var err error
 	success := false
 	ipAndScope := ra.ipAddr.String() + "%" + ra.ifaceName
 	ra.conn, err = icmp.ListenPacket("ip6:ipv6-icmp", ipAndScope)

internal/dhcpd/routeradv_test.go

@@ -7,8 +7,23 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestRA(t *testing.T) {
-	data := createICMPv6RAPacket(icmpv6RA{
+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,
@@ -17,13 +32,7 @@ func TestRA(t *testing.T) {
 		recursiveDNSServer:          net.ParseIP("fe80::800:27ff:fe00:0"),
 		sourceLinkLayerAddress:      []byte{0x0a, 0x00, 0x27, 0x00, 0x00, 0x00},
 	})
-	dataCorrect := []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,
-		0x19, 0x03, 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, dataCorrect, data)
+
+	assert.NoError(t, err)
+	assert.Equal(t, wantData, gotData)
 }

internal/dhcpd/v4.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/agherr"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/go-ping/ping"
 	"github.com/insomniacslk/dhcp/dhcpv4"
@@ -289,8 +290,9 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) {
 		return fmt.Errorf("invalid ip %q, only ipv4 is supported", l.IP)
 	}
 
-	if len(l.HWAddr) != 6 {
-		return fmt.Errorf("invalid mac %q, only EUI-48 is supported", l.HWAddr)
+	err = aghnet.ValidateHardwareAddress(l.HWAddr)
+	if err != nil {
+		return fmt.Errorf("validating lease: %w", err)
 	}
 
 	l.Expiry = time.Unix(leaseExpireStatic, 0)
@@ -330,17 +332,21 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) {
 	return nil
 }
 
-// RemoveStaticLease removes a static lease (thread-safe)
-func (s *v4Server) RemoveStaticLease(l Lease) error {
+// RemoveStaticLease removes a static lease.  It is safe for concurrent use.
+func (s *v4Server) RemoveStaticLease(l Lease) (err error) {
+	defer agherr.Annotate("dhcpv4: %w", &err)
+
 	if len(l.IP) != 4 {
 		return fmt.Errorf("invalid IP")
 	}
-	if len(l.HWAddr) != 6 {
-		return fmt.Errorf("invalid MAC")
+
+	err = aghnet.ValidateHardwareAddress(l.HWAddr)
+	if err != nil {
+		return fmt.Errorf("validating lease: %w", err)
 	}
 
 	s.leasesLock.Lock()
-	err := s.rmLease(l)
+	err = s.rmLease(l)
 	if err != nil {
 		s.leasesLock.Unlock()
 
@@ -688,8 +694,10 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4
 		return
 	}
 
-	if len(req.ClientHWAddr) != 6 {
-		log.Debug("dhcpv4: Invalid ClientHWAddr")
+	err = aghnet.ValidateHardwareAddress(req.ClientHWAddr)
+	if err != nil {
+		log.Error("dhcpv4: invalid ClientHWAddr: %s", err)
+
 		return
 	}