Fix Heap-buffer-overflow in __parse_options by oss-patch · Pull Request #1678 · seladb/PcapPlusPlus

oss-patch · 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 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.10%. Comparing base (d60c699) to head (bcdb1a2).
Report is 7 commits behind head on dev.

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

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1678      +/-   ##
==========================================
- Coverage   83.11%   83.10%   -0.02%     
==========================================
  Files         277      277              
  Lines       48207    48212       +5     
  Branches    10192     9919     -273     
==========================================
- Hits        40069    40065       -4     
- Misses       7243     7251       +8     
- Partials      895      896       +1

Flag	Coverage Δ
alpine320	`75.08% <50.00%> (-0.03%)`	⬇️
fedora40	`75.13% <55.55%> (-0.03%)`	⬇️
macos-13	`80.60% <55.55%> (-0.02%)`	⬇️
macos-14	`80.59% <55.55%> (-0.03%)`	⬇️
macos-15	`80.57% <55.55%> (-0.02%)`	⬇️
mingw32	`70.84% <ø> (-0.03%)`	⬇️
mingw64	`70.78% <ø> (-0.05%)`	⬇️
npcap	`85.23% <ø> (-0.03%)`	⬇️
rhel94	`74.95% <55.55%> (-0.03%)`	⬇️
ubuntu2004	`58.56% <50.00%> (-0.04%)`	⬇️
ubuntu2004-zstd	`58.71% <50.00%> (+0.02%)`	⬆️
ubuntu2204	`74.87% <50.00%> (-0.03%)`	⬇️
ubuntu2204-icpx	`61.49% <ø> (+0.04%)`	⬆️
ubuntu2404	`75.15% <50.00%> (-0.01%)`	⬇️
unittest	`83.10% <55.55%> (-0.02%)`	⬇️
windows-2019	`85.27% <ø> (-0.01%)`	⬇️
windows-2022	`85.29% <ø> (-0.02%)`	⬇️
winpcap	`85.26% <ø> (ø)`
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

            (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

@Dimi1010 can you re-review the PR before merging? It looks good to me but it'd be good to have another pair of eyes looking at it

…40ad9cf

Fix vuln crash-7d18f37e1f05e0ff4aa4dfa2f67dd738340ad9cf

8d546b1

oss-patch 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

oss-patch added 2 commits January 9, 2025 11:31

Terminating immediately when overflow is found

6c67fb2

Fix typo err

6c5d27f

cla7aye15I4nd force-pushed the patch-crash-7d18f37e1f05e0ff4aa4dfa2f67dd738340ad9cf branch from 1fbb74c to 6c5d27f Compare January 21, 2025 12:32

Merge branch 'dev' into patch-crash-7d18f37e1f05e0ff4aa4dfa2f67dd7383…

bcdb1a2

…40ad9cf

seladb approved these changes Feb 2, 2025

View reviewed changes

tigercosmos merged commit a9e8239 into seladb:dev Feb 3, 2025
41 checks passed

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
tigercosmos merged 5 commits intoseladb:devfrom
oss-patch:patch-crash-7d18f37e1f05e0ff4aa4dfa2f67dd738340ad9cf

oss-patch commented Jan 4, 2025

Uh oh!

Uh oh!

Dimi1010 commented Jan 4, 2025

Uh oh!

codecov bot commented Jan 4, 2025 •

edited

Loading

Uh oh!

Dimi1010 Jan 5, 2025

Uh oh!

oss-patch Jan 7, 2025 •

edited

Loading

Uh oh!

seladb Jan 8, 2025

Uh oh!

seladb Feb 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

oss-patch commented Jan 4, 2025

Uh oh!

Uh oh!

Dimi1010 commented Jan 4, 2025

Uh oh!

codecov bot commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Dimi1010 Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

oss-patch Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seladb Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

seladb Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jan 4, 2025 •

edited

Loading

oss-patch Jan 7, 2025 •

edited

Loading