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

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

On Mon, Aug 25, 2008 at 01:20:09PM -0700, Brad Allen wrote:
[...]
> " 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.
> 
> I agree.  I was in a hurry.

Sure, no problem.

> I appended an incomplete todo list to this email, notes basically for
> myself to read.

Right.

> " 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'm not an expert at security, I just think about it in decent
> measures.  To me it is obvious that the server needs to authenticate
> to the client, because we don't want someone to pretend to be the
> server.

Yes, though there's a limit on what the server could accomplish by being
evil.

> Perhaps I've had too many people tell me they were police when they
> were trying to mug me (they always failed, but that's not the point).
> I don't trust the "higher up" until they show me reason to trust them
> (at which point I make a decision that based upon least problems, most
> quality, and usually try to go with decent systems in place).  This
> goes with servers as well.

Good strategy.

> Anyway, since I am new to writing password hashing code (this was my
> first, and I just followed your procedure list rather than researching
> it myself), I thought about the ordering of events that happens, and
> don't know techniques that will work better.  Here are the issues:
> when the client sends its hash back to the server with the password in
> it, the server would then have a way to .. wait, I was worried that
> when you send the password to the server, it would just accept it,
> then send the password it just got from the client back to the client.
> I guess the theory is that that is hard to do.

Yes, indeed.

The whole point of secure hash functions (like SHA-512, though that is
rather computationally intensive, but that's okay for our purposes) is
that they're one-way; given a particular set of data it's easy to
compute the hash, but the other way around is practically infeasible
(with a good hash function, you'd need infinite processing time to
accomplish that through a brute-force attack).

> I was thinking the hash was just a way to stop evesdroppers from
> listening in and fabricating the passwords, but I suppose it also
> could be utilized to prevent the peer (server in this case) from
> knowing what password was in the hash without foreknowledge of it.

Indeed.

> Still, with a sufficiently low quality password, this would allow a
> rather simple brute-force attack from the server to corrupt the client
> if they shared the same password.

In theory, yes. In practice, not likely.

Note that even a one-character change to the data set will radically
alter the resulting hash; so it's even impossible to detect whether or
not you are 'close' to the right result.

A 'pure' brute-force will therefore most likely not work, although a
dictionary attack of course could (but it's not like we can defend
against that in the algorithm).

> Ok, I've just decided.  Is it correct that in this scenereo, I could
> just re-use the password, and allow them to be equal?

I would think so.

> If so, I'll
> recode it to allow either one or two passwords.  If only one password
> is supplied, it is used for both.  If two passwords are supplied, then
> one is for server and one is for client.  I'll have to think of an easy
> way to configure that without much fuss or muss.

Wouldn't it be easier to just forget about the two-password case...?

If you really have to use two passwords to be secure, just cut the
password in two in a predictable location; then use one half as the
server password, and the other half as the client one.

Actually, it's even easier. What we're using to authenticate is:

- A random number, supplied by one of the parties;
- The remote IP address and port number;
- The password

If, instead, we change it to the following:
- Two random numbers, one each chosen by each of the sides;
- The IP addresses of and port numbers of both parties;
- The password

then make sure we compute the hash twice--once with the client data
specified first and once with the server data being specified first--and
have the client send one of the hashes and the server the other, then we
should be secure.

> " > 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.
> 
> I agree.
> 
> " [...]
> " > +#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?
> 
> I've never seen the "err" code.  That means I ought to just write my
> own equivilent function that does the same thing, perhaps just reusing
> the err code, something like "security_err(...)" with that exit in it.
> If the err code returns and there's no exit in there, then the
> system's security gets bypassed.

err() is defined in cliserv.h (yeah, I know, should probably be a .c
file). It always, unconditionally, ends with exit(1); it has an
__attribute__((__noreturn__)) assigned to it for that reason.

> " > +	}
> " > +	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)
> 
> It was the last bug I had that I fixed:  since I was in a hurry, my
> sloppy hack for inputting passwords in nbd-client bit me:  fgets puts
> a '\n' at the end of the strings :)  I was busy diagnosing it for like
> 2 hours.

Heh :-)

Yeah, those can be sucky.

> " [...]
> " > +		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.
> 
> Excellent point.  I will contingent it upon whether or not the
> password(s) exist.  If there are passwords, then security is invoked.

Exactly.

Might also want to have an option to nbd-client to _require_
authentication from the server.

> I'll keep the protocol the same.
> 
> " 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.
> 
> I might end up renaming it a bit, to avoid confusion.

Yeah; I'd been planning to do that for a while, but never actually
bothered.

> " >  		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.
> 
> Not at all.  It's a work in progress, but I know it's already useful,
> and it would take us all a bit less time to have it progress in an
> orderly manner rather with a bit of feedback (like that INIT_PASSWD
> comment you pointed out) than I go make a big can of worms and submit
> it like that.

True.

