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

mbedtls_net_poll corrupts the stack if socket number >= 1024 #4169

Closed
FigBug opened this issue Feb 24, 2021 · 2 comments · Fixed by #4173
Closed

mbedtls_net_poll corrupts the stack if socket number >= 1024 #4169

FigBug opened this issue Feb 24, 2021 · 2 comments · Fixed by #4173
Assignees
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)

Comments

@FigBug
Copy link

FigBug commented Feb 24, 2021

Description

  • Type: Bug
  • Priority: Major

Bug

OS
macOS

mbed TLS build:
Version: a0fd0f8
OS version: 11.1

Expected behavior
Not corrupt the stack

Actual behavior
The stack is corrupted

Steps to reproduce
Open 1024 sockets. Doesn't matter if they are mbedtls sockets or not. Then open another socket via mbedtls and call mbedtls_net_poll


Enhancement\Feature Request

Justification - why does the library need this feature?
mbedtls should not crash the application if more than 1024 sockets are open

Suggested enhancement
Man page for fd_set says "An fd_set is a fixed size buffer. Executing FD_CLR() or FD_SET() with a value of fd that is negative or is equal to or larger than FD_SETSIZE will result in undefined behavior. Moreover, POSIX requires fd to be a valid file descriptor." mbedtls_net_poll should check that the socket number is >= 0 and < FD_SETSIZE.


@FigBug
Copy link
Author

FigBug commented Feb 24, 2021

ASAN report:

=================================================================
==75195==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x00010bc7c4c0 at pc 0x000103669612 bp 0x7ffeecff3990 sp 0x7ffeecff3988
READ of size 4 at 0x00010bc7c4c0 thread T0
    #0 0x103669611 in mbedtls_net_poll net_sockets.c:482
    #1 0x10341df91 in gin::SecureStreamingSocket::Impl::waitUntilReady(bool, int) gin_securestreamingsocket.cpp:199
    #2 0x10341de64 in gin::SecureStreamingSocket::waitUntilReady(bool, int) gin_securestreamingsocket.cpp:314
    #3 0x102c0d013 in MainContentComponent::MainContentComponent() MainComponent.cpp:1351
    #4 0x102c0e9d4 in MainContentComponent::MainContentComponent() MainComponent.cpp:1339
    #5 0x102c95934 in DemoApplication::MainWindow::MainWindow(juce::String) Main.cpp:66
    #6 0x102c955a4 in DemoApplication::MainWindow::MainWindow(juce::String) Main.cpp:64
    #7 0x102c953f2 in std::__1::__unique_if<DemoApplication::MainWindow>::__unique_single std::__1::make_unique<DemoApplication::MainWindow, juce::String const>(juce::String const&&) memory:3033
    #8 0x102c94a4a in DemoApplication::initialise(juce::String const&) Main.cpp:34
    #9 0x1041020e3 in juce::JUCEApplicationBase::initialiseApp() juce_ApplicationBase.cpp:297
    #10 0x1048672a7 in juce::JUCEApplication::initialiseApp() juce_Application.cpp:92
    #11 0x104101b9a in juce::JUCEApplicationBase::main() juce_ApplicationBase.cpp:256
    #12 0x10410189d in juce::JUCEApplicationBase::main(int, char const**) juce_ApplicationBase.cpp:240
    #13 0x102c94681 in main Main.cpp:87
    #14 0x7fff203a9620 in start+0x0 (libdyld.dylib:x86_64+0x15620)

Address 0x00010bc7c4c0 is located in stack of thread T0 at offset 192 in frame
    #0 0x1036692cf in mbedtls_net_poll net_sockets.c:456

  This frame has 3 object(s):
    [32, 48) 'tv' (line 458)
    [64, 192) 'read_fds' (line 460) <== Memory access at offset 192 overflows this variable
    [224, 352) 'write_fds' (line 461)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow net_sockets.c:482 in mbedtls_net_poll
Shadow bytes around the buggy address:
  0x10002178f840: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10002178f850: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10002178f860: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10002178f870: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10002178f880: f1 f1 f1 f1 00 00 f2 f2 00 00 00 00 00 00 00 00
