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

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

Hi Brad,

On Fri, Aug 29, 2008 at 02:37:29PM -0700, ulmo@...205... wrote:
> I worked a little on the password stuff.  It's far from done.
> 
> This is against the svn latest.  I tested it in Linux and Cygwin.
> 
> It's still not ENDIAN and non-Linux/Cygwin safe.
> 
> Note that I fixed the auth IP# stuff, which wasn't working right before.

Sorry for the late reply -- I was out on a Debian/m68k porters meeting,
and had little time for anything but that this weekend.

I've seen your other mail, but am on the train right now. I thought I'd
comment on this patch; if any of my comments are regarding something
that's been fixed in your new patch, feel free to disregard it.

> diff -x'*~' -x.svn -Nrup svn/auth.c nbd-new/auth.c
> --- svn/auth.c	1969-12-31 16:00:00.000000000 -0800
> +++ nbd-new/auth.c	2008-08-29 14:22:17.000000000 -0700
> @@ -0,0 +1,264 @@
> +#include <sys/socket.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <ctype.h>
> +#include "config.h"
> +#define NBD_AUTH_C 1	/* for cliserv.h */

That's one way to do it. You could of course also check for NBD_AUTH in
cliserv.h directly...

> +#include "cliserv.h"
> +#include "auth.h"
> +#include "sha2.h"
> +
> +#ifdef NBD_AUTH
> +
> +/* Debugging macros */
> +#ifdef DODBG
> +#define DEBUG( a ) fprintf(stderr, a )
> +#define DEBUG2( a,b ) fprintf(stderr, a,b )
> +#define DEBUG3( a,b,c ) fprintf(stderr, a,b,c )
> +#define DEBUG4( a,b,c,d ) fprintf(stderr, a,b,c,d )
> +void debug_hexdump(char *name, unsigned char *x,size_t len) {
> +	size_t i;
> +	for (i=0;i<len;++i) {
> +		fprintf(stderr, "%02X%c", x[i], isprint(x[i])?x[i]:'_');
> +	}
> +	fprintf(stderr, " %s\n",name);
> +}
> +#else /* DODBG */
> +#define DEBUG( a )
> +#define DEBUG2( a,b ) 
> +#define DEBUG3( a,b,c ) 
> +#define DEBUG4( a,b,c,d ) 
> +#define debug_hexdump(x)
> +#endif /* DODBG */

Maybe these should be moved out to a separate header file -- or to
cliserv.h. Whatever; in any case that's my problem, certainly not yours
:-)

> +#ifndef PACKAGE_VERSION
> +#define PACKAGE_VERSION ""
> +#endif
> +
> +#define HOSTNUMS_SPACESZ ((NI_MAXHOST+NI_MAXSERV*2)*2) /* socknums arg to socktonumbrs */
> +
> +#ifndef NI_MAXHOST
> +#define NI_MAXHOST      1025
> +#endif
> +#ifndef NI_MAXSERV
> +#define NI_MAXSERV      32
> +#endif
> +
> +/* I first implemented this with getnameinfo(), but since CYGWIN
> +   doesn't have that at all, I went back and used my preferred format,
> +   binary network byte order (NBO), after I finally 1learned enough to
> +   find all the include files and names to get at it; here's all I
> +   need to know: */
> +
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> +
> +/*
> +struct  in_addr sin_addr;	/ *  32 bit IPv4 address, NBO * /
> +struct in6_addr sin6_addr;	/ * 128 bit IPv6 address, NBO * /

Note that you cannot nest comments; i.e., the first line of the above
two is commented out, while the second and all the lines up to your
'final' end-of-comment marker below are not.

This certainly isn't something we like to see :-)

> +/ * Structure describing an Internet socket address.  * /
> +struct sockaddr_in {
> +    __SOCKADDR_COMMON (sin_);
> +    in_port_t sin_port;                 / * Port number.  * /
> +    struct in_addr sin_addr;            / * Internet address.  * /
> +
> +    / * Pad to size of `struct sockaddr'.  * /
> +    unsigned char sin_zero[sizeof (struct sockaddr) -
> +                           __SOCKADDR_COMMON_SIZE -
> +                           sizeof (in_port_t) -
> +                           sizeof (struct in_addr)];
> +  };
> +
> +/ * Ditto, for IPv6.  * /
> +struct sockaddr_in6 {
> +    __SOCKADDR_COMMON (sin6_);
> +    in_port_t sin6_port;        / * Transport layer port # * /
> +    uint32_t sin6_flowinfo;     / * IPv6 flow information * /
> +    struct in6_addr sin6_addr;  / * IPv6 address * /
> +    uint32_t sin6_scope_id;     / * IPv6 scope-id * /
> +  };
> +*/
> +
> +/* standardize sockaddr formats for us; needs to be portably canonical */
> +/* default to default, otherwise map for known families for portability */
> +#define	NBD_AF_INET	(2)
> +#define	NBD_AF_INET6	(10)
> +#ifdef AF_INET6
> +#define FamilyCanon(x)	(((x)==AF_INET)?NBD_AF_INET:(((x)==AF_INET6)? \
> +				NBD_AF_INET6:(x)))

