Catch OverflowError

[guedou]

Fixes #4527.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.58%. Comparing base (1464fa9) to head (dee0c70).

Files with missing lines	Patch %	Lines
scapy/utils.py	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4529      +/-   ##
==========================================
- Coverage   81.59%   81.58%   -0.01%     
==========================================
  Files         356      356              
  Lines       85287    85292       +5     
==========================================
- Hits        69592    69588       -4     
- Misses      15695    15704       +9     

Files with missing lines	Coverage Δ
scapy/utils.py	73.33% <50.00%> (-0.09%)	⬇️

... and 3 files with indirect coverage changes

[guedou]

My understanding is that caplen is a 32bits unsigned int, while the read parameter is a 32bits signed int. Hence, high values don’t fit.Sent from my iPhoneOn 7 Sep 2024, at 21:20, Nils Weiss ***@***.***> wrote: @polybassa approved this pull request. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[evverx]

As far as I can see in this particular case the length of the packet is 3402287818 and it's much more than sys.maxsize on i386 (2147483647). If size in self.f.read(caplen)[:size] is reasonable it should be possible to call read several times with some reasonable value to get the first bytes and skip the rest without triggering the overflow.

(Then again I'm not sure how important that is)

[evverx]

I took a look at what Wireshark does. Looks like it doesn't read more than 262144 bytes by default:
https://github.com/wireshark/wireshark/blob/4a2965cad39332f47b472fb873817ed021bffccb/wiretap/wtap.h#L363-L367

 * We don't want to write out files that specify a maximum packet size
 * greater than 262144 if we don't have to, as software reading those
 * files might allocate a buffer much larger than necessary, wasting memory.
 */
#define WTAP_MAX_PACKET_SIZE_STANDARD    262144U

and that pcap file is rejected with

(pcap: File has 3402287818-byte packet, bigger than maximum of 262144)

When -s 3402287818 is passed to tshark on i386 it bails out with

tshark: The specified snapshot length "3402287818" is too large (greater than 2147483647)

I think in practice it should be OK to catch the overflow and bail out then. Though I think it should be a separate test case.

[guedou]

2147483647 will be a 2Gb packet, that why I assumed that it was OK to only catch the OverflowError exception =)

[guedou]

If this MR is OK to you, let's merge it, then we will apply #4530

[gpotter2]

Bah it's the opposite way. First we merge #4530 then you come up with a different unit test in this PR that only tests this issue :) (or reuse the previous one I guess).

[guedou]

What do you want to change in the current test?

[evverx]

What do you want to change in the current test?

I think something like

b'\xd4\xc3\xb2\xa1\x02\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\x00\x00\x01\x00\x00\x00%\xa8\xddfK\x1b\x05\x00\xca\xca\xca\xca*\x00\x00\x00\xff\xff\xff\xff\xff\xff\x86"\x11&\xab3\x08\x06\x00\x01\x08\x00\x06\x04\x00\x01]\x80\x0f\x13*r\n\x00\x02\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

should work. It contains \xca\xca\xca\xca leading to

OverflowError: cannot fit 'int' into an index-sized integer

on i386. On 64-bit machines scapy should read a normal ARP packet without triggering any other issues:

0000 Ether / ARP who has 0.0.0.0 says 10.0.2.15

I also passed that pcap to tshark and it rejects it with

(pcap: File has 3402287818-byte packet, bigger than maximum of 262144)

as expected.

[guedou]

Is there really a need for two tests that will each give different results on 32 and 64 bits platforms?Currently, the file from oss-fuzz allows to test the overflow issue on 32 bits platforms, and the LDAP files access on 64 bits ones.Sent from my iPhoneOn 8 Sep 2024, at 16:02, Evgeny Vereshchagin ***@***.***> wrote: What do you want to change in the current test? I think something like b'\xd4\xc3\xb2\xa1\x02\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\x00\x00\x01\x00\x00\x00%\xa8\xddfK\x1b\x05\x00\xca\xca\xca\xca*\x00\x00\x00\xff\xff\xff\xff\xff\xff\x86"\x11&\xab3\x08\x06\x00\x01\x08\x00\x06\x04\x00\x01]\x80\x0f\x13*r\n\x00\x02\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' should work. It contains \xca\xca\xca\xca leading to OverflowError: cannot fit 'int' into an index-sized integer on i386. On 64-bit machines scapy should read a normal ARP packet without triggering any other issues: 0000 Ether / ARP who has 0.0.0.0 says 10.0.2.15 I also passed that pcap to tshark and it rejects it with (pcap: File has 3402287818-byte packet, bigger than maximum of 262144) as expected. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[evverx]

Is there really a need for two tests that will each give different results on 32 and 64 bits platforms?

My thinking was that since the original testcase triggers the overflow (apart from the LDAP crash) it prevents the LDAP crash from being tested on those platforms. To test the LDAP crash and the overflow there ideally there should be two different testcases.

[guedou]

It still don’t see why we need to test the LDAP crash on a specific platform, but as you can provided a working test case, let’s use it =)Sent from my iPhoneOn 8 Sep 2024, at 17:48, Evgeny Vereshchagin ***@***.***> wrote: Is there really a need for two tests that will each give different results on 32 and 64 bits platforms? My thinking was that since the original testcase triggers the overflow (apart from the LDAP crash) it prevents the LDAP crash from being tested on those platforms. To test the LDAP crash and the overflow there ideally there should be two different testcases. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[evverx]

It still don’t see why we need to test the LDAP crash on a specific platform

I agree that it isn't i386-specific but if nothing else coverage reports would look better with those two lines covered there as well :-) When I opened #4530 I hadn't figured out what the issue was so to me it seemed reasonable to unblock the test on 32-bit machines while the overflow issue was in the process of being fixed.

[guedou]

I pushed the alternative test.

[evverx]

I ran it on i386 and it passed

# python3 -m scapy.tools.UTscapy -t test/regression.uts -n 167
...
>>> l = rdpcap(file)
Pcap: cannot fit 'int' into an index-sized integer
>>> assert len(l) == 0 or ARP in l[0]

(what's interesting is that it turns out Wireshark rejects snapshot lengths larger than 2147483647 on 64 bit platforms too even though those in theory should be unsigned integers)