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

[Martin Schwenke]

On Fri, 18 Mar 2016 08:57:11 -0700, Jeremy Allison <jra at samba.org>
wrote:

> 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:
> > >   
>  [...]  
>  [...]  
> > > 
> > > 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.   :-(

> > 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));  

Sure.  It ends up being fairly arbitrary because for some strange
reason "name" has 2 extra characters:

  struct ctdb_iface {
	char name[CTDB_IFACE_SIZE+2];
	uint16_t link_state;
	uint32_t references;
  };

So we never really want to use the last character!

Some day we'll change that but that will also change the wire format.
We should do that when we switch to a new protocol with auto-generated
marshalling code and we try to add proper protocol versioning.  :-)

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

Sure.  The original patches I posted will never unnecessarily truncate
and are safe (because of the talloc_zero()).    However, if the
standard way of making strncpy(3) usable, with explicit termination, is
clearer then please use the attached patches.

In theory, if you switch the order of the patches (done in attached
version) then any stupid style of copy is safe because it will never
overflow.  Famous last words...  ;-)

So, updated patches attached.  Please review and maybe push...

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

Yeah, we'll get there.  Some of the code in the ctdb CLI tool (written
by me) uses strcpy(3) (perhaps even technically incorrectly) because it
is simple and obvious. There is a buffer declared, there are a finite
number of strings declared nearby that can be concatenated into the
buffer.  It will fit.  You'd have to be trying hard to change
the code so it actually breaks and overflows the buffer.  More famous
last words!  I'll take another look at my patch for it on Monday and
probably post it... and then Amitay will throw it all away when he
finally gets time to apply the final tweaks to his replacement...  :-)

peace & happiness,
martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb.patch
Type: text/x-patch
Size: 2583 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160319/146ff2c1/ctdb.bin>