Re: [Nbd] Problems in negotiating modern handshake

[Wouter Verhelst <w@...112...>]

On Mon, May 16, 2011 at 10:21:12PM +0100, Alex Bligh wrote:
> There seems to be some non-trivial problems negotiating the handshake with
> modern negotiation.
> 
> 1. If negotiate() returns an error (i.e. NULL), the server SEGVs

Sigh. Crap.

This is a remote DoS. And a fairly trivial one, too; just use an export
name that nbd-server isn't exporting. Blam.

> 2. If an export name is specified, then flags etc. are not sent, it
>    simply returns from negotiate.
> 
> Aside from that, the error checking could be improved a little.
> 
> I've tweaked it here to do what I think it ought to do, but it now
> isn't negotiating. What I can't work out is how it was negotiating
> before.
> 
> Any ideas?
[...]
>  		for(i=0; i<servers->len; i++) {
>  			SERVER* serve = &(g_array_index(servers, SERVER, i));
>  			if(!strcmp(serve->servename, name)) {
> -				CLIENT* client = g_new0(CLIENT, 1);
> +				client = g_new0(CLIENT, 1);
>  				client->server = serve;
>  				client->exportsize = OFFT_MAX;
>  				client->net = net;
>  				client->modern = TRUE;
> -				free(name);
> -				return client;
> +				break;

You've removed a return statement here. Are you sure that's not the
problem?

>  			}
>  		}
>  		free(name);
> -		return NULL;
> +		if (!client) {
> +			err("Negotiation failed: bad export name");
> +			return NULL;
> +		}
>  	}
>  	/* common */
>  	size_host = htonll((u64)(client->exportsize));
> @@ -1824,7 +1843,8 @@ int serveloop(GArray* servers) {
>  					close(net);
>  					net=0;
>  				}
> -				serve = client->server;
> +				else
> +				  serve = client->server;

No, we should start the next loop iteration here; not doing so may cause
the problem to reappear in the future. I've patched it thus.

-- 
The volume of a pizza of thickness a and radius z can be described by
the following formula:

pi zz a