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 <E.Burkov@AdGuard.COM>
Date:   Wed Sep 7 14:14:21 2022 +0300

    Merge branch 'master' into 4337-dhcp-msg-len

commit f37070ea0f3a7ff8efcbbafd36001f78d9b082b5
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Sep 7 13:15:25 2022 +0300

    dhcpd: imp ether pkt building

commit fa43a0bcc24d4ca5e9193899dbba8495f3de5df9
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Sep 6 18:55:07 2022 +0300

    dhcpd: incr msg size

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]).
 

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)
 	}

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) {

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)
 	}
 }