Skip to content

Commit 26c6cf8

Browse files
ainar-gainar-g
committed
dhcpd: do not assume mac addrs of 6 bytes
1 parent 48e82f9 commit 26c6cf8

File tree

7 files changed

+204
-48
lines changed

7 files changed

+204
-48
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ and this project adheres to
3333

3434
### Fixed
3535

36+
- Assumption that MAC addresses always have the length of 6 octets ([#2828]).
3637
- Support for more than one `/24` subnet in DHCP ([#2541]).
3738
- Invalid filenames in the `mobileconfig` API responses ([#2835]).
3839

@@ -47,6 +48,7 @@ and this project adheres to
4748
[#2498]: https://github.com/AdguardTeam/AdGuardHome/issues/2498
4849
[#2533]: https://github.com/AdguardTeam/AdGuardHome/issues/2533
4950
[#2541]: https://github.com/AdguardTeam/AdGuardHome/issues/2541
51+
[#2828]: https://github.com/AdguardTeam/AdGuardHome/issues/2828
5052
[#2835]: https://github.com/AdguardTeam/AdGuardHome/issues/2835
5153
[#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
5254

‎internal/aghnet/addr.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package aghnet
2+
3+
import (
4+
"fmt"
5+
"net"
6+
7+
"github.com/AdguardTeam/AdGuardHome/internal/agherr"
8+
)
9+
10+
// ValidateHardwareAddress returns an error if hwa is not a valid EUI-48,
11+
// EUI-64, or 20-octet InfiniBand link-layer address.
12+
func ValidateHardwareAddress(hwa net.HardwareAddr) (err error) {
13+
defer agherr.Annotate("validating hardware address %q: %w", &err, hwa)
14+
15+
switch l := len(hwa); l {
16+
case 0:
17+
return agherr.Error("address is empty")
18+
case 6, 8, 20:
19+
return nil
20+
default:
21+
return fmt.Errorf("bad len: %d", l)
22+
}
23+
}

‎internal/aghnet/addr_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
package aghnet
2+
3+
import (
4+
"net"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func TestValidateHardwareAddress(t *testing.T) {
12+
testCases := []struct {
13+
name string
14+
wantErrMsg string
15+
in net.HardwareAddr
16+
}{{
17+
name: "success_eui_48",
18+
wantErrMsg: "",
19+
in: net.HardwareAddr{0x00, 0x01, 0x02, 0x03, 0x04, 0x05},
20+
}, {
21+
name: "success_eui_64",
22+
wantErrMsg: "",
23+
in: net.HardwareAddr{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07},
24+
}, {
25+
name: "success_infiniband",
26+
wantErrMsg: "",
27+
in: net.HardwareAddr{
28+
0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
29+
0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
30+
0x10, 0x11, 0x12, 0x13,
31+
},
32+
}, {
33+
name: "error_nil",
34+
wantErrMsg: `validating hardware address "": address is empty`,
35+
in: nil,
36+
}, {
37+
name: "error_empty",
38+
wantErrMsg: `validating hardware address "": address is empty`,
39+
in: net.HardwareAddr{},
40+
}, {
41+
name: "error_bad",
42+
wantErrMsg: `validating hardware address "00:01:02:03": bad len: 4`,
43+
in: net.HardwareAddr{0x00, 0x01, 0x02, 0x03},
44+
}}
45+
46+
for _, tc := range testCases {
47+
t.Run(tc.name, func(t *testing.T) {
48+
err := ValidateHardwareAddress(tc.in)
49+
if tc.wantErrMsg == "" {
50+
assert.NoError(t, err)
51+
} else {
52+
require.Error(t, err)
53+
assert.Equal(t, tc.wantErrMsg, err.Error())
54+
}
55+
})
56+
}
57+
}

‎internal/dhcpd/routeradv.go‎

Lines changed: 49 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"sync/atomic"
88
"time"
99

10+
"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
1011
"github.com/AdguardTeam/golibs/log"
1112
"golang.org/x/net/icmp"
1213
"golang.org/x/net/ipv6"
@@ -36,6 +37,33 @@ type icmpv6RA struct {
3637
mtu uint32
3738
}
3839

40+
// hwAddrToLinkLayerAddr converts a hardware address into a form required by
41+
// RFC4861. That is, a byte slice of length divisible by 8.
42+
//
43+
// See https://tools.ietf.org/html/rfc4861#section-4.6.1.
44+
func hwAddrToLinkLayerAddr(hwa net.HardwareAddr) (lla []byte, err error) {
45+
err = aghnet.ValidateHardwareAddress(hwa)
46+
if err != nil {
47+
// Don't wrap the error, because it already contains enough
48+
// context.
49+
return nil, err
50+
}
51+
52+
if len(hwa) == 6 || len(hwa) == 8 {
53+
lla = make([]byte, 8)
54+
copy(lla, hwa)
55+
56+
return lla, nil
57+
}
58+
59+
// Assume that aghnet.ValidateHardwareAddress prevents lengths other
60+
// than 20 by now.
61+
lla = make([]byte, 24)
62+
copy(lla, hwa)
63+
64+
return lla, nil
65+
}
66+
3967
// Create an ICMPv6.RouterAdvertisement packet with all necessary options.
4068
//
4169
// ICMPv6:
@@ -63,15 +91,23 @@ type icmpv6RA struct {
6391
// Reserved[2]
6492
// MTU[4]
6593
// Option=Source link-layer address(1):
66-
// Link-Layer Address[6]
94+
// Link-Layer Address[8/24]
6795
// Option=Recursive DNS Server(25):
6896
// Type[1]
6997
// Length * 8bytes[1]
7098
// Reserved[2]
7199
// Lifetime[4]
72100
// Addresses of IPv6 Recursive DNS Servers[16]
73-
func createICMPv6RAPacket(params icmpv6RA) []byte {
74-
data := make([]byte, 88)
101+
func createICMPv6RAPacket(params icmpv6RA) (data []byte, err error) {
102+
var lla []byte
103+
lla, err = hwAddrToLinkLayerAddr(params.sourceLinkLayerAddress)
104+
if err != nil {
105+
return nil, fmt.Errorf("converting source link layer address: %w", err)
106+
}
107+
108+
// TODO(a.garipov): Don't use a magic constant here. Refactor the code
109+
// and make all constants named instead of all those comments..
110+
data = make([]byte, 82+len(lla))
75111
i := 0
76112

77113
// ICMPv6:
@@ -138,8 +174,9 @@ func createICMPv6RAPacket(params icmpv6RA) []byte {
138174
data[i] = 1 // Type
139175
data[i+1] = 1 // Length
140176
i += 2
141-
copy(data[i:], params.sourceLinkLayerAddress) // Link-Layer Address[6]
142-
i += 6
177+
178+
copy(data[i:], lla) // Link-Layer Address[8/24]
179+
i += len(lla)
143180

144181
// Option=Recursive DNS Server:
145182

@@ -152,11 +189,11 @@ func createICMPv6RAPacket(params icmpv6RA) []byte {
152189
i += 4
153190
copy(data[i:], params.recursiveDNSServer) // Addresses of IPv6 Recursive DNS Servers[16]
154191

155-
return data
192+
return data, nil
156193
}
157194

158195
// Init - initialize RA module
159-
func (ra *raCtx) Init() error {
196+
func (ra *raCtx) Init() (err error) {
160197
ra.stop.Store(0)
161198
ra.conn = nil
162199
if !(ra.raAllowSLAAC || ra.raSLAACOnly) {
@@ -177,9 +214,12 @@ func (ra *raCtx) Init() error {
177214
params.prefix = make([]byte, 16)
178215
copy(params.prefix, ra.prefixIPAddr[:8]) // /64
179216

180-
data := createICMPv6RAPacket(params)
217+
var data []byte
218+
data, err = createICMPv6RAPacket(params)
219+
if err != nil {
220+
return fmt.Errorf("creating packet: %w", err)
221+
}
181222

182-
var err error
183223
success := false
184224
ipAndScope := ra.ipAddr.String() + "%" + ra.ifaceName
185225
ra.conn, err = icmp.ListenPacket("ip6:ipv6-icmp", ipAndScope)

‎internal/dhcpd/routeradv_test.go‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,23 @@ import (
77
"github.com/stretchr/testify/assert"
88
)
99

10-
func TestRA(t *testing.T) {
11-
data := createICMPv6RAPacket(icmpv6RA{
10+
func TestCreateICMPv6RAPacket(t *testing.T) {
11+
wantData := []byte{
12+
0x86, 0x00, 0x00, 0x00, 0x40, 0x40, 0x07, 0x08,
13+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
14+
0x03, 0x04, 0x40, 0xc0, 0x00, 0x00, 0x0e, 0x10,
15+
0x00, 0x00, 0x0e, 0x10, 0x00, 0x00, 0x00, 0x00,
16+
0x12, 0x34, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
17+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
18+
0x05, 0x01, 0x00, 0x00, 0x00, 0x00, 0x05, 0xdc,
19+
0x01, 0x01, 0x0a, 0x00, 0x27, 0x00, 0x00, 0x00,
20+
0x00, 0x00, 0x19, 0x03, 0x00, 0x00, 0x00, 0x00,
21+
0x0e, 0x10, 0xfe, 0x80, 0x00, 0x00, 0x00, 0x00,
22+
0x00, 0x00, 0x08, 0x00, 0x27, 0xff, 0xfe, 0x00,
23+
0x00, 0x00,
24+
}
25+
26+
gotData, err := createICMPv6RAPacket(icmpv6RA{
1227
managedAddressConfiguration: false,
1328
otherConfiguration: true,
1429
mtu: 1500,
@@ -17,13 +32,7 @@ func TestRA(t *testing.T) {
1732
recursiveDNSServer: net.ParseIP("fe80::800:27ff:fe00:0"),
1833
sourceLinkLayerAddress: []byte{0x0a, 0x00, 0x27, 0x00, 0x00, 0x00},
1934
})
20-
dataCorrect := []byte{
21-
0x86, 0x00, 0x00, 0x00, 0x40, 0x40, 0x07, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
22-
0x03, 0x04, 0x40, 0xc0, 0x00, 0x00, 0x0e, 0x10, 0x00, 0x00, 0x0e, 0x10, 0x00, 0x00, 0x00, 0x00,
23-
0x12, 0x34, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
24-
0x05, 0x01, 0x00, 0x00, 0x00, 0x00, 0x05, 0xdc, 0x01, 0x01, 0x0a, 0x00, 0x27, 0x00, 0x00, 0x00,
25-
0x19, 0x03, 0x00, 0x00, 0x00, 0x00, 0x0e, 0x10, 0xfe, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
26-
0x08, 0x00, 0x27, 0xff, 0xfe, 0x00, 0x00, 0x00,
27-
}
28-
assert.Equal(t, dataCorrect, data)
35+
36+
assert.NoError(t, err)
37+
assert.Equal(t, wantData, gotData)
2938
}

‎internal/dhcpd/v4.go‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"time"
1111

1212
"github.com/AdguardTeam/AdGuardHome/internal/agherr"
13+
"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
1314
"github.com/AdguardTeam/golibs/log"
1415
"github.com/go-ping/ping"
1516
"github.com/insomniacslk/dhcp/dhcpv4"
@@ -289,8 +290,9 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) {
289290
return fmt.Errorf("invalid ip %q, only ipv4 is supported", l.IP)
290291
}
291292

292-
if len(l.HWAddr) != 6 {
293-
return fmt.Errorf("invalid mac %q, only EUI-48 is supported", l.HWAddr)
293+
err = aghnet.ValidateHardwareAddress(l.HWAddr)
294+
if err != nil {
295+
return fmt.Errorf("validating lease: %w", err)
294296
}
295297

296298
l.Expiry = time.Unix(leaseExpireStatic, 0)
@@ -330,17 +332,21 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) {
330332
return nil
331333
}
332334

333-
// RemoveStaticLease removes a static lease (thread-safe)
334-
func (s *v4Server) RemoveStaticLease(l Lease) error {
335+
// RemoveStaticLease removes a static lease. It is safe for concurrent use.
336+
func (s *v4Server) RemoveStaticLease(l Lease) (err error) {
337+
defer agherr.Annotate("dhcpv4: %w", &err)
338+
335339
if len(l.IP) != 4 {
336340
return fmt.Errorf("invalid IP")
337341
}
338-
if len(l.HWAddr) != 6 {
339-
return fmt.Errorf("invalid MAC")
342+
343+
err = aghnet.ValidateHardwareAddress(l.HWAddr)
344+
if err != nil {
345+
return fmt.Errorf("validating lease: %w", err)
340346
}
341347

342348
s.leasesLock.Lock()
343-
err := s.rmLease(l)
349+
err = s.rmLease(l)
344350
if err != nil {
345351
s.leasesLock.Unlock()
346352

@@ -688,8 +694,10 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4
688694
return
689695
}
690696

691-
if len(req.ClientHWAddr) != 6 {
692-
log.Debug("dhcpv4: Invalid ClientHWAddr")
697+
err = aghnet.ValidateHardwareAddress(req.ClientHWAddr)
698+
if err != nil {
699+
log.Error("dhcpv4: invalid ClientHWAddr: %s", err)
700+
693701
return
694702
}
695703

0 commit comments

Comments
 (0)