Skip to content

Fix double free in Net::MailMessage if Content-Disposition header is empty#4688

Merged
matejk merged 1 commit intopocoproject:mainfrom
tyler92:mail-message-double-free
Sep 13, 2024
Merged

Fix double free in Net::MailMessage if Content-Disposition header is empty#4688
matejk merged 1 commit intopocoproject:mainfrom
tyler92:mail-message-double-free

Conversation

@tyler92
Copy link
Copy Markdown
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

matejk
matejk approved these changes Sep 13, 2024
View reviewed changes
Copy link
Copy Markdown
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
@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

Reviewers

@matejk matejk matejk approved these changes

Assignees

No one assigned

Labels

None yet

Projects

No open projects
1.14
Status: Done

Milestone

Release 1.14.0

Development

Successfully merging this pull request may close these issues.

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

2 participants

@tyler92 @matejk