nccgroup / SocksOverRDP

Socks5/4/4a Proxy support for Remote Desktop Protocol / Terminal Services / Citrix / XenApp / XenDesktop
https://research.nccgroup.com/2020/05/06/tool-release-socks-over-rdp/
MIT License
1.07k stars 168 forks source link

Build fails because buffer overflow is detected #6

Closed v-p-b closed 3 years ago

v-p-b commented 3 years ago

Compiling the server project results in the following error in VS:

error C4789: buffer 'answer2' of size 309 bytes will be overrun; 309 bytes will be written starting at offset 9

This is the relevant code:

        char    null[20], *answer, answer2[300 + sizeof(DWORD) + sizeof(DWORD) + 1];
    DWORD   dwWritten, ret;

    struct threads *pta;

    answer = answer2 + sizeof(DWORD) + sizeof(DWORD) + 1;
    memset(answer, 0, 300 + sizeof(DWORD) + sizeof(DWORD) + 1);

https://github.com/nccgroup/SocksOverRDP/blob/master/SocksOverRDP-Server/SocksServer.cpp#L184

While answer is pointed inside the answer2 buffer, memset writes the full size of the answer2 buffer.

I'm not sure about the intention here, but based on the initialization of answer, I assume that only 300 bytes should be set here (first two pointers and one byte remains intact at the beginning of the buffer).

earthquake commented 3 years ago

Based on a quick review I think that was a mistake and it should be memset(answer2, 0, 300 + sizeof(DWORD) + sizeof(DWORD) + 1);

*answer is only used as a pointer there. Can you please compile like that and get back to me. Will check and fix ASAP but I think with testing could take up a few days.

Thanks for the bug report.

v-p-b commented 3 years ago

Makes sense, and also the compilation succeeds this way.

earthquake commented 3 years ago

First 9 bytes were reserved for header that contained the thread ID, length of packet and end of transmission flag. That is why it was there, and it seemed the wrong variable was zero'd.

Fixed, committed and pushed, now considered closed until the next bug :) https://github.com/nccgroup/SocksOverRDP/commit/1c9dee27f3d1274254f82388d6fb1f70d1c9bf9d