CMake Build Support by nibanks · Pull Request #2 · microsoft/msquic

nibanks · 2019-11-02T15:56:22Z

Updates the build files to CMake (instead of VS project files and Linux makefiles). Also adds googletest submodule for test dependency.

Corresponding internal PR: 3965727

jkotalik · 2019-11-04T04:13:01Z

@BrennanConroy if you are interested.

BUILD.md

azure-pipelines.yml

core/CMakeLists.txt

test/bin/quic_gtest.h

test/lib/QuicTest.cpp

jkotalik

Overall looks solid to me besides some nits.

BrennanConroy · 2019-11-04T16:36:47Z

azure-pipelines.yml

+#- task: CmdLine@2
+#  displayName: 'Run Tests'
+#  inputs:
+#    script: '$(testCmd)'


Enable this before merging?

Nope. Not yet. Windows tests don't work because Azure Pipelines doesn't seem to support Windows Insider builds, and Linux also has a few test failures that need investigating. Tests will (hopefully) be enabled in a future PR.

Nick mentioned this over email; apparently AzDO agents don't have the latest Windows nightlys, meaning they don't have TLS 1.3 support.

Being tied to a windows version for TLS support isn't great; besides using a different TLS implementation, there any workarounds for using schannel without updating windows, right? Mostly because linux doesn't have this problem.

My goal is to get OpenSSL working both on Windows and Linux. That will allow us to run tests on Azure Pipelines.

