TODO: Handle over-sized message decently (nmap)

[thijsterlouw]

Current 2.0.9 version of ZeroMQ does not handle illegal messages correctly. In the current master there is still a zmq_assert in the decoder, so might also have this issue?

Expected: ignore invalid messages, close connection. Do not terminate.

bool zmq::zmq_decoder_t::one_byte_size_ready ()
{
//  First byte of size is read. If it is 0xff read 8-byte size.
//  Otherwise allocate the buffer for message data and read the
//  message data into it.
if (*tmpbuf == 0xff)
    next_step (tmpbuf, 8, &zmq_decoder_t::eight_byte_size_ready);
else {

    //  TODO:  Handle over-sized message decently.

    //  There has to be at least one byte (the flags) in the message).
    zmq_assert (*tmpbuf > 0);

    //  in_progress is initialised at this point so in theory we should
    //  close it before calling zmq_msg_init_size, however, it's a 0-byte
    //  message and thus we can treat it as uninitialised...
    int rc = zmq_msg_init_size (&in_progress, *tmpbuf - 1);
    errno_assert (rc == 0);
    next_step (tmpbuf, 1, &zmq_decoder_t::flags_ready);
}
return true;
}

This is an issue when you for example run an "nmap" portscan over a ZeroMQ socket. It will crash your server with the following exit message:

Assertion failed: *tmpbuf > 0 (zmq_decoder.cpp:60)
Aborted (core dumped)

[thijsterlouw]

    if(*tmpbuf <= 0)   //patched by thijs to prevent nmap crash?
    {
        return false;
    }
    //zmq_assert (*tmpbuf > 0);

seems to work ok

[mato]

We know about this; neither the 2.0.x nor the 2.1.x series have been through a proper audit to make them "Internet ready", i.e. suitable for deploying on potentially untrusted networks.

This will be done systematically in the near future once I have some free time to look at it.

[thijsterlouw]

I use it in a trusted environment, but my company uses internal nmap portscans, so I still hope this particular assert() can be removed as soon as possible.

[thijsterlouw]

I noticed the 2.0.10 branch was released without this fix. I understand you want to systematically check all code later, but perhaps this can be first fixed for the 2.0.11 release? An nmap to crash your server is not fun :)

[sustrik]

The real solution is to appear in 2.1.x rather than in 2.0.x series. The problem is that detecting the poisonous data on the connection should close down the connection decently. Closing the connection means dealing with 0MQ shutdown system. 0MQ shutdown system is the most complex part of 0MQ. And by complex I mean really complex. The shutdown system have been completely rewritten for 2.1 to make it somehow less complex and more systematic. Thus, fixing the problem in both 2.0.x and 2.1.x would mean doing the whole work (implementation, testing, debugging etc.) twice.

[sustrik]

Ok. There's a fix in master for the issue. Please, check whether it works as expected and if so, close the issue.

[thijsterlouw]

I just tested it and it's indeed fixed, thanks a lot!