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 double free in Net::MailMessage if Content-Disposition header is empty #4688

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

tyler92
Copy link
Contributor

@tyler92 tyler92 commented Sep 12, 2024

This is a fix for #4687

The same pointer was added to Net::MailMessage::_parts twice which led to calling delete twice on the same pointer. It happened due to incorrect handling of an empty Content-Disposition header. The patch fixes this behavior according to RFC 6266:

Recipients MAY take steps to recover a usable field value from an
invalid header field, but SHOULD NOT reject the message outright,
unless this is explicitly desirable behavior (e.g., the
implementation is a validator). As such, the default handling of
invalid fields is to ignore them.

Also a unit test was added for the case. ASAN report for the test without the fix:

==629628==ERROR: AddressSanitizer: heap-use-after-free on address 0x515000003280 at pc 0x7f4522504ba5 bp 0x7ffdb0169220 sp 0x7ffdb0169218
READ of size 8 at 0x515000003280 thread T0
    #0 0x7f4522504ba4 in Poco::Net::MailMessage::~MailMessage() poco/Net/src/MailMessage.cpp:225:3
    #1 0x55f7205a457d in MailMessageTest::testReadMultiPartInvalidContentDisposition() poco/Net/testsuite/src/MailMessageTest.cpp:594:1
    #2 0x7f4521def935 in CppUnit::TestCase::run(CppUnit::TestResult*, std::function<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> (std::exception const&)> const&) poco/CppUnit/src/TestCase.cpp:116:3
    #3 0x7f4521df80ae in CppUnit::TestRunner::run(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, std::function<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> (std::exception const&)> const&) poco/CppUnit/src/TestRunner.cpp:141:14
    #4 0x55f7203da5a0 in main poco/Net/testsuite/src/Driver.cpp:17:1
    #5 0x7f4520dbbd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #6 0x7f4520dbbe3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #7 0x55f7202a2f14 in _start (poco/build/forfuzz/bin/Net-testrunner+0x1d1f14)

0x515000003280 is located 0 bytes inside of 504-byte region [0x515000003280,0x515000003478)
freed by thread T0 here:
    #0 0x55f72037c58d in operator delete(void*) (poco/build/forfuzz/bin/Net-testrunner+0x2ab58d)
    #1 0x7f4522504afb in Poco::Net::MailMessage::~MailMessage() poco/Net/src/MailMessage.cpp:225:3
    #2 0x55f7205a457d in MailMessageTest::testReadMultiPartInvalidContentDisposition() poco/Net/testsuite/src/MailMessageTest.cpp:594:1
    #3 0x7f4521def935 in CppUnit::TestCase::run(CppUnit::TestResult*, std::function<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> (std::exception const&)> const&) poco/CppUnit/src/TestCase.cpp:116:3
    #4 0x7f4521df80ae in CppUnit::TestRunner::run(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, std::function<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> (std::exception const&)> const&) poco/CppUnit/src/TestRunner.cpp:141:14
    #5 0x55f7203da5a0 in main poco/Net/testsuite/src/Driver.cpp:17:1
    #6 0x7f4520dbbd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

previously allocated by thread T0 here:
    #0 0x55f72037bd2d in operator new(unsigned long) (poco/build/forfuzz/bin/Net-testrunner+0x2aad2d)
    #1 0x7f452250d924 in Poco::Net::MailMessage::createPartStore(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) poco/Net/src/MailMessage.cpp:700:30
    #2 0x7f452250d924 in Poco::Net::(anonymous namespace)::MultiPartHandler::handlePart(Poco::Net::MessageHeader const&, std::istream&) poco/Net/src/MailMessage.cpp:102:30
    #3 0x7f4522507264 in Poco::Net::MailMessage::handlePart(std::istream&, Poco::Net::MessageHeader const&, Poco::Net::PartHandler&) poco/Net/src/MailMessage.cpp:543:10
    #4 0x7f4522507264 in Poco::Net::MailMessage::readPart(std::istream&, Poco::Net::MessageHeader const&, Poco::Net::PartHandler&) poco/Net/src/MailMessage.cpp:526:9
    #5 0x7f4522506af1 in Poco::Net::MailMessage::readMultipart(std::istream&, Poco::Net::PartHandler&) poco/Net/src/MailMessage.cpp:501:3
    #6 0x7f4522507b1d in Poco::Net::MailMessage::read(std::istream&) poco/Net/src/MailMessage.cpp:369:3
    #7 0x55f7205a31d8 in MailMessageTest::testReadMultiPartInvalidContentDisposition() poco/Net/testsuite/src/MailMessageTest.cpp:585:17
    #8 0x7f4521def935 in CppUnit::TestCase::run(CppUnit::TestResult*, std::function<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> (std::exception const&)> const&) poco/CppUnit/src/TestCase.cpp:116:3
    #9 0x7f4521df80ae in CppUnit::TestRunner::run(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, std::function<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> (std::exception const&)> const&) poco/CppUnit/src/TestRunner.cpp:141:14
    #10 0x55f7203da5a0 in main poco/Net/testsuite/src/Driver.cpp:17:1
    #11 0x7f4520dbbd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: heap-use-after-free poco/Net/src/MailMessage.cpp:225:3 in Poco::Net::MailMessage::~MailMessage()
Shadow bytes around the buggy address:
  0x515000003000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x515000003080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x515000003100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x515000003180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x515000003200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x515000003280:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x515000003300: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x515000003380: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x515000003400: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x515000003480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x515000003500: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==629628==ABORTING

Closes #4687

Copy link
Contributor

@matejk matejk left a comment

Choose a reason for hiding this comment

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

Changes and unit tests seem OK to me.

@matejk matejk added this to the Release 1.14.0 milestone Sep 13, 2024
@matejk matejk merged commit cefab15 into pocoproject:main Sep 13, 2024
33 checks passed
@tyler92 tyler92 deleted the mail-message-double-free branch September 13, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Net::MailMessage: Double free if Content-Disposition header is empty
2 participants