I don't think it's necessary to also put the address family in the
checksum. An IPv4 address is, by definition, different from an IPv6
address; so it should be impossible for anyone to get the same has that
would result from an IPv6 address by using an IPv4 address.

At least, if that is not the case, then our hash algorithm is broken. If
you can indeed find an example where doing this is possible, by all
means report it :-)

The ability to remove these macros from the file would make it a bit
more readable, I think.

> +#define Addr6(y)	(((struct sockaddr_in6*)y)->sin6_addr)
> +#define Port6(y)	(((struct sockaddr_in6*)y)->sin6_port)
> +#else /* AF_INET6 */
> +#define FamilyCanon(x)	(((x)==AF_INET)?NBD_AF_INET:(x))
> +#endif /* AF_INET6 */
> +#define Family(y)	(((struct sockaddr*)y)->sa_family)
> +#define Addr4(y)	(((struct sockaddr_in*)y)->sin_addr)
> +#define Port4(y)	(((struct sockaddr_in*)y)->sin_port)

I prefer if you'd use something like

struct sockaddr_in* sa4 = (struct sockaddr_in*) sastg;
struct sockaddr_in6* sa6 = (struct sockaddr_in6*) sastg;

Then you can just reference them directly. The reason I prefer this is
that macros can be extremely hard to debug; so unless they significantly
improve the readability of a bit of code, I tend to avoid them (yes, I
know about SEND and ERROR in nbd-server -- I didn't write them, even if
that is a lame excuse).

Also, I don't care for non-modern operating systems. If something
doesn't support AF_INET6 in this day and age, well...

