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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 21 Sep 2014 14:43:41 +0200
From:	Dominik Paulus <dominik.paulus@....de>
To:	Max Vozeler <max@...terhof.net>
Cc:	Maximilian Eschenbacher <maximilian@...henbacher.email>,
	linux-kernel@...r.kernel.org, valentina.manea.m@...il.com,
	shuah.kh@...sung.com, gregkh@...uxfoundation.org,
	Dominik Paulus <dominik.paulus@....de>,
	Fjodor Schelichow <fjodor.schelichow@...mail.com>,
	Johannes Stadlinger <johannes.stadlinger@....de>,
	Tobias Polzer <tobias.polzer@....de>
Subject: Re: [PATCH 02/18] usbip: Add support for client authentication

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/