Re: [PATCH 02/18] usbip: Add support for client authentication

[Dominik Paulus <dominik.paulus@fau.de>]

Hi,

thanks for your feedback!

On Sun, Sep 21, 2014 at 02:42:59AM +0200, Max Vozeler wrote:
> > +	if (strcmp(username, "dummyuser"))
> > +		/* User invalid, stored dummy data in g and n. */
> > +		return 1;
> 
> Could you describe the role of "dummyuser" in a comment? It seems to be
> a placeholder for GnuTLS SRP requiring a username.

It is, SRP is designed for username/password authentication, but we only
do password.

> 
> > +	return 0;
> > +}
> > +
> > +int usbip_net_init_gnutls(void)
> > +{
> > +	int ret;
> > +
> > +	gnutls_global_init();
> > +
> > +	usbip_net_srp_salt.data = gnutls_malloc(16);
> 
> This should be freed somewhere.

Actually, it is used for the whole lifetime of the process.

> > +	/*
> > +	 * Process opcodes. We might receive more than one, as the
> > +	 * client might send STARTTLS first
> > +	 */
> > +	while (cont) {
> > +		uint16_t code = OP_UNSPEC;
> >  
> > -	if (ret == 0)
> > -		info("request %#0x(%d): complete", code, connfd);
> > -	else
> > -		info("request %#0x(%d): failed", code, connfd);
> > +		ret = usbip_net_recv_op_common(connfd, &code);
> > +		if (ret < 0) {
> > +			dbg("could not receive opcode: %#0x", code);
> > +			return -1;
> > +		}
> > +
> > +		info("received request: %#0x(%d)", code, connfd);
> > +
> > +		/* We require an authenticated encryption */
> > +		if (!auth && code != OP_REQ_STARTTLS) {
> > +			usbip_net_send_op_common(connfd, OP_REPLY, ST_NA);
> > +			return -1;
> > +		}
> > +
> > +		switch (code) {
> > +#ifdef HAVE_GNUTLS
> > +		case OP_REQ_STARTTLS:
> > +			if (!need_auth) {
> > +				ret = -1;
> > +				err("Unexpected TLS handshake attempt (client uses password, server doesn't)");
> > +			} else {
> > +				ret = net_srp_server_handshake(connfd);
> > +				if (ret != 0)
> > +					err("TLS handshake failed");
> > +				auth = 1;
> 
> This marks the connection as authenticated even if the handshake has
> failed, which doesn't seem right.
> 
> The err() used here is a macro that merely logs the error and moves
> on. It does not exit() like the err.h err() functions.
> 
> So this should probably be
> 
> > +				if (ret != 0)
> > +					err("TLS handshake failed");
> 				else
> > +					auth = 1;
> 
> instead?

It might be even better to just drop the connection after a failed
handshake, as existing clients aren't going to do more than one
authentication attempt, anyway.

Note that - after patch 11/18 has been applied, too -
net_srp_server_handshake() sets the "have_crypto" flag on the connection
handle, thus, all subsequent network operations are going to fail
anyway, as no TLS session has been established. Thus, the incorrectly
set "auth" flag doesn't have any security implications here.

Regards,
Dominik