=>0x10002178f890: 00 00 00 00 00 00 00 00[f2]f2 f2 f2 00 00 00 00
  0x10002178f8a0: 00 00 00 00 00 00 00 00 00 00 00 00 f3 f3 f3 f3
  0x10002178f8b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002178f8c0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10002178f8d0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10002178f8e0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
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
  Shadow gap:              cc
2021-02-23 17:13:58.925471-0800 Demo[75195:11338657] =================================================================
2021-02-23 17:13:58.925555-0800 Demo[75195:11338657] ==75195==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x00010bc7c4c0 at pc 0x000103669612 bp 0x7ffeecff3990 sp 0x7ffeecff3988
2021-02-23 17:13:58.925597-0800 Demo[75195:11338657] READ of size 4 at 0x00010bc7c4c0 thread T0
2021-02-23 17:13:58.925631-0800 Demo[75195:11338657]     #0 0x103669611 in mbedtls_net_poll net_sockets.c:482
2021-02-23 17:13:58.925665-0800 Demo[75195:11338657]     #1 0x10341df91 in gin::SecureStreamingSocket::Impl::waitUntilReady(bool, int) gin_securestreamingsocket.cpp:199
2021-02-23 17:13:58.925697-0800 Demo[75195:11338657]     #2 0x10341de64 in gin::SecureStreamingSocket::waitUntilReady(bool, int) gin_securestreamingsocket.cpp:314
2021-02-23 17:13:58.925735-0800 Demo[75195:11338657]     #3 0x102c0d013 in MainContentComponent::MainContentComponent() MainComponent.cpp:1351
2021-02-23 17:13:58.925768-0800 Demo[75195:11338657]     #4 0x102c0e9d4 in MainContentComponent::MainContentComponent() MainComponent.cpp:1339
2021-02-23 17:13:58.925809-0800 Demo[75195:11338657]     #5 0x102c95934 in DemoApplication::MainWindow::MainWindow(juce::String) Main.cpp:66
2021-02-23 17:13:58.925842-0800 Demo[75195:11338657]     #6 0x102c955a4 in DemoApplication::MainWindow::MainWindow(juce::String) Main.cpp:64
2021-02-23 17:13:58.925947-0800 Demo[75195:11338657]     #7 0x102c953f2 in std::__1::__unique_if<DemoApplication::MainWindow>::__unique_single std::__1::make_unique<DemoApplication::MainWindow, juce::String const>(juce::String const&&) memory:3033
2021-02-23 17:13:58.925981-0800 Demo[75195:11338657]     #8 0x102c94a4a in DemoApplication::initialise(juce::String const&) Main.cpp:34
2021-02-23 17:13:58.926016-0800 Demo[75195:11338657]     #9 0x1041020e3 in juce::JUCEApplicationBase::initialiseApp() juce_ApplicationBase.cpp:297
2021-02-23 17:13:58.926051-0800 Demo[75195:11338657]     #10 0x1048672a7 in juce::JUCEApplication::initialiseApp() juce_Application.cpp:92
2021-02-23 17:13:58.926084-0800 Demo[75195:11338657]     #11 0x104101b9a in juce::JUCEApplicationBase::main() juce_ApplicationBase.cpp:256
2021-02-23 17:13:58.926115-0800 Demo[75195:11338657]     #12 0x10410189d in juce::JUCEApplicationBase::main(int, char const**) juce_ApplicationBase.cpp:240
2021-02-23 17:13:58.926158-0800 Demo[75195:11338657]     #13 0x102c94681 in main Main.cpp:87
2021-02-23 17:13:58.926191-0800 Demo[75195:11338657]     #14 0x7fff203a9620 in start+0x0 (libdyld.dylib:x86_64+0x15620)
2021-02-23 17:13:58.926222-0800 Demo[75195:11338657] 
2021-02-23 17:13:58.926252-0800 Demo[75195:11338657] Address 0x00010bc7c4c0 is located in stack of thread T0 at offset 192 in frame
2021-02-23 17:13:58.927706-0800 Demo[75195:11338657]     #0 0x1036692cf in mbedtls_net_poll net_sockets.c:456
2021-02-23 17:13:58.927770-0800 Demo[75195:11338657] 
2021-02-23 17:13:58.927805-0800 Demo[75195:11338657]   This frame has 3 object(s):
2021-02-23 17:13:58.927848-0800 Demo[75195:11338657]     [32, 48) 'tv' (line 458)
2021-02-23 17:13:58.927883-0800 Demo[75195:11338657]     [64, 192) 'read_fds' (line 460) <== Memory access at offset 192 overflows this variable
2021-02-23 17:13:58.927918-0800 Demo[75195:11338657]     [224, 352) 'write_fds' (line 461)
2021-02-23 17:13:58.927955-0800 Demo[75195:11338657] HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
2021-02-23 17:13:58.928002-0800 Demo[75195:11338657]       (longjmp and C++ exceptions *are* supported)
2021-02-23 17:13:58.928040-0800 Demo[75195:11338657] SUMMARY: AddressSanitizer: stack-buffer-overflow net_sockets.c:482 in mbedtls_net_poll
2021-02-23 17:13:58.928080-0800 Demo[75195:11338657] Shadow bytes around the buggy address:
2021-02-23 17:13:58.928115-0800 Demo[75195:11338657]   0x10002178f840: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928152-0800 Demo[75195:11338657]   0x10002178f850: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928184-0800 Demo[75195:11338657]   0x10002178f860: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928217-0800 Demo[75195:11338657]   0x10002178f870: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928251-0800 Demo[75195:11338657]   0x10002178f880: f1 f1 f1 f1 00 00 f2 f2 00 00 00 00 00 00 00 00
2021-02-23 17:13:58.928285-0800 Demo[75195:11338657] =>0x10002178f890: 00 00 00 00 00 00 00 00[f2]f2 f2 f2 00 00 00 00
2021-02-23 17:13:58.928317-0800 Demo[75195:11338657]   0x10002178f8a0: 00 00 00 00 00 00 00 00 00 00 00 00 f3 f3 f3 f3
2021-02-23 17:13:58.928350-0800 Demo[75195:11338657]   0x10002178f8b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2021-02-23 17:13:58.928382-0800 Demo[75195:11338657]   0x10002178f8c0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928416-0800 Demo[75195:11338657]   0x10002178f8d0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928448-0800 Demo[75195:11338657]   0x10002178f8e0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
2021-02-23 17:13:58.928482-0800 Demo[75195:11338657] Shadow byte legend (one shadow byte represents 8 application bytes):
2021-02-23 17:13:58.928514-0800 Demo[75195:11338657]   Addressable:           00
2021-02-23 17:13:58.928547-0800 Demo[75195:11338657]   Partially addressable: 01 02 03 04 05 06 07
2021-02-23 17:13:58.928578-0800 Demo[75195:11338657]   Heap left redzone:       fa
2021-02-23 17:13:58.928610-0800 Demo[75195:11338657]   Freed heap region:       fd
2021-02-23 17:13:58.928648-0800 Demo[75195:11338657]   Stack left redzone:      f1
2021-02-23 17:13:58.928682-0800 Demo[75195:11338657]   Stack mid redzone:       f2
2021-02-23 17:13:58.928715-0800 Demo[75195:11338657]   Stack right redzone:     f3
2021-02-23 17:13:58.928746-0800 Demo[75195:11338657]   Stack after return:      f5
2021-02-23 17:13:58.928779-0800 Demo[75195:11338657]   Stack use after scope:   f8
2021-02-23 17:13:58.928810-0800 Demo[75195:11338657]   Global redzone:          f9
2021-02-23 17:13:58.928907-0800 Demo[75195:11338657]   Global init order:       f6
2021-02-23 17:13:58.928972-0800 Demo[75195:11338657]   Poisoned by user:        f7
2021-02-23 17:13:58.929011-0800 Demo[75195:11338657]   Container overflow:      fc
2021-02-23 17:13:58.929046-0800 Demo[75195:11338657]   Array cookie:            ac
2021-02-23 17:13:58.929079-0800 Demo[75195:11338657]   Intra object redzone:    bb
2021-02-23 17:13:58.929113-0800 Demo[75195:11338657]   ASan internal:           fe
2021-02-23 17:13:58.929145-0800 Demo[75195:11338657]   Left alloca redzone:     ca
2021-02-23 17:13:58.929176-0800 Demo[75195:11338657]   Right alloca redzone:    cb
2021-02-23 17:13:58.929207-0800 Demo[75195:11338657]   Shadow gap:              cc
==75195==ABORTING

