Fix out of bounds read

[Boneyan]

PR fixing bug:

panic: runtime error: index out of range [9] with length 0

goroutine 2182 [running]:
github.com/insomniacslk/dhcp/dhcpv4/nclient4.ipv4.protocol(...)
	/go/src/.../vendor/github.com/insomniacslk/dhcp/dhcpv4/nclient4/ipv4.go:108
github.com/insomniacslk/dhcp/dhcpv4/nclient4.ipv4.transportProtocol(...)
	/go/src/.../vendor/github.com/insomniacslk/dhcp/dhcpv4/nclient4/ipv4.go:124
github.com/insomniacslk/dhcp/dhcpv4/nclient4.(*BroadcastRawUDPConn).ReadFrom(0x400048a0f0, {0x40005f2600, 0x5dc, 0x1?})
	/go/src/.../vendor/github.com/insomniacslk/dhcp/dhcpv4/nclient4/conn_unix.go:115 +0x370
github.com/insomniacslk/dhcp/dhcpv4/nclient4.(*Client).receiveLoop(0x40000163f0)
	/go/src/.../vendor/github.com/insomniacslk/dhcp/dhcpv4/nclient4/client.go:262 +0x90
created by github.com/insomniacslk/dhcp/dhcpv4/nclient4.new
	/go/src/.../vendor/github.com/insomniacslk/dhcp/dhcpv4/nclient4/client.go:223 +0x3a4

[pmazzini]

Please sign the commit: https://github.com/insomniacslk/dhcp/pull/507/checks?check_run_id=15096874830

Example:

• In your local branch, run: git rebase HEAD~4 --signoff
• Force push your changes to overwrite the branch: git push --force-with-lease origin fix-out-of-bounds-read

[pmazzini]

Can you share some context on when you faced this out of bounds read? Didn't the packet have the full IP header?

[codecov]

Codecov Report

Patch coverage: 35.00% and project coverage change: +72.77% 🎉

Comparison is base (5648422) 0.00% compared to head (5771e16) 72.77%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #507       +/-   ##
===========================================
+ Coverage        0   72.77%   +72.77%     
===========================================
  Files           0       91       +91     
  Lines           0     5777     +5777     
===========================================
+ Hits            0     4204     +4204     
- Misses          0     1398     +1398     
- Partials        0      175      +175     

Files Changed	Coverage Δ
dhcpv4/nclient4/conn_unix.go	63.51% <0.00%> (ø)
dhcpv4/nclient4/ipv4.go	87.40% <36.84%> (ø)

... and 89 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Boneyan]

App made dhcp requests in a mobile network and seems like received packet that caused panic. I think similar issue is mentioned in #468.

[pmazzini]

From https://github.com/insomniacslk/dhcp/blob/master/dhcpv4/nclient4/ipv4.go#L81

ipv4 represents an ipv4 header stored in a byte array.
Most of the methods of IPv4 access to the underlying slice without
checking the boundaries and could panic because of 'index out of range'.
Always call IsValid() to validate an instance of IPv4 before using other methods.

[pmazzini]

Maybe we should add the IsValid method:

https://github.com/google/gvisor/blob/master/pkg/tcpip/header/ipv4.go#L483

[Boneyan]

Seems like current version of PR doesn't work (it can't receive response packet after offer)

[pmazzini]

Seems like current version of PR doesn't work (it can't receive response packet after offer)

Do you know why?

[Boneyan]

I had such issue in old version of this PR when it was smth like

if headerLength < protocol {continue}  
//do udpheader check

and then I removed continue

if headerLength > protocol {
    //do udpheader check
}

and it started working. So it has something to do with continue

[Boneyan]

I ran more tests and this version is actually working, previous test was misconducted