dhcpd: imp static lease validation

CHANGELOG.md

@@ -20,6 +20,8 @@ and this project adheres to
 
 ### Changed
 
+- Stricter validation of the IP addresses of static leases in the DHCP server
+  with regards to the netmask ([#2838]).
 - Stricter validation of `$dnsrewrite` filter modifier parameters ([#2498]).
 - New, more correct versioning scheme ([#2412]).
 
@@ -42,6 +44,7 @@ and this project adheres to
 [#2533]: https://github.com/AdguardTeam/AdGuardHome/issues/2533
 [#2541]: https://github.com/AdguardTeam/AdGuardHome/issues/2541
 [#2835]: https://github.com/AdguardTeam/AdGuardHome/issues/2835
+[#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
 
 
 

go.mod

@@ -32,7 +32,6 @@ require (
 	github.com/stretchr/testify v1.6.1
 	github.com/ti-mo/netfilter v0.4.0
 	github.com/u-root/u-root v7.0.0+incompatible
-	github.com/willf/bitset v1.1.11
 	go.etcd.io/bbolt v1.3.5
 	golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83
 	golang.org/x/net v0.0.0-20210226172049-e18ecbb05110

go.sum

@@ -425,8 +425,6 @@ github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGr
 github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
 github.com/viant/assertly v0.4.8/go.mod h1:aGifi++jvCrUaklKEKT0BU95igDNaqkvz+49uaYMPRU=
 github.com/viant/toolbox v0.24.0/go.mod h1:OxMCG57V0PXuIP2HNQrtJf2CjqdmbrOx5EkMILuUhzM=
-github.com/willf/bitset v1.1.11 h1:N7Z7E9UvjW+sGsEl7k/SJrvY2reP1A07MrGuCjIOjRE=
-github.com/willf/bitset v1.1.11/go.mod h1:83CECat5yLh5zVOf4P1ErAgKA5UDvKtgyUABdr3+MjI=
 github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU=
 github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:aYKd//L2LvnjZzWKhF00oedf4jCCReLcmhLdhm1A27Q=
 go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=

internal/dhcpd/bitset.go

@@ -0,0 +1,52 @@
+package dhcpd
+
+const bitsPerWord = 64
+
+// bitSet is a sparse bitSet.  A nil *bitSet is an empty bitSet.
+type bitSet struct {
+	words map[uint64]uint64
+}
+
+// newBitSet returns a new bitset.
+func newBitSet() (s *bitSet) {
+	return &bitSet{
+		words: map[uint64]uint64{},
+	}
+}
+
+// isSet returns true if the bit n is set.
+func (s *bitSet) isSet(n uint64) (ok bool) {
+	if s == nil {
+		return false
+	}
+
+	wordIdx := n / bitsPerWord
+	bitIdx := n % bitsPerWord
+
+	var word uint64
+	word, ok = s.words[wordIdx]
+	if !ok {
+		return false
+	}
+
+	return word&(1<<bitIdx) != 0
+}
+
+// set sets or unsets a bit.
+func (s *bitSet) set(n uint64, ok bool) {
+	if s == nil {
+		return
+	}
+
+	wordIdx := n / bitsPerWord
+	bitIdx := n % bitsPerWord
+
+	word := s.words[wordIdx]
+	if ok {
+		word |= 1 << bitIdx
+	} else {
+		word &^= 1 << bitIdx
+	}
+
+	s.words[wordIdx] = word
+}

internal/dhcpd/bitset_test.go

@@ -0,0 +1,90 @@
+package dhcpd
+
+import (
+	"math"
+	"testing"
+	"testing/quick"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBitSet(t *testing.T) {
+	t.Run("nil", func(t *testing.T) {
+		var s *bitSet
+
+		ok := s.isSet(0)
+		assert.False(t, ok)
+
+		assert.NotPanics(t, func() {
+			s.set(0, true)
+		})
+
+		ok = s.isSet(0)
+		assert.False(t, ok)
+
+		assert.NotPanics(t, func() {
+			s.set(0, false)
+		})
+
+		ok = s.isSet(0)
+		assert.False(t, ok)
+	})
+
+	t.Run("non_nil", func(t *testing.T) {
+		s := newBitSet()
+
+		ok := s.isSet(0)
+		assert.False(t, ok)
+
+		s.set(0, true)
+
+		ok = s.isSet(0)
+		assert.True(t, ok)
+
+		s.set(0, false)
+
+		ok = s.isSet(0)
+		assert.False(t, ok)
+	})
+
+	t.Run("non_nil_long", func(t *testing.T) {
+		s := newBitSet()
+
+		s.set(0, true)
+		s.set(math.MaxUint64, true)
+		assert.Len(t, s.words, 2)
+
+		ok := s.isSet(0)
+		assert.True(t, ok)
+
+		ok = s.isSet(math.MaxUint64)
+		assert.True(t, ok)
+	})
+
+	t.Run("quick", func(t *testing.T) {
+		m := map[uint64]struct{}{}
+		s := newBitSet()
+
+		mapFunc := func(setNew, checkOld, delOld uint64) (ok bool) {
+			m[setNew] = struct{}{}
+			delete(m, delOld)
+			_, ok = m[checkOld]
+
+			return ok
+		}
+
+		setFunc := func(setNew, checkOld, delOld uint64) (ok bool) {
+			s.set(setNew, true)
+			s.set(delOld, false)
+			ok = s.isSet(checkOld)
+
+			return ok
+		}
+
+		err := quick.CheckEqual(mapFunc, setFunc, &quick.Config{
+			MaxCount:      10_000,
+			MaxCountScale: 10,
+		})
+		assert.NoError(t, err)
+	})
+}

internal/dhcpd/db.go

@@ -128,7 +128,7 @@ func normalizeLeases(staticLeases, dynLeases []*Lease) []*Lease {
 func (s *Server) dbStore() {
 	var leases []leaseJSON
 
-	leases4 := s.srv4.GetLeasesRef()
+	leases4 := s.srv4.getLeasesRef()
 	for _, l := range leases4 {
 		if l.Expiry.Unix() == 0 {
 			continue
@@ -143,7 +143,7 @@ func (s *Server) dbStore() {
 	}
 
 	if s.srv6 != nil {
-		leases6 := s.srv6.GetLeasesRef()
+		leases6 := s.srv6.getLeasesRef()
 		for _, l := range leases6 {
 			if l.Expiry.Unix() == 0 {
 				continue

internal/dhcpd/dhcpd.go

@@ -36,16 +36,23 @@ type Lease struct {
 	Expiry time.Time `json:"expires"`
 }
 
+// IsStatic returns true if the lease is static.
+//
+// TODO(a.garipov): Just make it a boolean field.
+func (l *Lease) IsStatic() (ok bool) {
+	return l != nil && l.Expiry.Unix() == leaseExpireStatic
+}
+
 // MarshalJSON implements the json.Marshaler interface for *Lease.
 func (l *Lease) MarshalJSON() ([]byte, error) {
 	var expiryStr string
-	if expiry := l.Expiry; expiry.Unix() != leaseExpireStatic {
+	if !l.IsStatic() {
 		// The front-end is waiting for RFC 3999 format of the time
 		// value.  It also shouldn't got an Expiry field for static
 		// leases.
 		//
 		// See https://github.com/AdguardTeam/AdGuardHome/issues/2692.
-		expiryStr = expiry.Format(time.RFC3339)
+		expiryStr = l.Expiry.Format(time.RFC3339)
 	}
 
 	type lease Lease
@@ -203,6 +210,7 @@ func Create(conf ServerConfig) *Server {
 func (s *Server) onNotify(flags uint32) {
 	if flags == LeaseChangedDBStore {
 		s.dbStore()
+
 		return
 	}
 
@@ -218,6 +226,7 @@ func (s *Server) notify(flags int) {
 	if len(s.onLeaseChanged) == 0 {
 		return
 	}
+
 	for _, f := range s.onLeaseChanged {
 		f(flags)
 	}

internal/dhcpd/dhcpd_test.go

@@ -54,7 +54,8 @@ func TestDB(t *testing.T) {
 	srv4, ok := s.srv4.(*v4Server)
 	require.True(t, ok)
 
-	srv4.addLease(&leases[0])
+	err = srv4.addLease(&leases[0])
+	require.Nil(t, err)
 	require.Nil(t, s.srv4.AddStaticLease(leases[1]))
 
 	s.dbStore()
@@ -69,7 +70,7 @@ func TestDB(t *testing.T) {
 
 	assert.Equal(t, leases[1].HWAddr, ll[0].HWAddr)
 	assert.Equal(t, leases[1].IP, ll[0].IP)
-	assert.EqualValues(t, leaseExpireStatic, ll[0].Expiry.Unix())
+	assert.True(t, ll[0].IsStatic())
 
 	assert.Equal(t, leases[0].HWAddr, ll[1].HWAddr)
 	assert.Equal(t, leases[0].IP, ll[1].IP)

internal/dhcpd/iprange.go

@@ -93,7 +93,7 @@ func (r *ipRange) find(p ipPredicate) (ip net.IP) {
 
 // offset returns the offset of ip from the beginning of r.  It returns 0 and
 // false if ip is not in r.
-func (r *ipRange) offset(ip net.IP) (offset uint, ok bool) {
+func (r *ipRange) offset(ip net.IP) (offset uint64, ok bool) {
 	if r == nil {
 		return 0, false
 	}
@@ -108,5 +108,5 @@ func (r *ipRange) offset(ip net.IP) (offset uint, ok bool) {
 
 	// Assume that the range was checked against maxRangeLen during
 	// construction.
-	return uint(offsetInt.Uint64()), true
+	return offsetInt.Uint64(), true
 }

internal/dhcpd/iprange_test.go

@@ -66,10 +66,10 @@ func TestNewIPRange(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			r, err := newIPRange(tc.start, tc.end)
 			if tc.wantErrMsg == "" {
-				assert.Nil(t, err)
+				assert.NoError(t, err)
 				assert.NotNil(t, r)
 			} else {
-				require.NotNil(t, err)
+				require.Error(t, err)
 				assert.Equal(t, tc.wantErrMsg, err.Error())
 			}
 		})
@@ -79,7 +79,7 @@ func TestNewIPRange(t *testing.T) {
 func TestIPRange_Contains(t *testing.T) {
 	start, end := net.IP{0, 0, 0, 1}, net.IP{0, 0, 0, 3}
 	r, err := newIPRange(start, end)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	assert.True(t, r.contains(start))
 	assert.True(t, r.contains(net.IP{0, 0, 0, 2}))
@@ -92,7 +92,7 @@ func TestIPRange_Contains(t *testing.T) {
 func TestIPRange_Find(t *testing.T) {
 	start, end := net.IP{0, 0, 0, 1}, net.IP{0, 0, 0, 5}
 	r, err := newIPRange(start, end)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	want := net.IPv4(0, 0, 0, 2)
 	got := r.find(func(ip net.IP) (ok bool) {
@@ -110,12 +110,12 @@ func TestIPRange_Find(t *testing.T) {
 func TestIPRange_Offset(t *testing.T) {
 	start, end := net.IP{0, 0, 0, 1}, net.IP{0, 0, 0, 5}
 	r, err := newIPRange(start, end)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	testCases := []struct {
 		name       string
 		in         net.IP
-		wantOffset uint
+		wantOffset uint64
 		wantOK     bool
 	}{{
 		name:       "in",

internal/dhcpd/server.go

@@ -11,8 +11,6 @@ type DHCPServer interface {
 	ResetLeases(leases []*Lease)
 	// GetLeases - get leases
 	GetLeases(flags int) []Lease
-	// GetLeasesRef - get reference to leases array
-	GetLeasesRef() []*Lease
 	// AddStaticLease - add a static lease
 	AddStaticLease(lease Lease) error
 	// RemoveStaticLease - remove a static lease
@@ -29,6 +27,8 @@ type DHCPServer interface {
 	Start() error
 	// Stop - stop server
 	Stop()
+
+	getLeasesRef() []*Lease
 }
 
 // V4ServerConf - server configuration
@@ -68,7 +68,12 @@ type V4ServerConf struct {
 	subnetMask net.IPMask    // value for Option SubnetMask
 	options    []dhcpOption
 
-	// Server calls this function when leases data changes
+	// notify is a way to signal to other components that leases have
+	// change.  notify must be called outside of locked sections, since the
+	// clients might want to get the new data.
+	//
+	// TODO(a.garipov): This is utter madness and must be refactored.  It
+	// just begs for deadlock bugs and other nastiness.
 	notify func(uint32)
 }