wolfSSL WOLFSSL_CALLBACKS Heap Buffer Over-Read

# wolfSSL before 5.5.2: Heap-buffer over-read with WOLFSSL_CALLBACKS====================================================================## INFO=======The CVE project has assigned the id CVE-2022-42905 to this issue.Severity: 9.1 CRITICALAffected version: before 5.5.2End of embargo: Ended October 28, 2022Blog Post: https://blog.trailofbits.com/2023/01/12/wolfssl-vulnerabilities-tlspuffin-fuzzing-ssh/## SUMMARY==========If wolfSSL callback functions are enabled (i.e., the flag `WOLFSSL_CALLBACKS` isenabled), then a malicious client or network attacker can send a Client Hellomessage to a server that when parsed by the server will trigger a bufferover-read on the heap of at least 5 bytes. Similarly, a malicious server or anetwork attacker can send a Hello Retry Request message to a client that whenparsed by the client will trigger a buffer over-read on the heap of at least 15bytes.The `AddPacketInfo` is given a buffer that should be the input buffer and thatactually is shifted by 5 bytes on the left, i.e., instead of reading`input[0]..input[length]`, the function will read `input[-5]..input[length]` andstore it in a buffer that is exposed through the wolfSSL API. Note that `input`is stored on the heap and `input[-5]` to `input[-1]` might store sensitive datathat should not be given to `AddPacketInfo`, for example when callback functionsare used as logging facility (through the API functions `wolfSSL_accept_ex` and`wolfSSL_connect_ex`).This buffer over-read can be triggered at a server with a single Client Hellomessage. We have confirmed this with a proof-of-concept test case given below onwolfSSL 5.5.0, on the version from the master branch, and on version 5.4.0. Asimilar buffer over-read can be triggered at a client with the same wolfSSLversions.## DETAILS==========(Note: All code snippets and line numbers are with respect to the git hash`#43715d1bb5b8c5b8b18cba4be3171fd1dd7eb046` on remote`[email protected]:wolfssl/wolfssl.git`.)When executing the first proof of concept test case given below, we reach a callto `DoTls13HandShakeMsgType` from `tls13.c:DoTls13HandShakeMsg:10443` when theserver parses the Client Hello message, with the following values:```csize = 16520;totalSz = 16524;*inOutIdx = 4;type = 1;input; // buffer containing the input to be processed, before input,there seems to be another input stored````*inOutIdx=4` because of the call to `GetHandshakeHeader` at line`tls13.c:10411`.We enter the function `tls13.c:DoTls13HandShakeMsgType` and because`WOLFSSL_CALLBACKS` flag is defined, we enter this snippet:```c#if defined(WOLFSSL_CALLBACKS)/* add name later, add on record and handshake header part back on */if (ssl->toInfoOn) {int add = RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ;AddPacketInfo(ssl, 0, handshake, input + *inOutIdx - add,size + add, READ_PROTO, ssl->heap);AddLateRecordHeader(&ssl->curRL, &ssl->timeoutInfo);}#endif````tls13.c:DoTls13HandShakeMsgType:10094`We have that `RECORD_HEADER_SZ = 5` and `HANDSHAKE_HEADER_SZ = 4` so `add = 9`while `*inOutIdx` is still set to `4`. Therefore, the pointer indicating thebeginning of the alleged input that is given to the `AddPacketInfo` function(argument `data`) is `data = input + *inOutIdx - add = input - 5`. This bufferstarts 5 bytes before the actual input buffer. The argument `sz = size + add` isthe total length of the input, which is too long here `16529` instead of `16524`bytes.It seems the snippet above makes a wrong assumption about the past increases of`*inOutIdx`, which has not been increased by `RECORD_HEADER_SZ` as no recordheader has been parsed yet.Next, in `internal.c:AddPacketInfo:25153`, see the snippet:```c/* add data, put in buffer if bigger than static buffer */info->packets[info->numberPackets].valueSz = sz;if (sz < MAX_VALUE_SZ)XMEMCPY(info->packets[info->numberPackets].value, data, sz);else {info->packets[info->numberPackets].bufferValue = (byte*)XMALLOC(sz,heap, DYNAMIC_TYPE_INFO);if (!info->packets[info->numberPackets].bufferValue)/* let next alloc catch, just don't fill, not fatal here */info->packets[info->numberPackets].valueSz = 0;elseXMEMCPY(info->packets[info->numberPackets].bufferValue, data, sz);}````internal.c:AddPacketInfo:25169`and recall that `sz = 16529`, `data = input - 5`. The last line will write`input[-5]..input[16524]` into `info->packets[0]`. Recall that`info->packets[0]` equals `ssl->timeoutInfo->packets[0]`.It turns out `ssl->timeoutInfo` and in particular the content of`ssl->timeoutInfo->packets[0]` is exposed to the wolfSSL user through thefunctions `wolfSSL_connect_ex` and `wolfSSL_accept_ex` whose declarations are asfollows (where `typedef int (*TimeoutCallBack)(TimeoutInfo*);` from `ssl.h`);see also the[wolfSSL documentation](https://www.wolfssl.com/documentation/manuals/wolfssl/chapter06.html).Therefore, in addition to the over-read per se, this might be exploited toexfiltrate sensitive data, depending on the use case.## POTENTIAL EXPLOITATION=========================We found no systematic exploit using this bug but it could be exploited in somespecific use cases. We note that this problem only occurs when`WOLFSSL_CALLBACKS` is enabled, but we found no documentation forbidding the useof this flag in production.For instance, a wolfSSL server could be configured to use the API function`wolfSSL_connect_ex` with an argument `srvTimeoutCB`. This argument could be afunction that logs on disk the content of `timeoutInfo->packets` when theincoming message has a packet name (packetName) "ClientHello". The rationalebeing that logging Client Hello messages should not expose sensitive and secretinformation on disk (assuming logs are not well-protected). Because of this bug,the log will contain extra data from the heap that do not correspond to thereceived Client Hello message and that could be sensitive data.Here is a threat model for which this could be a problem:- attacker has partial access to the architecture, for example log files (logfiles could be stored at rest and because considered less sensitive, they areless secured),- wolfSSL server is set up to use callback functions to log some allegedlynon-sensitive information, for example it logs the received packets (let ussay client hello for now) that were rejected by the server for futureinspection. But it does not log full handshake.Then the attacker can trigger at will the bug to write in the log some bytes ofthe heap, it could theoretically fine-tune the timing to exfiltrate data ofinterest.## POTENTIAL REMEDIATION========================First, the use of `WOLFSSL_CALLBACKS` could be vehemently discouraged inproduction. Second, the flawed logic of `tls13.c:DoTls13HandShakeMsgType`exposed above should be fixed. The addition of `RECORD_HEADER_SZ` to `add`should be conditionded by whether a record layer header was processed or not.In `internal.c:AddPacketInfo` and for a similar input message without a recordheader, the lines below call `ssl->protoMsgCb` on a (right) shifted inputbuffer:```cssl->protoMsgCb(written, version, type,(const void *)(data + RECORD_HEADER_SZ),(size_t)(sz - RECORD_HEADER_SZ),ssl, ssl->protoMsgCtx);```This does not trigger a buffer overflow but yields inconsistent log messages.