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  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/

Powered by blists - more mailing lists