Closed Boneyan closed 11 months ago
Please sign the commit: https://github.com/insomniacslk/dhcp/pull/507/checks?check_run_id=15096874830
Example:
Can you share some context on when you faced this out of bounds read? Didn't the packet have the full IP header?
Patch coverage: 35.00%
and project coverage change: +72.77%
:tada:
Comparison is base (
5648422
) 0.00% compared to head (5771e16
) 72.77%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
App made dhcp requests in a mobile network and seems like received packet that caused panic. I think similar issue is mentioned in #468.
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.
Maybe we should add the IsValid
method:
https://github.com/google/gvisor/blob/master/pkg/tcpip/header/ipv4.go#L483
Seems like current version of PR doesn't work (it can't receive response packet after offer)
Seems like current version of PR doesn't work (it can't receive response packet after offer)
Do you know why?
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
I ran more tests and this version is actually working, previous test was misconducted
PR fixing bug: