From 5e56eebf6ab85ca5fd0a0278c312674d921a3077 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Wed, 17 Mar 2021 18:47:54 +0300 Subject: [PATCH] dhcpd: imp static lease validation --- CHANGELOG.md | 3 + go.mod | 1 - go.sum | 2 - internal/dhcpd/bitset.go | 52 ++++++ internal/dhcpd/bitset_test.go | 90 ++++++++++ internal/dhcpd/db.go | 4 +- internal/dhcpd/dhcpd.go | 13 +- internal/dhcpd/dhcpd_test.go | 5 +- internal/dhcpd/iprange.go | 4 +- internal/dhcpd/iprange_test.go | 12 +- internal/dhcpd/server.go | 11 +- internal/dhcpd/v4.go | 289 +++++++++++++++++++++++---------- internal/dhcpd/v4_test.go | 7 +- internal/dhcpd/v6.go | 4 +- 14 files changed, 383 insertions(+), 114 deletions(-) create mode 100644 internal/dhcpd/bitset.go create mode 100644 internal/dhcpd/bitset_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 13688bbba26..995783ea000 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/go.mod b/go.mod index d31b9dfcd27..c6eb56a20a4 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 1d9bebfbc79..c26a26cd873 100644 --- a/go.sum +++ b/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= diff --git a/internal/dhcpd/bitset.go b/internal/dhcpd/bitset.go new file mode 100644 index 00000000000..e726319c445 --- /dev/null +++ b/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< now && !s.blacklisted(lease)) || - ((flags&LeasesStatic) != 0 && lease.Expiry.Unix() == leaseExpireStatic) { - result = append(result, *lease) + defer s.leasesLock.Unlock() + + now := time.Now() + for _, l := range s.leases { + if getDynamic && l.Expiry.After(now) && !s.isBlocklisted(l) { + res = append(res, *l) + } + + if getStatic && l.IsStatic() { + res = append(res, *l) } } - s.leasesLock.Unlock() - return result + return res } // FindMACbyIP - find a MAC address by IP address in the currently active DHCP leases func (s *v4Server) FindMACbyIP(ip net.IP) net.HardwareAddr { - now := time.Now().Unix() + now := time.Now() s.leasesLock.Lock() defer s.leasesLock.Unlock() @@ -102,12 +138,12 @@ func (s *v4Server) FindMACbyIP(ip net.IP) net.HardwareAddr { for _, l := range s.leases { if l.IP.Equal(ip4) { - unix := l.Expiry.Unix() - if unix > now || unix == leaseExpireStatic { + if l.Expiry.After(now) || l.IsStatic() { return l.HWAddr } } } + return nil } @@ -142,7 +178,7 @@ func (s *v4Server) rmLeaseByIndex(i int) { r := s.conf.ipRange offset, ok := r.offset(l.IP) if ok { - s.leasedOffsets.Clear(offset) + s.leasedOffsets.set(offset, false) } log.Debug("dhcpv4: removed lease %s (%s)", l.IP, l.HWAddr) @@ -150,12 +186,12 @@ func (s *v4Server) rmLeaseByIndex(i int) { // Remove a dynamic lease with the same properties // Return error if a static lease is found -func (s *v4Server) rmDynamicLease(lease Lease) error { +func (s *v4Server) rmDynamicLease(lease *Lease) (err error) { for i := 0; i < len(s.leases); i++ { l := s.leases[i] if bytes.Equal(l.HWAddr, lease.HWAddr) { - if l.Expiry.Unix() == leaseExpireStatic { + if l.IsStatic() { return fmt.Errorf("static lease already exists") } @@ -168,40 +204,70 @@ func (s *v4Server) rmDynamicLease(lease Lease) error { } if net.IP.Equal(l.IP, lease.IP) { - if l.Expiry.Unix() == leaseExpireStatic { + if l.IsStatic() { return fmt.Errorf("static lease already exists") } s.rmLeaseByIndex(i) } } + + return nil +} + +func (s *v4Server) addStaticLease(l *Lease) (err error) { + subnet := &net.IPNet{ + IP: s.conf.routerIP, + Mask: s.conf.subnetMask, + } + + if !subnet.Contains(l.IP) { + return fmt.Errorf("subnet %s does not contain the ip %q", subnet, l.IP) + } + + s.leases = append(s.leases, l) + + r := s.conf.ipRange + offset, ok := r.offset(l.IP) + if ok { + s.leasedOffsets.set(offset, true) + } + return nil } -// addLease adds a lease. -func (s *v4Server) addLease(l *Lease) { +func (s *v4Server) addDynamicLease(l *Lease) (err error) { r := s.conf.ipRange offset, ok := r.offset(l.IP) if !ok { - // TODO(a.garipov): Better error handling. - log.Debug("dhcpv4: lease %s (%s) out of range, not adding", l.IP, l.HWAddr) - - return + return fmt.Errorf("lease %s (%s) out of range, not adding", l.IP, l.HWAddr) } s.leases = append(s.leases, l) - s.leasedOffsets.Set(offset) + s.leasedOffsets.set(offset, true) - log.Debug("dhcpv4: added lease %s (%s)", l.IP, l.HWAddr) + return nil +} + +// addLease adds a dynamic or static lease. +func (s *v4Server) addLease(l *Lease) (err error) { + if l.IsStatic() { + return s.addStaticLease(l) + } + + return s.addDynamicLease(l) } // Remove a lease with the same properties func (s *v4Server) rmLease(lease Lease) error { + if len(s.leases) == 0 { + return nil + } + for i, l := range s.leases { if l.IP.Equal(lease.IP) { - if !bytes.Equal(l.HWAddr, lease.HWAddr) || - l.Hostname != lease.Hostname { - return fmt.Errorf("lease not found") + if !bytes.Equal(l.HWAddr, lease.HWAddr) || l.Hostname != lease.Hostname { + return fmt.Errorf("lease for ip %s is different: %+v", lease.IP, l) } s.rmLeaseByIndex(i) @@ -210,30 +276,55 @@ func (s *v4Server) rmLease(lease Lease) error { } } - return fmt.Errorf("lease not found") + return agherr.Error("lease not found") } -// AddStaticLease adds a static lease (thread-safe) -func (s *v4Server) AddStaticLease(lease Lease) error { - if len(lease.IP) != 4 { - return fmt.Errorf("invalid IP") +// AddStaticLease adds a static lease. It is safe for concurrent use. +func (s *v4Server) AddStaticLease(l Lease) (err error) { + defer agherr.Annotate("dhcpv4: %w", &err) + + if ip4 := l.IP.To4(); ip4 == nil { + return fmt.Errorf("invalid ip %q, only ipv4 is supported", l.IP) } - if len(lease.HWAddr) != 6 { - return fmt.Errorf("invalid MAC") + + if len(l.HWAddr) != 6 { + return fmt.Errorf("invalid mac %q, only EUI-48 is supported", l.HWAddr) } - lease.Expiry = time.Unix(leaseExpireStatic, 0) - s.leasesLock.Lock() - err := s.rmDynamicLease(lease) + l.Expiry = time.Unix(leaseExpireStatic, 0) + + // Perform the following actions in an anonymous function to make sure + // that the lock gets unlocked before the notification step. + func() { + s.leasesLock.Lock() + defer s.leasesLock.Unlock() + + err = s.rmDynamicLease(&l) + if err != nil { + err = fmt.Errorf( + "removing dynamic leases for %s (%s): %w", + l.IP, + l.HWAddr, + err, + ) + + return + } + + err = s.addLease(&l) + if err != nil { + err = fmt.Errorf("adding static lease for %s (%s): %w", l.IP, l.HWAddr, err) + + return + } + }() if err != nil { - s.leasesLock.Unlock() return err } - s.addLease(&lease) - s.conf.notify(LeaseChangedDBStore) - s.leasesLock.Unlock() + s.conf.notify(LeaseChangedDBStore) s.conf.notify(LeaseChangedAddedStatic) + return nil } @@ -250,12 +341,14 @@ func (s *v4Server) RemoveStaticLease(l Lease) error { err := s.rmLease(l) if err != nil { s.leasesLock.Unlock() + return err } - s.conf.notify(LeaseChangedDBStore) s.leasesLock.Unlock() + s.conf.notify(LeaseChangedDBStore) s.conf.notify(LeaseChangedRemovedStatic) + return nil } @@ -318,7 +411,7 @@ func (s *v4Server) nextIP() (ip net.IP) { return false } - return !s.leasedOffsets.Test(offset) + return !s.leasedOffsets.isSet(offset) }) return ip.To4() @@ -326,19 +419,19 @@ func (s *v4Server) nextIP() (ip net.IP) { // Find an expired lease and return its index or -1 func (s *v4Server) findExpiredLease() int { - now := time.Now().Unix() + now := time.Now() for i, lease := range s.leases { - if lease.Expiry.Unix() != leaseExpireStatic && - lease.Expiry.Unix() <= now { + if !lease.IsStatic() && lease.Expiry.Before(now) { return i } } + return -1 } // reserveLease reserves a lease for a client by its MAC-address. It returns // nil if it couldn't allocate a new lease. -func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease) { +func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease, err error) { l = &Lease{ HWAddr: make([]byte, len(mac)), } @@ -349,17 +442,20 @@ func (s *v4Server) reserveLease(mac net.HardwareAddr) (l *Lease) { if l.IP == nil { i := s.findExpiredLease() if i < 0 { - return nil + return nil, nil } copy(s.leases[i].HWAddr, mac) - return s.leases[i] + return s.leases[i], nil } - s.addLease(l) + err = s.addLease(l) + if err != nil { + return nil, err + } - return l + return l, nil } func (s *v4Server) commitLease(l *Lease) { @@ -373,46 +469,55 @@ func (s *v4Server) commitLease(l *Lease) { } // Process Discover request and return lease -func (s *v4Server) processDiscover(req, resp *dhcpv4.DHCPv4) *Lease { +func (s *v4Server) processDiscover(req, resp *dhcpv4.DHCPv4) (l *Lease, err error) { mac := req.ClientHWAddr s.leasesLock.Lock() defer s.leasesLock.Unlock() - lease := s.findLease(mac) - if lease == nil { + // TODO(a.garipov): Refactor this mess. + l = s.findLease(mac) + if l == nil { toStore := false - for lease == nil { - lease = s.reserveLease(mac) - if lease == nil { - log.Debug("dhcpv4: No more IP addresses") + for l == nil { + l, err = s.reserveLease(mac) + if err != nil { + return nil, fmt.Errorf("reserving a lease: %w", err) + } + + if l == nil { + log.Debug("dhcpv4: no more ip addresses") if toStore { s.conf.notify(LeaseChangedDBStore) } - return nil + + // TODO(a.garipov): Return a special error? + return nil, nil } toStore = true - if !s.addrAvailable(lease.IP) { - s.blocklistLease(lease) - lease = nil + if !s.addrAvailable(l.IP) { + s.blocklistLease(l) + l = nil + continue } + break } s.conf.notify(LeaseChangedDBStore) - } else { reqIP := req.RequestedIPAddress() - if len(reqIP) != 0 && !reqIP.Equal(lease.IP) { - log.Debug("dhcpv4: different RequestedIP: %v != %v", reqIP, lease.IP) + if len(reqIP) != 0 && !reqIP.Equal(l.IP) { + log.Debug("dhcpv4: different RequestedIP: %s != %s", reqIP, l.IP) } } resp.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeOffer)) - return lease + + return l, nil } type optFQDN struct { @@ -490,7 +595,7 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (*Lease, bool) { return nil, true } - if lease.Expiry.Unix() != leaseExpireStatic { + if !lease.IsStatic() { lease.Hostname = req.HostName() s.commitLease(lease) } else if len(lease.Hostname) != 0 { @@ -515,22 +620,27 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (*Lease, bool) { // Return 0: error; reply with Nak // Return -1: error; don't reply func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int { - var lease *Lease + var err error resp.UpdateOption(dhcpv4.OptServerIdentifier(s.conf.dnsIPAddrs[0])) + var l *Lease switch req.MessageType() { - case dhcpv4.MessageTypeDiscover: - lease = s.processDiscover(req, resp) - if lease == nil { + l, err = s.processDiscover(req, resp) + if err != nil { + log.Error("dhcpv4: processing discover: %s", err) + return 0 } + if l == nil { + return 0 + } case dhcpv4.MessageTypeRequest: var toReply bool - lease, toReply = s.processRequest(req, resp) - if lease == nil { + l, toReply = s.processRequest(req, resp) + if l == nil { if toReply { return 0 } @@ -539,7 +649,7 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int { } resp.YourIPAddr = make([]byte, 4) - copy(resp.YourIPAddr, lease.IP) + copy(resp.YourIPAddr, l.IP) resp.UpdateOption(dhcpv4.OptIPAddressLeaseTime(s.conf.leaseTime)) resp.UpdateOption(dhcpv4.OptRouter(s.conf.routerIP)) @@ -549,6 +659,7 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int { for _, opt := range s.conf.options { resp.Options[opt.code] = opt.data } + return 1 } @@ -683,7 +794,7 @@ func v4Create(conf V4ServerConf) (srv DHCPServer, err error) { return s, fmt.Errorf("dhcpv4: %w", err) } - s.leasedOffsets = &bitset.BitSet{} + s.leasedOffsets = newBitSet() if conf.LeaseDuration == 0 { s.conf.leaseTime = time.Hour * 24 diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index 9c5897423cc..9afe5fbb8a4 100644 --- a/internal/dhcpd/v4_test.go +++ b/internal/dhcpd/v4_test.go @@ -40,7 +40,7 @@ func TestV4_AddRemove_static(t *testing.T) { require.Len(t, ls, 1) assert.True(t, l.IP.Equal(ls[0].IP)) assert.Equal(t, l.HWAddr, ls[0].HWAddr) - assert.EqualValues(t, leaseExpireStatic, ls[0].Expiry.Unix()) + assert.True(t, ls[0].IsStatic()) // Try to remove static lease. assert.NotNil(t, s.RemoveStaticLease(Lease{ @@ -77,7 +77,8 @@ func TestV4_AddReplace(t *testing.T) { }} for i := range dynLeases { - s.addLease(&dynLeases[i]) + err = s.addLease(&dynLeases[i]) + require.Nil(t, err) } stLeases := []Lease{{ @@ -98,7 +99,7 @@ func TestV4_AddReplace(t *testing.T) { for i, l := range ls { assert.True(t, stLeases[i].IP.Equal(l.IP)) assert.Equal(t, stLeases[i].HWAddr, l.HWAddr) - assert.EqualValues(t, leaseExpireStatic, l.Expiry.Unix()) + assert.True(t, l.IsStatic()) } } diff --git a/internal/dhcpd/v6.go b/internal/dhcpd/v6.go index b47387e7387..aff6d3e335e 100644 --- a/internal/dhcpd/v6.go +++ b/internal/dhcpd/v6.go @@ -92,8 +92,8 @@ func (s *v6Server) GetLeases(flags int) []Lease { return result } -// GetLeasesRef - get leases -func (s *v6Server) GetLeasesRef() []*Lease { +// getLeasesRef returns the actual leases slice. For internal use only. +func (s *v6Server) getLeasesRef() []*Lease { return s.leases }