Fix Heap-buffer-overflow in __parse_options #1678

aled-ua · 2025-01-04T10:42:49Z

[Warning] This PR is generated by AI

PR Title: Fix for Heap-buffer-overflow Vulnerability in pcapplusplus
PR Description:
- Bug Type: Heap-buffer-overflow
- Summary: The vulnerability involved a heap-buffer-overflow where the program attempted to access memory outside of an allocated heap object. Specifically, the allocated memory size was 40 bytes, but the program attempted to access 72 bytes at offset 40. This occurred in the __parse_options function in the LightPcapNg library utilized by pcapplusplus.
- Fix Summary: The patch resolves the heap-buffer-overflow by introducing bounds checking for the option_length variable. It ensures that memory operations like memcpy are only performed within valid memory bounds. This prevents the program from accessing out-of-bounds memory, thereby improving the security and stability of the program.
Sanitizer Report Summary: The AddressSanitizer detected a heap-buffer-overflow triggered when the program attempted to read 72 bytes from a 40-byte allocated region. This error originated in the __parse_options function in /3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c:58:10, which was called through a chain ultimately leading to LLVMFuzzerTestOneInput.

Full Sanitizer Report:

==4611==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5040000000b8 at pc 0x55fce6a53a66 bp 0x7ffee4103470 sp 0x7ffee4102c30
READ of size 72 at 0x5040000000b8 thread T0
    #0 0x55fce6a53a65 in __asan_memcpy (/root/out/FuzzWriterNg+0x120a65) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #1 0x55fce6abb400 in __parse_options /root/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c:58:10
    #2 0x55fce6ab9e0b in parse_by_block_type /root/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c:110:16
    #3 0x55fce6abbd1b in light_read_record /root/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c:356:4
    #4 0x55fce6ab45db in light_pcapng_open_read /root/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng_ext.c:193:2
    #5 0x55fce6a9c914 in pcpp::PcapNgFileReaderDevice::open() /root/Pcap++/src/PcapFileDevice.cpp:376:19
    #6 0x55fce6a968a3 in LLVMFuzzerTestOneInput /root/Tests/Fuzzers/FuzzWriter.cpp:30:15
    #7 0x55fce69a1a84 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/out/FuzzWriterNg+0x6ea84) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #8 0x55fce698abb6 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/out/FuzzWriterNg+0x57bb6) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #9 0x55fce699066a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/out/FuzzWriterNg+0x5d66a) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #10 0x55fce69bae26 in main (/root/out/FuzzWriterNg+0x87e26) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #11 0x7f2f6aedd1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #12 0x7f2f6aedd28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #13 0x55fce6985784 in _start (/root/out/FuzzWriterNg+0x52784) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)

0x5040000000b8 is located 0 bytes after 40-byte region [0x504000000090,0x5040000000b8)
allocated by thread T0 here:
    #0 0x55fce6a55d9d in calloc (/root/out/FuzzWriterNg+0x122d9d) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #1 0x55fce6abbb41 in light_read_record /root/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c:333:27
    #2 0x55fce6ab45db in light_pcapng_open_read /root/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng_ext.c:193:2
    #3 0x55fce6a9c914 in pcpp::PcapNgFileReaderDevice::open() /root/Pcap++/src/PcapFileDevice.cpp:376:19
    #4 0x55fce6a968a3 in LLVMFuzzerTestOneInput /root/Tests/Fuzzers/FuzzWriter.cpp:30:15
    #5 0x55fce69a1a84 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/out/FuzzWriterNg+0x6ea84) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #6 0x55fce698abb6 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/out/FuzzWriterNg+0x57bb6) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #7 0x55fce699066a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/out/FuzzWriterNg+0x5d66a) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #8 0x55fce69bae26 in main (/root/out/FuzzWriterNg+0x87e26) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)
    #9 0x7f2f6aedd1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #10 0x7f2f6aedd28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #11 0x55fce6985784 in _start (/root/out/FuzzWriterNg+0x52784) (BuildId: 5f2198dd65c02dad0c0611a29e0088abab37738e)

SUMMARY: AddressSanitizer

Files Modified:

/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c

--- a/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c
+++ b/3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c
@@ -48,6 +48,12 @@
 
      opt->custom_option_code = *local_memory++;
      opt->option_length = *local_memory++;
+
+      // Validate option_length
+      if (opt->option_length > max_len - 2 * sizeof(*local_memory)) {
+          free(opt);
+          return NULL;
+      }
 
      actual_length = (opt->option_length % alignment) == 0 ?
            opt->option_length :
@@ -56,7 +62,12 @@
 
      if (actual_length > 0) {
         opt->data = calloc(1, actual_length);
-        memcpy(opt->data, local_memory, actual_length);
+        if (actual_length <= max_len - 2 * sizeof(*local_memory)) {
+            memcpy(opt->data, local_memory, actual_length);
+        } else {
+            free(opt->data);
+            opt->data = NULL;
+        }
         local_memory += (sizeof(**memory) / sizeof(*local_memory)) * (actual_length / alignment);
      }

