[PATCH] Remove strcpy() from most Team-controlled code.

[Jeremy Allison]

On Fri, Mar 18, 2016 at 08:48:49PM +1100, Martin Schwenke wrote:
> On Fri, 18 Mar 2016 11:27:54 +1100, Martin Schwenke <martin at meltin.net>
> wrote:
> 
> > On Thu, 17 Mar 2016 13:03:24 -0700, Jeremy Allison <jra at samba.org>
> > wrote:
> > 
> > > On Thu, Mar 17, 2016 at 12:53:27PM -0700, Jeremy Allison wrote:  
> > > > Removes uses of strcpy() [...]
> > > > 
> > > > There are still some in ctdb I haven't gotten to
> > > > yet.  
> > 
> > I'll fix up the CTDB ones.
> > 
> > I've already looked at most of them.  1 is a real worry... but to
> > exploit it you need to be able to write to a configuration file.
> 
> Patch attached to fix the one in the daemon.  I also added a patch to
> sanity check length of interface names when they're setup (i.e. read
> from public addresses file or from command-line option).
> 
> Please review and maybe push...
> 
> I've done the ctdb CLI tool ones as well, but want to have another
> look.  The changes in the flag pretty printing code make the code
> unreadable.   :-(
> 
> peace & happiness,
> martin

> From 2598e39c28ba3d8d6ccd65f4505b73e582e0d376 Mon Sep 17 00:00:00 2001
> From: Martin Schwenke <martin at meltin.net>
> Date: Fri, 18 Mar 2016 11:49:49 +1100
> Subject: [PATCH 1/2] ctdb-daemon: Replace an unsafe strcpy(3) call
> 
> The buffer is allocated using talloc_zero_size() so the string will
> always be NUL-terminated.
> 
> Signed-off-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/server/ctdb_takeover.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/ctdb/server/ctdb_takeover.c b/ctdb/server/ctdb_takeover.c
> index 5fba46a..b367978 100644
> --- a/ctdb/server/ctdb_takeover.c
> +++ b/ctdb/server/ctdb_takeover.c
> @@ -2514,7 +2514,8 @@ int32_t ctdb_control_get_public_ip_info(struct ctdb_context *ctdb,
>  		if (vnn->iface == cur) {
>  			info->active_idx = i;
>  		}
> -		strncpy(info->ifaces[i].name, cur->name, sizeof(info->ifaces[i].name)-1);
> +		strncpy(info->ifaces[i].name, cur->name,
> +			sizeof(info->ifaces[i].name)-1);
>  		info->ifaces[i].link_state = cur->link_up;
>  		info->ifaces[i].references = cur->references;
>  	}
> @@ -2549,7 +2550,8 @@ int32_t ctdb_control_get_ifaces(struct ctdb_context *ctdb,
>  
>  	i = 0;
>  	for (cur=ctdb->ifaces;cur;cur=cur->next) {
> -		strcpy(ifaces->ifaces[i].name, cur->name);
> +		strncpy(ifaces->ifaces[i].name, cur->name,
> +			sizeof(ifaces->ifaces[i].name)-1);
>  		ifaces->ifaces[i].link_state = cur->link_up;
>  		ifaces->ifaces[i].references = cur->references;
>  		i++;
> -- 
> 2.7.0

Shouldn't the two strncpy's above be (strncpy takes the
max size, not max size - 1, but doesn't guarentee null
termination):

> +             strncpy(ifaces->ifaces[i].name, cur->name,
> +                     sizeof(ifaces->ifaces[i].name));

followed by explicit null termination:

ifaces->ifaces[i].name[sizeof(ifaces->ifaces[i].name)-1] = '\0';

I think that's the correct way to use this.

Following that, adding  a #define strcpy ERROR in ctdb
would be a good thing :-).