dhcp4d: Panic on truncated packet

[chlunde]

Found using go-fuzz

func TestTruncatedPacketMustNotPanic(t *testing.T) {
        handler, cleanup := testHandler(t)
        defer cleanup()

        var hardwareAddr = net.HardwareAddr{0x11, 0x22, 0x33, 0x44, 0x55, 0x66}

        p := request(net.IPv4zero, hardwareAddr)
        for i := 1; i < len(p)-1; i++ {
                truncated := dhcp4.Packet(p[0:i])
                handler.ServeDHCP(truncated, dhcp4.Discover, truncated.ParseOptions())
                // should not panic...
        }
}

I did not try to replicate this "on the wire", so it could be a false positive.

[stapelberg]

Thanks for pointing your fuzzer to router7! :)

I’ll check this out when I get a chance

[stapelberg]

So the test you posted calls ServeDHCP directly, but in practice we are using https://github.com/krolaw/dhcp4, which ensures that the packet has a minimum length: https://github.com/krolaw/dhcp4/blob/a50d88189771b462f903e77995cd0f4d186fbea7/server.go#L46-L48

When modifying your test to ensure a minimum length of 240, nothing panics anymore.

Let me know if I missed something or misunderstood something, but I think everything’s okay here.

[chlunde]

The initial issue I created was not reproducing the issue that go-fuzz found, it is not a truncated packet but rather a packet with a too small DHCP hardware address length field. For reference, here is a packet which reproduces the condition where the issue can happen. Michael explained to me that triggering the panic is not reliable because it depends on implementation details. The capacity of []byte("") which varies, as discussed here: https://github.com/golang/go/issues/18424#issuecomment-269084384

type fuzzConn struct {
        data []byte
}

func (*fuzzConn) LocalAddr() net.Addr                                { return nil }
func (*fuzzConn) Close() error                                       { return nil }
func (*fuzzConn) WriteTo(b []byte, addr net.Addr) (n int, err error) { return len(b), nil }
func (*fuzzConn) SetDeadline(t time.Time) error                      { return nil }
func (*fuzzConn) SetReadDeadline(t time.Time) error                  { return nil }
func (*fuzzConn) SetWriteDeadline(t time.Time) error                 { return nil }
func (f *fuzzConn) ReadFrom(buf []byte) (int, net.Addr, error) {
        if f.data == nil {
                return 0, nil, io.EOF
        }
        n := copy(buf, f.data)
        f.data = nil
        return n, &net.IPAddr{IP: net.IPv4zero, Zone: ""}, nil
}

func TestTruncatedPacketShouldNotPanic2(t *testing.T) {
        data := []byte{
                0x30, 0x30, 0x00, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
                0x35, 0x01, 0x01,
        }

        conn := &fuzzConn{data: data}
        handler, err := NewHandler(
                "/tmp/x", // TODO: see golden reference config in the tests for contents...
                &net.Interface{
                        HardwareAddr: net.HardwareAddr([]byte{0x11, 0x22, 0x33, 0x44, 0x55, 0x66}),
                },
                "lan0",
                conn,
        )
        if err != nil {
                panic(err)
        }
        dhcp4.Serve(conn, handler)
}