@yanesca yanesca added bug Community component-platform Portability layer and build scripts size-s Estimated task size: small (~2d) labels Feb 24, 2021
@gilles-peskine-arm gilles-peskine-arm self-assigned this Feb 24, 2021
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Feb 24, 2021
mbedtls_net_poll() and mbedtls_net_recv_timeout() rely on select(),
which represents sets of file descriptors through the fd_set type.
This type cannot hold file descriptors larger than FD_SETSIZE. Make
sure that these functions identify this failure code.

Without a proper range check of the file descriptor in the
mbedtls_net_xxx function, this test fails when running with UBSan:
```
net_poll beyond FD_SETSIZE ........................................ source/library/net_sockets.c:482:9: runtime error: index 16 out of bounds for type '__fd_mask [16]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior source/library/net_sockets.c:482:9 in
```
This is a non-regression test for
Mbed-TLS#4169 .

The implementation of this test is specific to Unix-like platforms.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Feb 24, 2021
Fix a stack buffer overflow with mbedtls_net_poll() and
mbedtls_net_recv_timeout() when given a file descriptor that is beyond
FD_SETSIZE. The bug was due to not checking that the file descriptor
is within the range of an fd_set object.

Fix Mbed-TLS#4169

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg
Copy link
Contributor

mpg commented Feb 25, 2021

