From 53209bc42c2401e017e5b727fb74272953a8cd0e Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 7 Sep 2022 14:34:30 +0300 Subject: [PATCH] Pull request: 4337 increase msg size Merge in DNS/adguard-home from 4337-dhcp-msg-len to master Updates #4337. Squashed commit of the following: commit 55e53c1fadd4ccb2a8b94117afff82e9a5d2734b Merge: f37070ea da1ae338 Author: Eugene Burkov Date: Wed Sep 7 14:14:21 2022 +0300 Merge branch 'master' into 4337-dhcp-msg-len commit f37070ea0f3a7ff8efcbbafd36001f78d9b082b5 Author: Eugene Burkov Date: Wed Sep 7 13:15:25 2022 +0300 dhcpd: imp ether pkt building commit fa43a0bcc24d4ca5e9193899dbba8495f3de5df9 Author: Eugene Burkov Date: Tue Sep 6 18:55:07 2022 +0300 dhcpd: incr msg size --- CHANGELOG.md | 2 ++ internal/dhcpd/conn_unix.go | 44 +++++++++++++++----------------- internal/dhcpd/conn_unix_test.go | 11 ++++---- internal/dhcpd/v4.go | 24 +++++++++++++++-- 4 files changed, 50 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d76e481..bb2e6cfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,8 @@ and this project adheres to ### Fixed +- The length of the DHCP server's response is now at least 576 bytes as per RFC + 2131 recommendation ([#4337]). - Dynamic leases created with empty hostnames ([#4745]). - Unnecessary logging of non-critical statistics errors ([#4850]). diff --git a/internal/dhcpd/conn_unix.go b/internal/dhcpd/conn_unix.go index 1ed2105a..01c063d1 100644 --- a/internal/dhcpd/conn_unix.go +++ b/internal/dhcpd/conn_unix.go @@ -83,7 +83,7 @@ func (s *v4Server) newDHCPConn(iface *net.Interface) (c net.PacketConn, err erro // wrapErrs is a helper to wrap the errors from two independent underlying // connections. -func (c *dhcpConn) wrapErrs(action string, udpConnErr, rawConnErr error) (err error) { +func (*dhcpConn) wrapErrs(action string, udpConnErr, rawConnErr error) (err error) { switch { case udpConnErr != nil && rawConnErr != nil: return errors.List(fmt.Sprintf("%s both connections", action), udpConnErr, rawConnErr) @@ -129,7 +129,7 @@ func (c *dhcpConn) WriteTo(p []byte, addr net.Addr) (n int, err error) { // connection. return c.udpConn.WriteTo(p, addr) default: - return 0, fmt.Errorf("peer is of unexpected type %T", addr) + return 0, fmt.Errorf("addr has an unexpected type %T", addr) } } @@ -188,32 +188,20 @@ func (c *dhcpConn) SetWriteDeadline(t time.Time) error { ) } -// ipv4DefaultTTL is the default Time to Live value as recommended by -// RFC-1700 (https://datatracker.ietf.org/doc/html/rfc1700) in seconds. +// ipv4DefaultTTL is the default Time to Live value in seconds as recommended by +// RFC-1700. +// +// See https://datatracker.ietf.org/doc/html/rfc1700. const ipv4DefaultTTL = 64 -// errInvalidPktDHCP is returned when the provided payload is not a valid DHCP -// packet. -const errInvalidPktDHCP errors.Error = "packet is not a valid dhcp packet" - -// buildEtherPkt wraps the payload with IPv4, UDP and Ethernet frames. The -// payload is expected to be an encoded DHCP packet. +// buildEtherPkt wraps the payload with IPv4, UDP and Ethernet frames. +// Validation of the payload is a caller's responsibility. func (c *dhcpConn) buildEtherPkt(payload []byte, peer *dhcpUnicastAddr) (pkt []byte, err error) { - dhcpLayer := gopacket.NewPacket(payload, layers.LayerTypeDHCPv4, gopacket.DecodeOptions{ - NoCopy: true, - }).Layer(layers.LayerTypeDHCPv4) - - // Check if the decoding succeeded and the resulting layer doesn't - // contain any errors. It should guarantee panic-safe converting of the - // layer into gopacket.SerializableLayer. - if dhcpLayer == nil || dhcpLayer.LayerType() != layers.LayerTypeDHCPv4 { - return nil, errInvalidPktDHCP - } - udpLayer := &layers.UDP{ SrcPort: dhcpv4.ServerPort, DstPort: dhcpv4.ClientPort, } + ipv4Layer := &layers.IPv4{ Version: uint8(layers.IPProtocolIPv4), Flags: layers.IPv4DontFragment, @@ -226,6 +214,7 @@ func (c *dhcpConn) buildEtherPkt(payload []byte, peer *dhcpUnicastAddr) (pkt []b // Ignore the error since it's only returned for invalid network layer's // type. _ = udpLayer.SetNetworkLayerForChecksum(ipv4Layer) + ethLayer := &layers.Ethernet{ SrcMAC: c.srcMAC, DstMAC: peer.HardwareAddr, @@ -233,10 +222,19 @@ func (c *dhcpConn) buildEtherPkt(payload []byte, peer *dhcpUnicastAddr) (pkt []b } buf := gopacket.NewSerializeBuffer() - err = gopacket.SerializeLayers(buf, gopacket.SerializeOptions{ + setts := gopacket.SerializeOptions{ FixLengths: true, ComputeChecksums: true, - }, ethLayer, ipv4Layer, udpLayer, dhcpLayer.(gopacket.SerializableLayer)) + } + + err = gopacket.SerializeLayers( + buf, + setts, + ethLayer, + ipv4Layer, + udpLayer, + gopacket.Payload(payload), + ) if err != nil { return nil, fmt.Errorf("serializing layers: %w", err) } diff --git a/internal/dhcpd/conn_unix_test.go b/internal/dhcpd/conn_unix_test.go index 84020acd..0fdbfec0 100644 --- a/internal/dhcpd/conn_unix_test.go +++ b/internal/dhcpd/conn_unix_test.go @@ -47,7 +47,7 @@ func TestDHCPConn_WriteTo_common(t *testing.T) { n, err := conn.WriteTo(nil, &unexpectedAddrType{}) require.Error(t, err) - testutil.AssertErrorMsg(t, "peer is of unexpected type *dhcpd.unexpectedAddrType", err) + testutil.AssertErrorMsg(t, "addr has an unexpected type *dhcpd.unexpectedAddrType", err) assert.Zero(t, n) }) } @@ -91,14 +91,13 @@ func TestBuildEtherPkt(t *testing.T) { } }) - t.Run("non-serializable", func(t *testing.T) { + t.Run("bad_payload", func(t *testing.T) { // Create an invalid DHCP packet. invalidPayload := []byte{1, 2, 3, 4} - pkt, err := conn.buildEtherPkt(invalidPayload, nil) - require.Error(t, err) + pkt, err := conn.buildEtherPkt(invalidPayload, peer) + require.NoError(t, err) - assert.ErrorIs(t, err, errInvalidPktDHCP) - assert.Empty(t, pkt) + assert.NotEmpty(t, pkt) }) t.Run("serializing_error", func(t *testing.T) { diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index 31c4d0b2..4d6c817b 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -1086,6 +1086,12 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4 s.send(peer, conn, req, resp) } +// minDHCPMsgSize is the minimum length of the encoded DHCP message in bytes +// according to RFC-2131. +// +// See https://datatracker.ietf.org/doc/html/rfc2131#section-2. +const minDHCPMsgSize = 576 + // send writes resp for peer to conn considering the req's parameters according // to RFC-2131. // @@ -1126,8 +1132,22 @@ func (s *v4Server) send(peer net.Addr, conn net.PacketConn, req, resp *dhcpv4.DH // Go on since peer is already set to broadcast. } - log.Debug("dhcpv4: sending to %s: %s", peer, resp.Summary()) - if _, err := conn.WriteTo(resp.ToBytes(), peer); err != nil { + pktData := resp.ToBytes() + pktLen := len(pktData) + if pktLen < minDHCPMsgSize { + // Expand the packet to match the minimum DHCP message length. Although + // the dhpcv4 package deals with the BOOTP's lower packet length + // constraint, it seems some clients expecting the length being at least + // 576 bytes as per RFC 2131 (and an obsolete RFC 1533). + // + // See https://github.com/AdguardTeam/AdGuardHome/issues/4337. + pktData = append(pktData, make([]byte, minDHCPMsgSize-pktLen)...) + } + + log.Debug("dhcpv4: sending %d bytes to %s: %s", len(pktData), peer, resp.Summary()) + + _, err := conn.WriteTo(pktData, peer) + if err != nil { log.Error("dhcpv4: conn.Write to %s failed: %s", peer, err) } }