> +static void socktonum(struct sockaddr_storage *sastg, socklen_t sastg_len, unsigned char *dat, size_t *pos) {
> +	/* typedef unsigned short uint16_t; / * comment out if already defined */

uint16_t is part of C99, so should be defined everywhere. However, C99
does not specify that 'uint16_t' is always 'unsigned short'; all it
defines is sizeof(char) == 1 && sizeof(char) <= sizeof(short) <=
sizeof(int), etc. Giving an example like that is, thus, actually
dangerous for those people using a platform where a short is not two
bytes, exactly.

> +	uint16_t family;
> +
> +	/* Must include Family, IP#, AND PORT for proper security. */
> +
> +	family=htons(FamilyCanon(Family(sastg)));
> +	memcpy(dat+*pos,(void*)&family,sizeof(family));
> +	(*pos)+=sizeof(family);
> +
> +	switch(Family(sastg)) {
> +	    case AF_INET:
> +		memcpy(dat+*pos,(void*)&(Addr4(sastg)),	sizeof(Addr4(sastg)));

so this would then be

memcpy(dat+*pos,(void*)&(sa4->sin_addr), sizeof(sa4->sin_addr));

> +		(*pos)+=				sizeof(Addr4(sastg));
> +		memcpy(dat+*pos,(void*)&(Port4(sastg)),	sizeof(Port4(sastg)));
> +		(*pos)+=				sizeof(Port4(sastg));
> +		break;
> +#ifdef AF_INET6
> +	    case AF_INET6:
> +		memcpy(dat+*pos,(void*)&(Addr6(sastg)),	sizeof(Addr6(sastg)));
> +		(*pos)+=				sizeof(Addr6(sastg));
> +		memcpy(dat+*pos,(void*)&(Port6(sastg)),	sizeof(Port6(sastg)));
> +		(*pos)+=				sizeof(Port6(sastg));
> +		break;
> +#endif /* AF_INET6 */
> +	    default:
> +		fprintf(stderr, "Unimplemented family %u; trying binary data \
> +(probably will not work);\nEnhance auth.c socktonum() to handle your family \
> +& submit patch.\n", Family(sastg));
> +		memcpy( dat+*pos,(void*)sastg,	sastg_len);
> +		Family((dat+*pos))=family;
> +		(*pos)+=			sastg_len;
> +		break;
> +	}
> +}
> +
> +/* given socket and a buffer socknums of size HOSTNUMS_SPACESZ, returns
> +   length of buffer used for canonical identity information including
> +   family, address, and port */
> +static size_t socktonumbers(int sock,unsigned char *socknums,char who) {
> +	struct sockaddr_storage sastg;
> +	socklen_t sastg_len=sizeof(sastg);

This is unsafe in the case of AF_INET6. struct sockaddr_storage is in
fact _smaller_ than struct sockaddr_in6; the only reason why the API did
not need to be changed is that, in most cases, the API required a
socklen_t argument anyway, which meant that it could be compatibly
extended to also support AF_INET6. In the few cases where this was not
the case, new functions have been provided.

If you're going to say that the length of your storage element is
sizeof(struct sockaddr_storage), then you'll be asking for trouble.

> +	size_t pos=0; /* must be zero */
> +
> +#define GetSocketInfo(getfunc) { \
> +		if (getfunc(sock, (struct sockaddr*)&sastg, &sastg_len) < 0) \
> +			err("getsockname/getpeername %m"); \
> +	}
> +	if(who==NBD_WHO_SERVER)	GetSocketInfo(getsockname)
> +	else			GetSocketInfo(getpeername)
> +	socktonum(&sastg,sastg_len,socknums,&pos);
> +
> +	if(who==NBD_WHO_SERVER)	GetSocketInfo(getpeername)
> +	else			GetSocketInfo(getsockname)
> +	socktonum(&sastg,sastg_len,socknums,&pos);
> +
> +#undef GetSocketInfo
> +	return pos;
> +}
> +
> +static void nbd_authhash(uint8_t *digest, unsigned char *hashdat, size_t hashlen, char *pass, size_t passlen) {
> +	SHA512_CTX hashcontext;
> +
> +	SHA512_Init(&hashcontext);
> +	SHA512_Update(&hashcontext, hashdat, hashlen);
> +	SHA512_Update(&hashcontext, (unsigned char*)pass, passlen);
> +	SHA512_Final(digest,&hashcontext);
> +}

