Heap-buffer-overflow detected by ASAN

[noboruma]

Describe the bug

While running msquic with ASAN activated on an environment with traffic, I am seeing the following:

==1457588==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x52105134d471 at pc 0x000000fa3ab5 bp 0x7be38203e870 sp 0x7be38203e860
READ of size 2 at 0x52105134d471 thread T22
    #0 0xfa3ab4 in QuicConnRecvDatagrams /msquic/src/core/connection.c:5887
    #1 0xfa3dbf in QuicConnFlushRecv /msquic/src/core/connection.c:5944
    #2 0xfab576 in QuicConnDrainOperations /msquic/src/core/connection.c:7817
    #3 0xfe33b5 in QuicWorkerProcessConnection /msquic/src/core/worker.c:579
    #4 0xfe4d92 in QuicWorkerLoop /msquic/src/core/worker.c:740
    #5 0xfd8df8 in CxPlatRunExecutionContexts /msquic/src/platform/platform_worker.c:562
    #6 0xfd9bb8 in CxPlatWorkerThread /msquic/src/platform/platform_worker.c:730
    #7 0x7be3e385ea41 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234
    #8 0x7be3e349caa3  (/lib/x86_64-linux-gnu/libc.so.6+0x9caa3) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
    #9 0x7be3e3529c3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
0x52105134d471 is located 143 bytes before 4128-byte region [0x52105134d500,0x52105134e520)
allocated by thread T17 here:
    #0 0x7be3e38fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x100f784 in QuicRecvBufferInitialize /msquic/src/core/recv_buffer.c:298
    #2 0x1056372 in QuicCryptoInitialize /msquic/src/core/crypto.c:151
    #3 0xfab72b in QuicConnDrainOperations /msquic/src/core/connection.c:7774
    #4 0xfe33b5 in QuicWorkerProcessConnection /msquic/src/core/worker.c:579
    #5 0xfe4d92 in QuicWorkerLoop /msquic/src/core/worker.c:740
    #6 0xfd8df8 in CxPlatRunExecutionContexts /msquic/src/platform/platform_worker.c:562
    #7 0xfd9bb8 in CxPlatWorkerThread /msquic/src/platform/platform_worker.c:730
    #8 0x7be3e385ea41 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234
    #9 0x7be3e349caa3  (/lib/x86_64-linux-gnu/libc.so.6+0x9caa3) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
Thread T22 created by T0 here:
    #0 0x7be3e38f51f9 in pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:245
    #1 0xf80e06 in CxPlatThreadCreate /msquic/src/platform/platform_posix.c:688
    #2 0xfd752f in CxPlatWorkerPoolInitWorker /msquic/src/platform/platform_worker.c:211
    #3 0xfd81c8 in CxPlatWorkerPoolCreate /msquic/src/platform/platform_worker.c:329
    #4 0xf872f7 in QuicLibraryLazyInitialize /msquic/src/core/library.c:812
    #5 0x100d930 in MsQuicRegistrationOpen /msquic/src/core/registration.c:59
    #6 0xf7c89a in MsQuicSetup /_/github.com/noboruma/[email protected]/pkg/quic/c/msquic.c:577
Thread T17 created by T0 here:
    #0 0x7be3e38f51f9 in pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:245
    #1 0xf80e06 in CxPlatThreadCreate /msquic/src/platform/platform_posix.c:688
    #2 0xfd752f in CxPlatWorkerPoolInitWorker /msquic/src/platform/platform_worker.c:211
    #3 0xfd81c8 in CxPlatWorkerPoolCreate /msquic/src/platform/platform_worker.c:329
    #4 0xf872f7 in QuicLibraryLazyInitialize /msquic/src/core/library.c:812
    #5 0x100d930 in MsQuicRegistrationOpen /msquic/src/core/registration.c:59
    #6 0xf7c89a in MsQuicSetup /_/github.com/noboruma/[email protected]/pkg/quic/c/msquic.c:577
SUMMARY: AddressSanitizer: heap-buffer-overflow /msquic/src/core/connection.c:5887 in QuicConnRecvDatagrams
Shadow bytes around the buggy address:
  0x52105134d180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52105134d200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52105134d280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52105134d300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52105134d380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x52105134d400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa
  0x52105134d480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52105134d500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52105134d580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52105134d600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52105134d680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
==1457588==ABORTING

Affected OS

• [ ] Windows
• [x] Linux
• [ ] macOS
• [ ] Other (specify below)

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

Leave msquic running for some time with traffic opening streams & connection on a regular basis.

Expected behavior

No crash

Actual outcome

Heap buffer overflow detected

Additional details

No response

[guhetier]

Thanks for reporting this. Which version / commit of MsQuic have you been using to get this ASAN report? It'll be helpful to map the lines numbers.

[noboruma]

Thanks for you reply @guhetier - I used this commit: c01ccbd202b15409b1f12079243e26bd79cb78d5

[gaurav2699]

for (uint8_t i = Connection->PathsCount - 1; i > 0; --i) {
    if (!Connection->Paths[i].GotValidPacket) {
        QuicTraceLogConnInfo(
            PathDiscarded,
            Connection,
            "Removing invalid path[%hhu]",
            Connection->Paths[i].ID);
        QuicPathRemove(Connection, i);
    }
}

I think this is the problematic section, The problem is that PathsCount is defined with the annotation Field_range(0, QUIC_MAX_PATH_COUNT), meaning it can be 0. When PathsCount is 0, PathsCount - 1 becomes 255 (due to unsigned underflow with uint8_t)

[noboruma]

@gaurav2699 happy to help with qualifying any fixes

[guhetier]

@gaurav2699 I agree i should be signed here, with the loop written as it is (or offset by one so it stays positive). I think there is another instance of the same issue in QuicConnGetPathForPacket.

I am not sure how this code is reached with zero paths though. Looking at the code of QuicConnGetPathForPacket, the comment below might be misleading?

            QuicPacketLogDrop(Connection, Packet, "Max paths already tracked");

Or there might be another path though this function that let us reach the loop with no path. I'd be good to understand if there is more to this issue that the loop counter.

[gaurav2699]

Yes that comment is probably not accurate. Speaking on how it could have reached there with no paths, its hard to tell without more details but I agree that would be essential to understand. It is probably a case of race condition.

[guhetier]

We have an internal work item reproducing the same issue: https://microsoft.visualstudio.com/OS/_workitems/edit/38211784/

Something is wrong with the path logic but we need logs to understand how we get there. The internal partner is trying to collect some.

@noboruma if you still have a repro of this issue and are able to collect MsQuic logs, that would be very helpful! I suspect a race or logic error in the path validation logic.

[noboruma]

@guhetier we are still able to repro. What logs can I provide?

[guhetier]

Assuming you have a build of MsQuic built with LTTNG enabled, please follow the instructions here to capture the trace: https://github.com/microsoft/msquic/blob/main/docs/Diagnostics.md#linux-1

And here to decode it: https://github.com/microsoft/msquic/blob/main/docs/Diagnostics.md#linux-2

Thanks!