Repository navigation
Fix out of bounds reads affecting variable-length replies #19
Description
If the server sends back a malformed variable-length response, the GetReply function can read out of bounds as it currently trusts that the size field in the response is correct. I suppose you wouldn't really run into this normally, but I still think it's important to fix these kind of things if only for the sake of fault tolerance.
I considered adding bounds checks to the GetReply function directly, but that wouldn't fix the problem in the case that a reply after the bad variable-length reply is accessed first, since all of those would then be incorrectly relocated off the end of the buffer. So the only real solution is to check the whole buffer at the end of SendCommand, which is what I've gone with.
I must say I'm not terribly confident in the correctness of all the bounds checking logic, since it is quite brittle, so I recommend you take your time reviewing this one.
I've included one unrelated commit that cleans up some code in the C FFI that is now redundant. I also noticed that it seems to not be possible to call SendCommand multiple times on a single batch command, since the return_locations array is modified, so I've documented this limitation.
Branch: https://github.com/chaoticgd/pine/tree/vle_oob
Client to test the problem:
#include "pine.h"
int main(int argc, char *argv[]) {
PINE::PCSX2 ipc;
printf("**** MsgVersion **** \n");
try {
for (int i = 0; i < 5; i++) {
printf("%s\n", ipc.Version());
}
} catch (PINE::Shared::IPCStatus status) { printf("ERROR: %d\n", status); }
printf("**** MsgVersion batched **** \n");
try {
ipc.InitializeBatch();
for (int i = 0; i < 5; i++) {
ipc.Version<true>();
}
PINE::Shared::BatchCommand batch = ipc.FinalizeBatch();
ipc.SendCommand(batch);
for (int i = 0; i < 5; i++) {
printf("%s\n", ipc.GetReply<PINE::Shared::MsgVersion>(batch, i));
}
} catch (PINE::Shared::IPCStatus status) { printf("ERROR: %d\n", status); }
printf("**** MsgTitle **** \n");
try {
for (int i = 0; i < 5; i++) {
printf("%s\n", ipc.GetGameTitle());
}
} catch (PINE::Shared::IPCStatus status) { printf("ERROR: %d\n", status); }
printf("**** MsgTitle batched **** \n");
try {
ipc.InitializeBatch();
for (int i = 0; i < 5; i++) {
ipc.GetGameTitle<true>();
}
PINE::Shared::BatchCommand batch = ipc.FinalizeBatch();
ipc.SendCommand(batch);
for (int i = 0; i < 5; i++) {
printf("%s\n", ipc.GetReply<PINE::Shared::MsgTitle>(batch, i));
}
} catch (PINE::Shared::IPCStatus status) { printf("ERROR: %d\n", status); }
printf("**** Accessing relocated responses after bad string ****\n");
try {
ipc.InitializeBatch();
ipc.GetGameTitle<true>();
ipc.Read<uint32_t, true>(0x100000);
PINE::Shared::BatchCommand batch = ipc.FinalizeBatch();
ipc.SendCommand(batch);
printf("Read from memory: %u\n",
ipc.GetReply<PINE::Shared::MsgRead32>(batch, 1));
} catch (PINE::Shared::IPCStatus status) { printf("ERROR: %d\n", status); }
}
Example modifications for PCSX2 to turn it into a faulty server (in PINE.cpp):
case MsgVersion:
{
u32 size = 24;
if (!SafetyChecks(buf_cnt, 0, ret_cnt, size + 4, buf_size)) [[unlikely]]
goto error;
ToResultVector(ret_buffer, size, ret_cnt);
ret_cnt += 4;
memcpy(reinterpret_cast<char*>(&ret_buffer[ret_cnt]), "Legit PS6 Emu (WinForms)", 24);
ret_cnt += 24;
break;
}
...
case MsgTitle:
{
if (!VMManager::HasValidVM())
goto error;
const std::string gameName = VMManager::GetTitle(false);
const u32 size = gameName.size() + 1;
if (!SafetyChecks(buf_cnt, 0, ret_cnt, size + 4, buf_size)) [[unlikely]]
goto error;
ToResultVector(ret_buffer, 1073741824, ret_cnt);
ret_cnt += 4;
memcpy(&ret_buffer[ret_cnt], gameName.c_str(), size);
ret_cnt += size;
break;
}
Result before:
**** MsgVersion ****
Legit PS6 Emu (WinForms)�
Legit PS6 Emu (WinForms)q
Legit PS6 Emu (WinForms)Q
Legit PS6 Emu (WinForms)1
Legit PS6 Emu (WinForms)
**** MsgVersion batched ****
Legit PS6 Emu (WinForms)�
Legit PS6 Emu (WinForms)�
Legit PS6 Emu (WinForms)q
Legit PS6 Emu (WinForms)Q
Legit PS6 Emu (WinForms)1
**** MsgTitle ****
Segmentation fault (core dumped) ./build/client
Result after:
**** MsgVersion ****
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
**** MsgVersion batched ****
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
Legit PS6 Emu (WinForms)
**** MsgTitle ****
ERROR: 1
**** MsgTitle batched ****
ERROR: 1
**** Accessing relocated responses after bad string ****
ERROR: 1
Activity
[-]Fix out of bounds reads in GetReply function[/-][+]Fix out of bounds reads affecting variable-length replies[/+]