It may be easier to understand if this function were to return the
digest (in fact, my first comment said something along the lines of 'you
forgot to actually do something with the hash here').

> +void nbd_auth(int sock, char *clientpass, char seppass, char morerandom, char who) {
> +	int f;
> +	int r;
> +	int i;
> +	uint8_t mydigest[SHA512_DIGEST_LENGTH];
> +	uint8_t	theirdigest[SHA512_DIGEST_LENGTH];
> +	char *rand;
> +	char *serverpass=clientpass;
> +	size_t clientpasslen;
> +	size_t serverpasslen;
> +	size_t commonhashlen;
> +	unsigned char hashdat[NBD_AUTHSZ+HOSTNUMS_SPACESZ];
> +
> +	/* hash in order:  random, identity, password */
> +	commonhashlen=NBD_AUTHSZ+socktonumbers(sock,hashdat+NBD_AUTHSZ,who);
> +
> +	serverpasslen=clientpasslen=strlen(clientpass);
> +	if(seppass) {		/* be sure to use a very big clientpass */
> +		serverpasslen -= (clientpasslen /= 2);
> +		serverpass    +=  clientpasslen;
> +	}
> +
> +	if(who==NBD_WHO_CLIENT) goto getrandom;

Ewww. There are valid cases for goto, when it increases the readability
of code (e.g., for errorhandling). However, this is not one of them;
this is spaghetti code.

Please rework this.

> +sendrandom:
> +	/* Send random to other, both apply server & client families &
> +	ips & ports & password, both digest, get back from other and
> +	compare. */
> +
> +	if(morerandom)	{
> +		rand="/dev/random";	/* Flag for strong random */
> +		fprintf(stderr,
> +			"Reading from %s; this can take a while", rand);
> +	} else	rand="/dev/urandom";
> +	if((f=open(rand,O_RDONLY))==-1) { err("open rand: %m"); exit(54); }
> +	i=0; readloop:
> +	r=read(f,hashdat+i,NBD_AUTHSZ-i);
> +	if(r==-1) { err("read rand: %m"); exit(55); }
> +	i+=r;
> +	if(i<NBD_AUTHSZ) {
> +		fprintf(stderr, ".");
> +		goto readloop;
> +	}
> +	if(close(f)==-1) err("close rand: %m");
> +	if(morerandom) fprintf(stderr, " Done.\n");
> +
> +	if (write(sock, hashdat, (size_t)NBD_AUTHSZ) < 0)
> +		if(who==NBD_WHO_SERVER)
> +			err("auth write send: %m");
> +		else
> +			err("auth write send: %m; Check if nbd-server doesn't have passwords.");
> +
> +	nbd_authhash(mydigest, hashdat, commonhashlen,
> +	    (who==NBD_WHO_SERVER)?clientpass:serverpass,
> +	    (who==NBD_WHO_SERVER)?clientpasslen:serverpasslen);
> +
> +	if(read(sock, (unsigned char*)theirdigest,
> +	    (size_t)SHA512_DIGEST_LENGTH)!=SHA512_DIGEST_LENGTH)
> +		if (who==NBD_WHO_SERVER)
> +			err("Auth read back: %m; Check if nbd-client doesn't have passwords.");
> +		else
> +			err("Auth read back: %m; Check your passwords, \n\
> +and if you and nbd-server failed to set seppass the same.");
> +	if(memcmp(mydigest,theirdigest,(size_t)SHA512_DIGEST_LENGTH)) {
> +		err("Bad password!");
> +		exit(59);			/* just to be sure */
> +	}
> +	if(who==NBD_WHO_CLIENT) {
> +		printf("server password ok");
> +		goto end;
> +	}
> +
> +getrandom:
> +	/* Get random from other, both apply server & client families
> +	& ips & ports & password, both digest, send back to other for
> +	them to compare. */
> +
> +	if (read(sock, hashdat, (size_t)NBD_AUTHSZ) < 0)
> +		err("Auth read get: %m");
> +
> +	nbd_authhash(mydigest, hashdat, commonhashlen,
> +		(who==NBD_WHO_CLIENT)?clientpass:serverpass,
> +		(who==NBD_WHO_CLIENT)?clientpasslen:serverpasslen);
> +
> +	if(write(sock, (unsigned char*)mydigest, (size_t)SHA512_DIGEST_LENGTH)
> +	   !=SHA512_DIGEST_LENGTH) err("Auth write back: %m");
> +	if(who==NBD_WHO_CLIENT) goto sendrandom;
> +
> +	end:
> +	return;
> +}
> +
> +#endif /* NBD_AUTH */

If we're not going to be compiling in the auth code, it's probably best
to completely leave out the auth.c and sha2.c files, then. This is
pretty easy in automake/autoconf; if you don't know how, don't worry,
I'll do it before applying the patch :-)

> diff -x'*~' -x.svn -Nrup svn/auth.h nbd-new/auth.h
> --- svn/auth.h	1969-12-31 16:00:00.000000000 -0800
> +++ nbd-new/auth.h	2008-08-29 09:12:13.000000000 -0700
> @@ -0,0 +1,4 @@
> +#define NBD_AUTHSZ (0x100)	/* 0x100 = 256 */
> +#define NBD_WHO_SERVER (0)
> +#define NBD_WHO_CLIENT (1)
> +void nbd_auth(int sock, char *password, char twopasswd, char morerandom, char who);
> diff -x'*~' -x.svn -Nrup svn/cliserv.h nbd-new/cliserv.h
> --- svn/cliserv.h	2008-08-29 13:25:00.000000000 -0700
> +++ nbd-new/cliserv.h	2008-08-29 13:36:20.000000000 -0700
> @@ -1,14 +1,22 @@
> +#ifndef NBD_CLISERV_H
> +#define NBD_CLISERV_H 1
> +
>  /* This header file is shared by client & server. They really have
>   * something to share...
>   * */
>  
>  /* Client/server protocol is as follows:
> -   Send INIT_PASSWD
> +   Password authentication if specified
> +   Send NBD_HELLO

No, that's not what I meant to suggest earlier.

>     Send 64-bit cliserv_magic
>     Send 64-bit size of exported device
>     Send 128 bytes of zeros (reserved for future use)

This should have one extra bit set, signalling that password
authentication is to be used; see NBD_FLAG_READ_ONLY in the current
source code.

We'll probably be using something like

#define NBD_FLAG_AUTH (1 << 2)

Which, if set, triggers the authentication after negotiation.

This would make it possible to more gracefully error out when a bit is
set (or is not set) that one end of the communication knows nothing
about; when you change a field's meaning, you change the protocol
incompatibly.

I'm actually also thinking about splitting the 'reserved' field in two;
one half would be for required options (if the remote end of the
connection sees a bit set there that it knows nothing about, it aborts
the connection), while the other would be for optional flags (that can
be safely ignored). 128 bytes is more than enough, anyway.

>   */
>  
> +#include "config.h"
> +
> +#include "lfs.h"

nbd-client and nbd-server both include these files themselves before
this point. Also, the <> syntaxis is used for other things so that VPATH
builds work (automake adds '-I.' for that purpose).

>  #include <errno.h>
>  #include <string.h>
>  #include <netdb.h>
> @@ -51,11 +59,14 @@ typedef unsigned long long u64;
>  #define _FILE_OFFSET_BITS 64
>  #endif
>  
> +#ifndef NBD_AUTH_C
>  u64 cliserv_magic = 0x00420281861253LL;
> -#define INIT_PASSWD "NBDMAGIC"
> +#endif
> +#define NBD_HELLO "NBDMAGIC"
>  
>  #define INFO(a) do { } while(0)
>  
> +#ifndef NBD_AUTH_C
>  void setmysockopt(int sock) {
>  	int size = 1;
>  #if 0
> @@ -73,6 +84,7 @@ void setmysockopt(int sock) {
>  		 INFO("(no sockopt/3: %m)");
>  #endif
>  }
> +#endif /* NBD_AUTH_C */
>  
>  #ifndef G_GNUC_NORETURN
>  #if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
> @@ -82,6 +94,8 @@ void setmysockopt(int sock) {
>  #endif
>  #endif
>  
> +#ifndef NBD_AUTH_C
> +
>  void err(const char *s) G_GNUC_NORETURN;

Why the ifdef here?

>  void err(const char *s) {
> @@ -111,14 +125,6 @@ void err(const char *s) {
>  	exit(1);
>  }
>  
> -void logging(void) {
> -#ifdef ISSERVER
> -	openlog(MY_NAME, LOG_PID, LOG_DAEMON);
> -#endif
> -	setvbuf(stdout, NULL, _IONBF, 0);
> -	setvbuf(stderr, NULL, _IONBF, 0);
> -}

Yes -- I know I said it should probably be in a separate .c file, but
that doesn't mean it has to be copied to nbd-client.c and nbd-server.c
:-)

[rest skipped -- it's based on the protocol ordering as you changed
earlier, but not the way I want to see it]
-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22