* squash callback fn changes * Add server client test (#2) * rebase test to fn branch * enable server client tests on windows * enable manual trigger for cargo ci * add documenation for dropping handle

t's a race between PerfClientWorker::WorkerThread and TcpWorker::WorkerThread. In the middle of creating a socket from the perf client worker thread, as long as epoll has been told to monitor the socket, a notification could come up at the same time from tcp worker thread and frees the connection. #1 0x00005b7e577eec9f in CxPlatSocketContextSetEvents #2 0x00005b7e577ef725 in CxPlatSocketCreateTcpInternal #3 0x00005b7e577efb8a in SocketCreateTcp #4 0x00005b7e577ebdb9 in CxPlatSocketCreateTcp #5 0x00005b7e577db274 in TcpConnection::Start #6 0x00005b7e577def35 in PerfClientConnection::Initialize #7 0x00005b7e577df124 in PerfClientWorker::StartNewConnection #8 0x00005b7e577df140 in PerfClientWorker::WorkerThread The fix is moving the actual connection start to the tcp worker thread.

…back type (#5839) Issue: UndefinedBehaviorSanitizer: undefined-behavior ../../src/ext/msquic/src/platform/tls_openssl.c:1251:17 The TicketLength parameter was declared as uint16_t in the function definition but the CXPLAT_TLS_RECEIVE_TICKET_CALLBACK typedef specifies uint32_t. This mismatch caused an undefined behavior sanitizer error when the function was called through the callback pointer. The following is the stack: ``` /usr/bin/llvm-symbolizer-21: /usr/local/lib/libcurl.so.4: no version information available (required by /usr/bin/llvm-symbolizer-21) ../../src/ext/msquic/src/platform/tls_openssl.c:1251:17: runtime error: call to function QuicConnRecvResumptionTicket through pointer to incorrect function type 'unsigned char (*)(struct QUIC_CONNECTION *, unsigned int, const unsigned char *)' /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:2113: note: QuicConnRecvResumptionTicket defined here #0 0x780a958641e1 in CxPlatTlsOnClientSessionTicketReceived /build/out/msquic-build/../../src/ext/msquic/src/platform/tls_openssl.c:1251:17 #1 0x780a958b2ae1 in ssl_update_cache (/build/out/msquic/lib/libmsquic.so.2+0x4b2ae1) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #2 0x780a958e21fd in tls_process_new_session_ticket (/build/out/msquic/lib/libmsquic.so.2+0x4e21fd) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #3 0x780a958dd1d7 in state_machine statem.c #4 0x780a958c9ca0 in ssl3_read_bytes (/build/out/msquic/lib/libmsquic.so.2+0x4c9ca0) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #5 0x780a958a4eb5 in ssl3_read_internal s3_lib.c #6 0x780a958afebe in SSL_read (/build/out/msquic/lib/libmsquic.so.2+0x4afebe) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #7 0x780a95871fea in CxPlatTlsProcessData /build/out/msquic-build/../../src/ext/msquic/src/platform/tls_openssl.c:3319:9 #8 0x780a957678eb in QuicCryptoProcessData /build/out/msquic-build/../../src/ext/msquic/src/core/crypto.c:1959:9 #9 0x780a95772ca4 in QuicCryptoProcessFrame /build/out/msquic-build/../../src/ext/msquic/src/core/crypto.c:1347:14 #10 0x780a9573620b in QuicConnRecvFrames /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:4622:17 #11 0x780a957405ce in QuicConnRecvDatagramBatch /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:5604:20 #12 0x780a95743c88 in QuicConnRecvDatagrams /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:5848:9 #13 0x780a95745c40 in QuicConnFlushRecv /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:5958:5 #14 0x780a95759e77 in QuicConnDrainOperations /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:7880:18 #15 0x780a956cd8de in QuicWorkerProcessConnection /build/out/msquic-build/../../src/ext/msquic/src/core/worker.c:658:9 #16 0x780a956c708a in QuicWorkerLoop /build/out/msquic-build/../../src/ext/msquic/src/core/worker.c:881:9 #17 0x780a95852f36 in CxPlatRunExecutionContexts /build/out/msquic-build/../../src/ext/msquic/src/platform/platform_worker.c:667:18 #18 0x780a95853874 in CxPlatWorkerPoolWorkerPoll /build/out/msquic-build/../../src/ext/msquic/src/platform/platform_worker.c:707:5 #19 0x780a9563917c in MsQuicExecutionPoll /build/out/msquic-build/../../src/ext/msquic/src/core/library.c:2839:23 ... ... SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../src/ext/msquic/src/platform/tls_openssl.c:1251:17 ``` --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: leikong <11276068+leikong@users.noreply.github.com>

## Description Fixes memory leaks in `casted_clog_bytearray` when using stdout logging, caused by C's unspecified argument evaluation order. The `clog` macro expands to: ```c struct clog_param * __head = 0; clog_stdout(__head, fmt, casted_clog_bytearray(data, len, &__head)); ``` `casted_clog_bytearray` updates `__head` via the `&__head` pointer, but if the compiler evaluates `__head` (first argument) *before* calling `casted_clog_bytearray`, `clog_stdout` receives NULL for `head` and never frees the allocated memory. The fix changes `clog_stdout` to accept `struct clog_param **` (double pointer) instead of `struct clog_param *`. The macro now passes `&__head`, and `clog_stdout` dereferences it after function entry (when all arguments are fully evaluated), ensuring it always sees the correct linked list head. Changes: - **`src/inc/quic_trace.h`**: Updated both the extern declaration and stub inline definition of `clog_stdout` to use `struct clog_param **`. Updated the `clog` macro to pass `&__head` instead of `__head`. Added an inline comment explaining why the address-of pattern is necessary (the `CASTED_CLOG_BYTEARRAY` macro in `__VA_ARGS__` hardcodes the `__head` variable name and modifies it during argument evaluation). - **`src/generated/stdout/quic_trace.c`**: Updated `clog_stdout` implementation to accept `struct clog_param ** head_ptr` and dereference it on entry (`struct clog_param * head = *head_ptr;`). ## Testing Build verified with `pwsh ./scripts/build.ps1 -loggingType stdout -DisablePerf -DisableTools -Tls openssl -Clean -SanitizeAddress`. All targets compiled and linked without errors. Existing tests cover this path, and the fix is verified by ASAN no longer reporting leaks from `casted_clog_bytearray` allocations. ## Documentation No documentation impact.  <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>ASAN leaks in casted_clog_bytearray with stdout logging</issue_title> <issue_description>### Describe the bug All leaks are allocated in casted_clog_bytearray in [quic_trace.c](https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L27). This function allocates byte arrays for formatting trace output and they are supposed to be freed in function [clog_stdout(struct clog_param * head, const char * const format, ...)](https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L68) before exting: https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L119 The issue is that clog_stdout never gets the __head pointer value updated by casted_clog_bytearray. In the following code example: ``` struct clog_param * __head = 0; clog_stdout(__head, fmt, casted_clog_bytearray(data, len, &__head)); ``` casted_clog_bytearray updates __head via [`*head = param;`](https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L61)during argument evaluation and the updated __head is expected to be passed into clog_stdout, but this depends on argument evaluation order. When clog_stdout arguments are evaluated from left to right, __head is evaluated before the casted_clog_bytearray() call, so clog_stdout gets NULL `head' and skipped its cleanup loop — leaking all memory allocated in casted_clog_bytearray. The fix is to pass `&__head` to clog_stdout. Regardless of evaluation order, the value of this double pointer is the same. Inside clog_stdout, the pointer is dereferenced after function entry (when all arguments have been fully evaluated), so it always sees the correct linked list head in clog_stdout, thus will be able to free the memory properly. The leaks stop happening with the attached fix: [fix-clog-stdout-memory-leak.patch](https://github.com/user-attachments/files/25310421/fix-clog-stdout-memory-leak.patch) Sample ASAN reports: ``` Direct leak of 192 byte(s) in 12 object(s) allocated from: microsoft/msquic#0 0x58f5d92cc134 in malloc (/home/leikong/repo/meru-common/out/test/quic_transport.test/quic_transport.test+0x1c3134) (BuildId: 1aab74186928132fd9c6fed25feb4cbe93eed340) #1 0x7058b6a1a139 in CxPlatAlloc /home/leikong/repo/meru-common/ext/msquic/src/platform/platform_posix.c:285:12 #2 0x7058b6a4c435 in casted_clog_bytearray /home/leikong/repo/meru-common/ext/msquic/src/generated/stdout/quic_trace.c:32:9 #3 0x7058b6a407c6 in CxPlatSocketContextRecvComplete /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:1671:9 #4 0x7058b6a41e5a in CxPlatSocketReceiveCoalesced /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:1805:9 #5 0x7058b6a44075 in CxPlatSocketReceive /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:1984:13 #6 0x7058b6a37b6e in CxPlatSocketContextIoEventComplete /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:2606:17 #7 0x58f5d9bdfb9b in quicpp::execution::poll_loop_async(unsigned int) (.resume) /home/leikong/repo/meru-common/src/prod/quicpp/lib/execution.cpp:177:13 #8 0x58f5d93c5b63 in std::__n4861::coroutine_handle<void>::resume() const /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/coroutine:135:29 #9 0x58f5d93c56b3 in lnm::detail::scheduler_entry_base::resume(bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/../inc/task_scheduler/lnm/detail/scheduler_entry_base.h:81:12 #10 0x58f5d9f51550 in lnm::detail::scheduler_entry<unsigned long, std::pair<unsigned int, bool>, true>::resume(std::pair<unsigned int, bool>&&, bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/../inc/task_scheduler/lnm/detail/scheduler_entry.h:93:31 #11 0x58f5d9f4df81 in lnm::detail::worker_thread::dispatch_poll(lnm::detail::scheduler_entry<unsigned long, std::pair<unsigned int, bool>, true>*, unsigned int, bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_thread.cpp:1876:12 #12 0x58f5d9f4d4a4 in lnm::detail::worker_thread::handle_poll_event(epoll_event const&) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_thread.cpp:1546:9 #13 0x58f5d9f4847a in lnm::detail::worker_thread::dispatch_poll(bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_thread.cpp:1463:9 #14 0x58f5d9f4b41c in lnm::detail::worker_thread::dispatch_poll() /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_t... </details>  - Fixes #5790  --- ✨ Let Copilot coding agent [set things up for you](https://github.com/microsoft/msquic/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: anrossi <41394064+anrossi@users.noreply.github.com> Co-authored-by: mtfriesen <3517159+mtfriesen@users.noreply.github.com>

…teger underflow leads to heap buffer overflow name it shell to hell or eternal blue 2.0 Vulnerability #2: ACK Frame Integer Underflow - **CRITICAL/EXPLOITABLE** Integer underflow vulnerability** exists in ACK frame processing. The validation check for `Count > Largest` uses the previous iteration's `Count` value, allowing an attacker to craft an ACK block that causes integer underflow. Fix: Verify Count is lesser than Largest in every iteration. If verification fails, mark InvalidFrame and fail the operation. Test: Repro using POC and validate fix confirming that it resolves the issue. ---- #### AI description (iteration 1) #### PR Classification Bug fix that addresses a critical security vulnerability by adding a validation check in the frame decoding logic. #### PR Summary This pull request fixes an integer underflow issue in the frame decoding function which could lead to a heap buffer overflow, a potential remote code execution vulnerability. - `src/core/frame.c`: Added a check to ensure that the computed count does not exceed the permissible limit, marking the frame as invalid and exiting early if the condition is met.

nibanks assigned jkotalik, maolson-msft, anrossi and sourad Nov 2, 2019

cmake and cross platform googletest support

bb296c7

nibanks force-pushed the pr/gtest branch from 460aca2 to bb296c7 Compare November 3, 2019 20:41