[zeromq-dev] Binding to TCP port 0
Martin Lucina
martin at lucina.net
Sun Feb 12 09:39:43 CET 2012
Hi Ian!
Thanks for the patch!
You asked me to review it, so here goes...
This is based on https://github.com/zeromq/libzmq/pull/238.diff as of
today; there doesn't seem to be a better way to uniquely identify a Github
pull request's *content* since it can change all the time :-(
> diff --git a/src/ipc_listener.cpp b/src/ipc_listener.cpp
> index 07a7dff..6797344 100644
> --- a/src/ipc_listener.cpp
> +++ b/src/ipc_listener.cpp
> @@ -95,8 +95,31 @@ void zmq::ipc_listener_t::in_event ()
> send_attach (session, engine, false);
> }
>
> +int zmq::ipc_listener_t::get_address (unsigned char *addr, size_t *len)
Coding style: Please use addr_ and len_ for function parameter names.
> +{
> + struct sockaddr_un sun;
> + int rc;
> +
> + // Get the details of the IPC socket
> + socklen_t sl = sizeof(sockaddr_un);
> + rc = getsockname (s, (sockaddr *)&sun, &sl);
> + if (rc != 0) {
> + return rc;
> + }
> + // Store the address for retrieval by users using wildcards
> + *len = sprintf((char *)addr, "ipc://%s", sun.sun_path);
> +
> + return 0;
> +}
> +
The use of sprintf() is a security hole if the user allocated not enough
space at *addr. Please use snprintf() to ensure a maximum of len bytes
(including the string terminator) are written to *addr.
> int zmq::ipc_listener_t::set_address (const char *addr_)
> {
> +
> + // Allow wildcard file
> + if(*addr_ == '*') {
> + addr_ = tempnam(NULL, NULL);
> + }
> +
> // Get rid of the file associated with the UNIX domain socket that
> // may have been left behind by the previous run of the application.
> ::unlink (addr_);
> @@ -124,7 +147,7 @@ int zmq::ipc_listener_t::set_address (const char *addr_)
> rc = listen (s, options.backlog);
> if (rc != 0)
> return -1;
> -
> +
> return 0;
> }
The use of tempnam() is a potential security hole, although we don't make
any guarantees about untrusted local users. I'm not sure if there is an
equivalent of mkstemp() for UNIX domain sockets, can't remember off the top
of my head.
> diff --git a/src/ipc_listener.hpp b/src/ipc_listener.hpp
> index e1f4817..57e04ef 100644
> --- a/src/ipc_listener.hpp
> +++ b/src/ipc_listener.hpp
> @@ -48,6 +48,9 @@
>
> // Set address to listen on.
> int set_address (const char *addr_);
> +
> + // Get the bound address for use with wildcards
> + int get_address(unsigned char *addr, size_t *len);
Coding style: Use addr_ and len_.
>
> private:
>
> diff --git a/src/options.cpp b/src/options.cpp
> index 4db1a6c..c8790a8 100644
> --- a/src/options.cpp
> +++ b/src/options.cpp
> @@ -30,6 +30,7 @@
> rcvhwm (1000),
> affinity (0),
> identity_size (0),
> + last_endpoint_size(0),
> rate (100),
> recovery_ivl (10000),
> multicast_hops (1),
> @@ -213,7 +214,6 @@ int zmq::options_t::setsockopt (int option_, const void *optval_,
> ipv4only = val;
> return 0;
> }
> -
> }
>
> errno = EINVAL;
> @@ -385,7 +385,15 @@ int zmq::options_t::getsockopt (int option_, void *optval_, size_t *optvallen_)
> *((int*) optval_) = ipv4only;
> *optvallen_ = sizeof (int);
> return 0;
> -
> +
> + case ZMQ_LAST_ENDPOINT:
> + if (*optvallen_ < last_endpoint_size) {
> + errno = EINVAL;
> + return -1;
> + }
> + memcpy (optval_, last_endpoint, last_endpoint_size);
> + *optvallen_ = last_endpoint_size;
> + return 0;
> }
Security hole: You want to copy last_endpoint_size bytes, or optvallen_
bytes, whichever is lesser, and return the actual # of bytes copied in
*optvallen_.
>
> errno = EINVAL;
> diff --git a/src/options.hpp b/src/options.hpp
> index bfc9dc7..7feea95 100644
> --- a/src/options.hpp
> +++ b/src/options.hpp
> @@ -46,6 +46,10 @@
> // Socket identity
> unsigned char identity_size;
> unsigned char identity [256];
> +
> + // Last socket endpoint URI
> + unsigned char last_endpoint [256];
> + size_t last_endpoint_size;
I guess you wanted to use the ZMQ_ENDPOINT_MAX define here rather than
hardcoding 256?
>
> // Maximum tranfer rate [kb/s]. Default 100kb/s.
> int rate;
> diff --git a/src/socket_base.cpp b/src/socket_base.cpp
> index 3761b46..74c807f 100644
> --- a/src/socket_base.cpp
> +++ b/src/socket_base.cpp
> @@ -161,6 +161,7 @@ int zmq::socket_base_t::parse_uri (const char *uri_,
> }
> protocol_ = uri.substr (0, pos);
> address_ = uri.substr (pos + 3);
> +
> if (protocol_.empty () || address_.empty ()) {
> errno = EINVAL;
> return -1;
> @@ -340,6 +341,8 @@ int zmq::socket_base_t::bind (const char *addr_)
> delete listener;
> return -1;
> }
> +
> + rc = listener->get_address (options.last_endpoint, &(options.last_endpoint_size));
> launch_child (listener);
> return 0;
> }
> @@ -354,6 +357,7 @@ int zmq::socket_base_t::bind (const char *addr_)
> delete listener;
> return -1;
> }
> + rc = listener->get_address (options.last_endpoint, &(options.last_endpoint_size));
> launch_child (listener);
> return 0;
> }
> diff --git a/src/tcp_address.cpp b/src/tcp_address.cpp
> index de6e0ad..9fe6083 100644
> --- a/src/tcp_address.cpp
> +++ b/src/tcp_address.cpp
> @@ -387,11 +387,18 @@ int zmq::tcp_address_t::resolve (const char *name_, bool local_, bool ipv4only_)
> addr_str [addr_str.size () - 1] == ']')
> addr_str = addr_str.substr (1, addr_str.size () - 2);
>
> - // Parse the port number (0 is not a valid port).
> - uint16_t port = (uint16_t) atoi (port_str.c_str());
> - if (port == 0) {
> - errno = EINVAL;
> - return -1;
> + uint16_t port;
> + // Allow 0 specifically, to detect invalid port error in atoi if not
> + if (port_str[0] == '*' || port_str[0] == '0') {
> + // Resolve wildcard to 0 to allow autoselection of port
> + port = 0;
> + } else {
> + // Parse the port number (0 is not a valid port).
> + port = (uint16_t) atoi (port_str.c_str());
> + if (port == 0) {
> + errno = EINVAL;
> + return -1;
> + }
> }
I'm not sure I understand what the business with "Allow 0 specifically" is
about?
>
> // Resolve the IP address.
> diff --git a/src/tcp_listener.cpp b/src/tcp_listener.cpp
> index 0b7a90d..191e05c 100644
> --- a/src/tcp_listener.cpp
> +++ b/src/tcp_listener.cpp
> @@ -119,6 +119,37 @@ void zmq::tcp_listener_t::close ()
> s = retired_fd;
> }
>
> +int zmq::tcp_listener_t::get_address (unsigned char *addr, size_t *len)
Coding style: Please use addr_ and len_.
> +{
> + struct sockaddr sa;
> + char host[INET6_ADDRSTRLEN];
> + int port, rc;
> +
> + // Get the details of the TCP socket
> + socklen_t sl = sizeof(sockaddr);
> + rc = getsockname (s, &sa, &sl);
> + if (rc != 0) {
> + return rc;
> + }
> +
> + // Split the retrieval between IPv4 and v6 addresses
> + if ( sa.sa_family == AF_INET ) {
> + inet_ntop(AF_INET, &(((struct sockaddr_in *)&sa)->sin_addr), host, INET6_ADDRSTRLEN);
> + port = ntohs( ((struct sockaddr_in *)&sa)->sin_port);
> +
> + // Store the address for retrieval by users using wildcards
> + *len = sprintf((char *)addr, "tcp://%s:%d", host, port);
> + } else {
> + inet_ntop(AF_INET6, &(((struct sockaddr_in6 *)&sa)->sin6_addr), host, INET6_ADDRSTRLEN);
> + port = ntohs( ((struct sockaddr_in6 *)&sa)->sin6_port);
> +
> + // Store the address for retrieval by users using wildcards
> + *len = sprintf((char *)*addr, "tcp://[%s]:%d", host, port);
> + }
> +
> + return 0;
> +}
Security hole: Please use snprintf(). (See above)
> +
> int zmq::tcp_listener_t::set_address (const char *addr_)
> {
> // Convert the textual address into address structure.
> @@ -168,6 +199,7 @@ int zmq::tcp_listener_t::set_address (const char *addr_)
> errno_assert (rc == 0);
> #endif
>
> +
> // Bind the socket to the network interface and port.
> rc = bind (s, address.addr (), address.addrlen ());
> #ifdef ZMQ_HAVE_WINDOWS
> diff --git a/src/tcp_listener.hpp b/src/tcp_listener.hpp
> index c2116b3..fc6ebd1 100644
> --- a/src/tcp_listener.hpp
> +++ b/src/tcp_listener.hpp
> @@ -44,6 +44,9 @@
>
> // Set address to listen on.
> int set_address (const char *addr_);
> +
> + // Get the bound address for use with wildcard
> + int get_address(unsigned char *addr, size_t *len);
Coding style: Please use addr_ and len_.
One more quick comment about this API: It actually won't work for the most
common TCP case (based on what people are asking for), which is:
bind("tcp://*:*")
The problem is that getsockname() will return INADDR_ANY or IN6ADDR_ANY for
such a socket, and such an address is not something you can *connect* to.
If the ZMQ_LAST_ENDPOINT API is to be theoretically sound, what guarantees
are we providing to the caller, exactly? If the guarantee is "Return an
endpoint I can connect to", then you have a problem with INADDR_ANY.
I don't see how this can be solved. Any ideas? Off the top of my head
trawling through all local interface IP addresses and returning a list of
endpoints but that rapidly becomes pretty horrible.
<philosophy>
We (Martin Sustrik and myself) have always tried to design the ZeroMQ APIs
to behave consistently and give explicit guarantees which can be applied to
all transports/patterns where possible.
In my opinion this is a major win for libzmq, and is an often misunderstood
aspect of the library. You don't notice it because it "just works". Many
times, the reason something has *not* been implemented is that we'd rather
have no implementation than a theoretically unsound one.
</philosophy>
Cheers,
-mato
More information about the zeromq-dev
mailing list