zeromq / zmqpp

0mq 'highlevel' C++ bindings
http://zeromq.github.io/zmqpp
Mozilla Public License 2.0
438 stars 195 forks source link

receive_raw buffer overflow #166

Open Takenbacon opened 8 years ago

Takenbacon commented 8 years ago

Hello, there is currently a buffer overflow issue with receive_raw. If we used the following example code then received a message with a size greater than the buffer's size, it will result in a buffer overflow of buf.

char buf[256]; size_t length; // Note: This is currently only used for returning the size of the message socket.receive_raw(buf, length);

I suggest two options for resolving this:

1) (Preferred) Change the implementation to use http://api.zeromq.org/4-1:zmq-recv which has a buffer size argument and will automatically truncate the rest of the message (which is even specified in zmqpp documentation for this function, however it does not work: https://github.com/zeromq/zmqpp/blob/develop/src/zmqpp/socket.hpp#L307)

2) Change the memcpy to a memmove

I have other minor complaints about how this function is implemented, but they are out of scope of this issue.

Takenbacon commented 8 years ago

Here is the version I am currently using with my application: https://github.com/Takenbacon/zmqpp/commit/34917b76fcf3e652f6c8d607675e1b161e41ab6f

Some notes about the changes:

bluca commented 8 years ago

While I am not familiar with zmqpp, in general I would recommend creating a new API call instead of breaking an existing one. Then, in time, the old one can be deprecated and eventually removed. This is the process we tend to follow with other libraries in the zeromq project, and it avoids creating problems for users and maintainers.

Takenbacon commented 8 years ago

@bluca I thought of that too, however I was honestly just matching the functionality to its own documentation, but yes you're right, I'll make a new API call and fix the invalid documentation. Any suggestions for the name of the new API call? I'm pretty bad at coming up with names.

benjamg commented 8 years ago

We could use receive_bytes or something along those lines. then just deprecate this one and encourage switching over.

mm120 commented 7 years ago

I've just hit this same issue. I would suggest fixing the receive_raw function to match it's documentation. This is the code that I have added: https://github.com/mm120/zmqpp/commit/398342519a98fe85644f4ddc3536160e8dcf3595

benjamg commented 7 years ago

I've merged that in as it doesn't break compatibility. It does leave us with the problem that we are silently ignoring the fact we are truncating messages, but I felt that was less of an issue than the potential buffer overflow.

This is what the documentation say, although I can't imagine why I thought that was more sane than reporting an error to the user, so I'm still of the mind we should add separate function that acts that way.