Patch Validation: The patch has been validated and confirmed to resolve the heap-buffer-overflow issue identified in the sanitizer report. The program has been tested using the provided PoC, and no new issues were introduced.
Links:
- Original PoC

3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c

Dimi1010 · 2025-01-04T11:05:56Z

Close and reopen to run CI after changing base branch.

codecov · 2025-01-04T11:19:37Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.15%. Comparing base (f81ced2) to head (e347d4d).
Report is 8 commits behind head on dev.

Files with missing lines	Patch %	Lines
...rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c	0.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1678      +/-   ##
==========================================
- Coverage   83.16%   83.15%   -0.02%     
==========================================
  Files         277      277              
  Lines       48193    48203      +10     
  Branches     9966     9975       +9     
==========================================
  Hits        40081    40081              
- Misses       7234     7246      +12     
+ Partials      878      876       -2

Flag	Coverage Δ
alpine320	`75.14% <0.00%> (-0.02%)`	⬇️
fedora40	`75.18% <0.00%> (-0.02%)`	⬇️
macos-13	`80.65% <0.00%> (-0.01%)`	⬇️
macos-14	`80.65% <0.00%> (-0.01%)`	⬇️
macos-15	`80.61% <0.00%> (-0.02%)`	⬇️
mingw32	`70.89% <ø> (-0.03%)`	⬇️
mingw64	`70.86% <ø> (-0.03%)`	⬇️
npcap	`85.30% <ø> (-0.02%)`	⬇️
rhel94	`75.01% <0.00%> (-0.02%)`	⬇️
ubuntu2004	`58.62% <0.00%> (-0.01%)`	⬇️
ubuntu2004-zstd	`58.74% <0.00%> (-0.01%)`	⬇️
ubuntu2204	`74.93% <0.00%> (-0.05%)`	⬇️
ubuntu2204-icpx	`61.40% <ø> (-0.04%)`	⬇️
ubuntu2404	`75.21% <0.00%> (+0.01%)`	⬆️
unittest	`83.15% <0.00%> (-0.02%)`	⬇️
windows-2019	`85.33% <ø> (-0.01%)`	⬇️
windows-2022	`85.36% <ø> (-0.01%)`	⬇️
winpcap	`85.33% <ø> (ø)`
xdp	`50.52% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Dimi1010 · 2025-01-05T09:54:18Z

3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c

      actual_length = (opt->option_length % alignment) == 0 ?
            opt->option_length :
            (opt->option_length / alignment + 1) * alignment;

-      if (actual_length > 0) {
+      if (actual_length > 0 && actual_length <= max_len - 2 * sizeof(*local_memory)) {


Not sure if this is enough to terminate the parsing operation.

If the actual length is out of bounds, the recursion continues.
Also remaining_size on L69 will underflow from the subtraction.

Not sure if this is enough to terminate the parsing operation.

If the actual length is out of bounds, the recursion continues. Also remaining_size on L69 will underflow from the subtraction.

Should we terminate immediately when overflow is found? patch similar to L52.

Yes, I think terminating immediately is better...

Also @aled-ua please use the // PCPP patch comment when making changes in LightPcapNg (you can find it in various places in LightPcapNg code), since it's a 3rd-party library and we'd like to contribute it back at some point

Fix vuln crash-7d18f37e1f05e0ff4aa4dfa2f67dd738340ad9cf

8d546b1

aled-ua requested a review from seladb as a code owner January 4, 2025 10:42

Dimi1010 changed the base branch from master to dev January 4, 2025 11:00

Dimi1010 reviewed Jan 4, 2025

View reviewed changes

3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c Outdated Show resolved Hide resolved

Dimi1010 closed this Jan 4, 2025

Dimi1010 reopened this Jan 4, 2025

Move the heap overflow check before the allocation

e347d4d

Dimi1010 reviewed Jan 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Heap-buffer-overflow in __parse_options #1678

Fix Heap-buffer-overflow in __parse_options #1678

aled-ua commented Jan 4, 2025

Dimi1010 commented Jan 4, 2025

codecov bot commented Jan 4, 2025 •

edited

Loading

Dimi1010 Jan 5, 2025

aled-ua Jan 7, 2025 •

edited

Loading

seladb Jan 8, 2025

Fix Heap-buffer-overflow in __parse_options #1678

Are you sure you want to change the base?

Fix Heap-buffer-overflow in __parse_options #1678

Conversation

aled-ua commented Jan 4, 2025

Dimi1010 commented Jan 4, 2025

codecov bot commented Jan 4, 2025 • edited Loading

Codecov Report

Dimi1010 Jan 5, 2025

Choose a reason for hiding this comment

aled-ua Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

seladb Jan 8, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2025 •

edited

Loading

aled-ua Jan 7, 2025 •

edited

Loading