Re: [Nbd] NBD passwords (was Re: nbd-server working easily in cygwin in XP)

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

Hi,

On Sun, Aug 24, 2008 at 03:34:50PM -0700, ulmo@...205... wrote:
> Following the maintainer's outline of what to do, I made a patch to the
> latest SVN, attached.  You can download it applied at:
> 
> ftp://ftp.armory.com/pub/user/ulmo/nbd/nbd-new.tar.gz

Whoa. Thanks! I did intend to implement this at some later point, but
you beat me to it. By far!

I still have a few questions, though:

> Note that since it is autoconf raw, you may need to grab a cooked set of
> autoconf files (*.in, config.*, and some other junk).
> 
> You have to write the client and server passwords to nbd-client's standard
> input, one line at a time.

I'm thinking it'd be nice if there'd be a command-line option to specify
a file containing the passwords; otherwise, this would make it damn hard
to script nbd-client.

Also: why two passwords? I must admit that I hadn't thought of
authenticating the server, which obviously makes sense and is the right
thing to do; but why can't we reuse the same password for both auth
passes?

> I also made a small patch to that patch that uses urandom at the end of
> random reads, so it goes faster, but is less secure.

Mm. Perhaps this should be made a config option: allow the user to
choose at runtime, rather than compile time.

[...]
> +#ifdef NBD_AUTH
> +	/* Prepare data for following section. */
> +
> +	memset(clipass,0,NBD_AUTHSZ);
> +	memset(srvpass,0,NBD_AUTHSZ);
> +	fprintf(stdout,"enter client password:");	/* stty -echo */

Looks like you forgot to actually add that stty :-)

> +	fgets(clipass, NBD_AUTHSZ, stdin);clipass[strlen(clipass)-1]='\0';
> +
> +	if (getpeername(sock, &peeraddr, &peeraddrlen) < 0) {
> +		err("Negotiation failed 2: %m");
> +		exit(62);				/* exit required to stop bugs */

Which bugs would that be?

> +	}
> +	if (getsockname(sock, &sockaddr, &sockaddrlen) < 0) {
> +		err("Negotiation failed 3: %m");
> +		exit(63);				/* exit required to stop bugs */
> +	}
> +
> +	/* Get random from server, both apply server & client ips &
> +	ports & families & client password, both digest, send back to
> +	server for them to compare. */
> +
> +	if (read(sock, auth, (size_t)NBD_AUTHSZ) < 0) {
> +		err("Negotiation failed 4: %m");
> +		exit(64);				/* exit required to stop bugs */
> +	}
> +	SHA512_Init(&hashcontext);
> +	SHA512_Update(&hashcontext, (unsigned char*)auth, NBD_AUTHSZ);
> +	SHA512_Update(&hashcontext, (unsigned char*)&peeraddr, sizeof(peeraddr));
> +	SHA512_Update(&hashcontext, (unsigned char*)&sockaddr, sizeof(sockaddr));
> +	SHA512_Update(&hashcontext, (unsigned char*)clipass, strlen(clipass));
> +	SHA512_Final(clidigest,&hashcontext);
> +#ifdef DODBGBYPASS

Looks like you forgot to rename something here, too (either that, or
remove it)

[...]
> +		err("BAD SERVER PASSWORD!!!!!!!!");
> +		exit(71);			/* exit required for bug security */
> +	}
> +#ifdef DODBG
> +	fprintf(stderr,"Server password succeeded, and before that, client, too, most likely.\n");
> +#endif /* DODBG */
> +#else /* NBD_AUTH */
>  	if (read(sock, buf, 8) < 0)
>  		err("Failed/1: %m");
>  	if (strlen(buf)==0)
>  		err("Server closed connection");
> -	if (strcmp(buf, INIT_PASSWD))
> +	if (strncmp(buf, INIT_PASSWD,strlen(INIT_PASSWD)))

Hmm, so you're replacing the INIT_PASSWD code with the SHA512 password
code you've implemented. I don't think that's a very good idea, since it
changes the protocol incompatibly. I was actually thinking of not
touching the INIT_PASSWD, but rather set an extra flag in the 'reserved'
field, signalling that the client and the server understand
authentication; they could then do the authentication itself immediately
thereafter. This would also allow one compilation of a client and a
server to serve/use both authenticated and non-authenticated exports.

The name INIT_PASSWD is a bit of a misnomer, anyway; it's not a
password, it's a protocol banner, like '220 <hostname> ESMTP <software name>'
for ESMTP, 'SSH-<protocol version>-<software name>' for SSH, etc.

>  		err("INIT_PASSWD bad");
> +#endif /* NBD_AUTH */
>  	printf(".");
>  	if (read(sock, &magic, sizeof(magic)) < 0)
>  		err("Failed/2: %m");
> diff -x'*~' -Nrup nbd/nbd-server.c nbd-new/nbd-server.c
> --- nbd/nbd-server.c	2008-08-24 09:42:13.000000000 -0700
> +++ nbd-new/nbd-server.c	2008-08-24 14:42:50.000000000 -0700
> @@ -173,6 +173,9 @@ typedef struct {
>  	off_t expected_size; /**< size of the exported file as it was told to
>  			       us through configuration */
>  	gchar* listenaddr;   /**< The IP address we're listening on */
> +#ifdef NBD_AUTH
> +	char *srvpass,*clipass;
> +#endif /* NBD_AUTH */
>  	unsigned int port;   /**< port we're exporting this file at */
>  	char* authname;      /**< filename of the authorization file */
>  	int flags;           /**< flags associated with this exported file */
> @@ -367,7 +370,7 @@ void dump_section(SERVER* serve, gchar* 
>  		printf("\tcopyonwrite = true\n");
>  	}
>  	if(serve->expected_size) {
> -		printf("\tfilesize = %Ld\n", (long long int)serve->expected_size);
> +		printf("\tfilesize = %lld\n", (long long int)serve->expected_size);

Ah, good catch :)

That's about it. Thanks again for this patch; and sorry for being a bit
picky about it.

-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22