> Brad Allen
> 
> " -- 
> " <Lo-lan-do> Home is where you have to wash the dishes.
> "   -- #debian-devel, Freenode, 2004-09-22
[...]
> This is a limited todo list, with a few man page and google notes, for
> me, not for you to bother with.  The annotations alone took me a few
> hours to produce; my first implementation didn't need to be concerned
> with that, but certainly fixing it up does.
> 
> - Better diagnostic output.  Mostly that it is taking a while to
>   read from /dev/random.  Use stderr.

Perhaps this could be mitigated by not reading too much data from
/dev/random. We need enough entropy so that the chance of the same
numbers (ports, IP addresses, random numbers) being assigned to one
connection is low enough so that an attacker cannot reasonably guess the
number. While you'd need more than a few bytes of randomness to
accomplish that, I'm thinking 0x100 (i.e., 4K) is much more than is
required; I'm thinking 256 bytes is probably more than enough.

> - Check portability of using /dev/random.  I could read other
>   projects' code for this.
[...]
> - Put in terminal echo code (off for passwords, then on again),
>   when appropriate.
>   linux termios ECHO
>   #include <termios.h>
>   #include <unistd.h>
>        int tcgetattr(int fd, struct termios *termios_p);
>        int tcsetattr(int fd, int optional_actions, struct termios *termios_p);
>   Idea is to getattr, save that (make copy), setattr of no ECHO
>   (~ECHO), then after done, setattr to original copy of original
>   settings (restore to what it was).  If getting password, should do
>   echo off as early as possible, so anybody pre-entering it can benefit.
>   tcflag_t c_iflag;      /* input modes */tcflag_t c_oflag;      /* output modes */
>   so, struct termios tios_dat tios_datmod;tcgetattr(inputfd,&tios_dat);memcpy(tios_datmod,tios_dat,sizeof(tios_datmod);
>   tios_datmod->c_oflag~=ECHO;tcsetattr(inputfd,TCSANOW,&tios_datmod);
>   ...read input...
>   tcsetattr(inputfd,TCSANOW /* TCSADRAIN */,&tios_dat); /* Not sure TCSANOW vs. TCSADRAIN, since we already have it and it's input related too */
> - Test #define NBD_AUTH on and off.
> - Test NBD_AUTH in big-endian systems.  I did not put the configure
>   hooks into the SHA512 code.

My laptop is a powerpc, and I'm a Debian m68k porter. If something
breaks because you could only test in little endian, I'll find out :-)

> - Find out if you prefer non-BSD code.

As long as whatever you provide me is GPLv2-compatible, I don't really
care :-)

> - Audit the SHA512 code.  I grabbed it from the web, but it looked
>   ok, and it created the same SHA512 output as "sha512sum" from Linux
>   tools package, which was enough for me to do a proof of concept
>   coding with it..  It also looked like really simple code, rather
>   than a convoluted interface with big third party libraries.
> - Test on cygwin.  I know, I ought to be the first to test it, but
>   that'll be in the next few days, probably after I do some other
>   work on it first, like some portability issues.
> - Fix portability.  I like having FAMILY for security purposes,
>   and IP and PORT are necessary, but put all that in portable format.
>   Probably the *ntoa* type functions ought to make canonical output in
>   strings for ip addresses with little difficulty in the code.
>   Throughout NBD convert inet_ntoa to inet_ntop if there are any
>   ntoa's left around:
>        inet_ntop(3) extends the inet_ntoa(3) function to support
>        multiple address families, inet_ntoa(3) is now considered to be
>        deprecated in favor of inet_ntop(3).  The following address
>        families are currently supported:
>        AF_INET
>               src points to a struct in_addr (network byte order
>               format) which is converted to an IPv4 network address in
>               the quad format, "ddd.ddd.ddd.ddd".  The buffer dst must
>               be at least INET_ADDRSTRLEN bytes long.
>        AF_INET6
>               src points to a struct in6_addr (network byte order
>               format) which is converted to a representation of this
>               address in the most appropriate IPv6 network address
>               format for this address.  The buffer dst must be at
>               least INET6_ADDRSTRLEN bytes long.

I've had 'support for IPv6 sockets' on my TODO list since, eh, well...

http://bugs.debian.org/382189

... 2006. Hmm, I thought it was longer than that :-)

Anyway, if you plan to work on that, it'd be very welcome.

>   Come to think of it, I want to use network byte order, and the
>   information is already in network byte order in the structs.
>   What I can do is just create my own family assignments that
>   equal Linux's using a quick lookup table, and then for ip# and
>   port# I can just use the existant values inside the structs.
>   That sucks, and requires family-dependent code, but sigh it is
>   nicer in output, because it avoids bugs like this:
>   BUGS
>        AF_INET6 converts mapped IPv4 addresses into an IPv6 format.IPv6
> - Along with fix portability, do the same for IPv6.  Perhaps have
>   to code the whole thing for IPv6 anyway.  See previous point.
> - Alternate password file for config.  Perhaps use "authfile",
>   the same one that has the ip#s in it.  That would require a bit
>   of coding.  I'm not sure it's worth it.

I'm not sure either. Just specifying it in the config file should work;
if people are concerned about readability of that file, changing file
permissions should do it.

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