Hi @FigBug and thanks for your report! We agree with your assessment and have created a PR with a fix and non-regression test: #4173, so the issue will be fixed in the next release.

As all memory management bugs, this potentially has security implications. In the future, I would like to encourage you (and any other contributor reading this) to report security vulnerabilities privately to mbed-tls-security@lists.trustedfirmware.org rather than using a github issue, so that we can handle it according to our security process - basically this ensures that a fix is available and ready to be deployed before the issue gets public.

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Feb 25, 2021
mbedtls_net_poll() and mbedtls_net_recv_timeout() rely on select(),
which represents sets of file descriptors through the fd_set type.
This type cannot hold file descriptors larger than FD_SETSIZE. Make
sure that these functions identify this failure code.

Without a proper range check of the file descriptor in the
mbedtls_net_xxx function, this test fails when running with UBSan:
```
net_poll beyond FD_SETSIZE ........................................ source/library/net_sockets.c:482:9: runtime error: index 16 out of bounds for type '__fd_mask [16]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior source/library/net_sockets.c:482:9 in
```
This is a non-regression test for
Mbed-TLS#4169 .

The implementation of this test is specific to Unix-like platforms.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Feb 25, 2021
Fix a stack buffer overflow with mbedtls_net_poll() and
mbedtls_net_recv_timeout() when given a file descriptor that is beyond
FD_SETSIZE. The bug was due to not checking that the file descriptor
is within the range of an fd_set object.

Fix Mbed-TLS#4169

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Mar 1, 2021
Fix a stack buffer overflow with mbedtls_net_poll() and
mbedtls_net_recv_timeout() when given a file descriptor that is beyond
FD_SETSIZE. The bug was due to not checking that the file descriptor
is within the range of an fd_set object.

Fix Mbed-TLS#4169

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Mar 1, 2021
Fix a stack buffer overflow with mbedtls_net_recv_timeout() when given a
file descriptor that is beyond FD_SETSIZE. The bug was due to not checking
that the file descriptor is within the range of an fd_set object.

Fix Mbed-TLS#4169

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Mar 3, 2021
Fix a stack buffer overflow with mbedtls_net_recv_timeout() when given a
file descriptor that is beyond FD_SETSIZE. The bug was due to not checking
that the file descriptor is within the range of an fd_set object.

Fix Mbed-TLS#4169

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants