search

Lichtso / netLink

Socket and Networking Library using msgpack.org[C++11]
GNU General Public License v3.0
217 stars 48 forks source link

Buffer Overflow problems #30

Closed powertomato closed 6 years ago

powertomato commented 6 years ago

I've been investigating a memory issue in our application and found it was a buffer overflow originating from a MsgPack-socket in netlink.

I've created a gist to help illustrate it. It consists of the source code, that replicates the problem for me and the trace output of dr. memory: https://gist.github.com/powertomato/341a32754b1c8702a98c8f5f15534a9e

What the code does is sets the buffer-lengths to 2, then replies to the connecting client with a map in an array: (in JSON for readability) [{"fo":0}] which results in a segfault or "UNADDRESSABLE ACCESS" as dr. memory detects it. What's odd: sending a long string, doesn't produce the same result. If buffer length needs to be longer than the maximum encoded message, I'd expect it to fail in a more graceful manner.

So far I've only been able to replicate it when writing to the socket: if I send the same array with netcat to the test app: echo -e -n '\x91\x81\xa2fo\x00' | nc localhost 9998, it gets parsed correctly, no segfault or dr. memory error.

I'm using MSYS2 on windows, with gcc v7.3 and binutils v2.30

Lichtso commented 6 years ago

Thank you for your detailed bug report. I will look into it further after lunch. From what I can see so far, the problem comes from serialize / sending not deserialize / receiving.

powertomato commented 6 years ago

Thank you for the quick response. I've debugged a bit and created a patch for Socket.cpp, and attached it to the gist above. It fixes the error for me, however I'm not quite familiar with std::streambuf, so it might be wrong.

Lichtso commented 6 years ago

Ok, I think I fixed it and some other bugs too. The Socket::sync() call resized the output buffer (instead of bumping the pptr). This could lead to a situation where the output buffer grew bigger than the allocated memory.

You can try the newest commit on master and see if everything works or if there are more problems.

powertomato commented 6 years ago

Can confirm, the memory issue is fixed! Thanks for the fast reaction!

Lichtso commented 6 years ago

No problem, thank you too.