Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vuln OSV-2023-1168 #1697

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

aled-ua
Copy link
Contributor

@aled-ua aled-ua commented Jan 23, 2025

[Warning] This PR is generated by AI

  1. PR Title: Fix for Heap-Buffer-Overflow Vulnerability in PcapPlusPlus - OSV-2023-1168

  2. PR Description:

    • Bug Type: Heap-buffer-overflow
    • Summary: A heap-buffer-overflow vulnerability was detected in the pcpp::PPP_PPTPLayer::computeCalculateFields() function in the PcapPlusPlus project. This issue occurs when the program attempts to access a heap object outside of its allocated memory, specifically trying to write 2 bytes at an offset of 31 bytes, which is beyond the allocated size.
    • Fix Summary: The patch addresses the issue by introducing a bounds check to ensure that the data length (m_DataLen) is sufficient before accessing the ppp_pptp_header structure. If the data length is insufficient, an error is logged and the function exits early, preventing out-of-bounds memory access. This fix improves the security and stability of the program by mitigating the risk of memory corruption due to heap-buffer-overflow.
  3. Sanitizer Report Summary:
    The AddressSanitizer identified a heap-buffer-overflow in the pcpp::PPP_PPTPLayer::computeCalculateFields() function. The program attempted to access 2 bytes at an offset of 31 bytes, which is outside the allocated memory region. The error originated from Packet++/src/GreLayer.cpp:609 and was called by other functions in the program. The issue stemmed from insufficient bounds checking before accessing the buffer.

  4. Full Sanitizer Report:

    ==52175==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x50300000e5af at pc 0x55ed583e9d58 bp 0x7ffdf8fe3600 sp 0x7ffdf8fe35f8
    WRITE of size 2 at 0x50300000e5af thread T0
        #0 0x55ed583e9d57 in pcpp::PPP_PPTPLayer::computeCalculateFields() /root/Packet++/src/GreLayer.cpp:609:21
        #1 0x55ed5842eaac in pcpp::Packet::computeCalculateFields() /root/Packet++/src/Packet.cpp:687:14
        #2 0x55ed58372f8b in LLVMFuzzerTestOneInput /root/Tests/Fuzzers/FuzzTarget.cpp:69:17
        #3 0x55ed5827d6d4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/out/FuzzTarget+0xd66d4) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #4 0x55ed58266806 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/out/FuzzTarget+0xbf806) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #5 0x55ed5826c2ba in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/out/FuzzTarget+0xc52ba) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #6 0x55ed58296a76 in main (/root/out/FuzzTarget+0xefa76) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #7 0x7f03d6e1b1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #8 0x7f03d6e1b28a in __libc_start_main csu/../csu/libc-start.c:360:3
        #9 0x55ed582613d4 in _start (/root/out/FuzzTarget+0xba3d4) (BuildId: d919513dbcd218818b74181b57a030d555310802)
    
    0x50300000e5af is located 0 bytes after 31-byte region [0x50300000e590,0x50300000e5af)
    allocated by thread T0 here:
        #0 0x55ed5836ff21 in operator new[](unsigned long) (/root/out/FuzzTarget+0x1c8f21) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #1 0x55ed5838d8c6 in pcpp::PcapFileReaderDevice::getNextPacket(pcpp::RawPacket&) /root/Pcap++/src/PcapFileDevice.cpp:338:28
        #2 0x55ed5838ae3f in pcpp::IFileReaderDevice::getNextPackets(pcpp::PointerVector<pcpp::RawPacket>&, int) /root/Pcap++/src/PcapFileDevice.cpp:117:22
        #3 0x55ed58372bf4 in LLVMFuzzerTestOneInput /root/Tests/Fuzzers/FuzzTarget.cpp:46:14
        #4 0x55ed5827d6d4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/out/FuzzTarget+0xd66d4) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #5 0x55ed58266806 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/out/FuzzTarget+0xbf806) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #6 0x55ed5826c2ba in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/out/FuzzTarget+0xc52ba) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #7 0x55ed58296a76 in main (/root/out/FuzzTarget+0xefa76) (BuildId: d919513dbcd218818b74181b57a030d555310802)
        #8 0x7f03d6e1b1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #9 0x7f03d6e1b28a in __libc_start_main csu/../csu/libc-start.c:360:3
        #10 0x55ed582613d4 in _start (/root/out/FuzzTarget+0xba3d4) (BuildId: d919513dbcd218818b74181b57a030d555310802)
    
    SUMMARY: AddressSanitizer
    
  5. Files Modified:

    • Packet++/src/GreLayer.cpp
    --- a/Packet++/src/GreLayer.cpp
    +++ b/Packet++/src/GreLayer.cpp
    @@ -591,6 +591,11 @@
     void PPP_PPTPLayer::computeCalculateFields()
     {
     	ppp_pptp_header* header = getPPP_PPTPHeader();
    +	if (m_DataLen < sizeof(ppp_pptp_header))
    +	{
    +		PCPP_LOG_ERROR("Insufficient data length for PPP_PPTP header");
    +		return;
    +	}
     	if (m_NextLayer != nullptr)
     	{
     		switch (m_NextLayer->getProtocol())
  6. Patch Validation: The patch has been validated using the provided PoC and resolved the heap-buffer-overflow vulnerability identified in the sanitizer report. No new issues have been introduced.

  7. Links:

@aled-ua aled-ua requested a review from seladb as a code owner January 23, 2025 09:31
Comment on lines 591 to +598
void PPP_PPTPLayer::computeCalculateFields()
{
ppp_pptp_header* header = getPPP_PPTPHeader();
if (m_DataLen < sizeof(ppp_pptp_header))
{
PCPP_LOG_ERROR("Insufficient data length for PPP_PPTP header");
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be above the ppp_pptp_header* header = getPPP_PPTPHeader(); line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emmm,yes. Should be checked at the beginning of the function.

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.10%. Comparing base (f81ced2) to head (3415918).
Report is 17 commits behind head on dev.

Files with missing lines Patch % Lines
Packet++/src/GreLayer.cpp 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1697      +/-   ##
==========================================
- Coverage   83.16%   83.10%   -0.07%     
==========================================
  Files         277      277              
  Lines       48193    48204      +11     
  Branches     9966     9922      -44     
==========================================
- Hits        40081    40058      -23     
- Misses       7234     7244      +10     
- Partials      878      902      +24     
Flag Coverage Δ
alpine320 75.07% <0.00%> (-0.09%) ⬇️
fedora40 75.10% <0.00%> (-0.10%) ⬇️
macos-13 80.60% <0.00%> (-0.06%) ⬇️
macos-14 80.60% <0.00%> (-0.06%) ⬇️
macos-15 80.58% <0.00%> (-0.06%) ⬇️
mingw32 70.81% <0.00%> (-0.11%) ⬇️
mingw64 70.77% <0.00%> (-0.11%) ⬇️
npcap 85.23% <33.33%> (-0.09%) ⬇️
rhel94 74.96% <0.00%> (-0.08%) ⬇️
ubuntu2004 58.54% <0.00%> (-0.09%) ⬇️
ubuntu2004-zstd 58.66% <0.00%> (-0.09%) ⬇️
ubuntu2204 74.88% <0.00%> (-0.10%) ⬇️
ubuntu2204-icpx 61.47% <0.00%> (+0.02%) ⬆️
ubuntu2404 75.11% <0.00%> (-0.09%) ⬇️
unittest 83.10% <33.33%> (-0.07%) ⬇️
windows-2019 85.26% <33.33%> (-0.09%) ⬇️
windows-2022 85.29% <33.33%> (-0.09%) ⬇️
winpcap 85.25% <33.33%> (-0.08%) ⬇️
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants