[Dnsmasq-discuss] [PATCH] Error: SECURE_CODING

[Tomas Hozza]

----- Original Message -----
> On Tue, Apr 23, 2013 at 03:55:11PM +0200, Tomas Hozza wrote:
> > Coverity output:
> > dnsmasq-2.66/src/ipset.c:173: secure_coding: [VERY RISKY]. Using
> > "strcpy" can cause a buffer overflow when done incorrectly.  If the
> > destination string of a strcpy() is not large enough then anything might
> > happen. Use strncpy() instead.
> > 
> > I checked the code path and the length is never checked so there
> > should be strncpy used.
> 
> But it *is* checked. Just above the chunk that your patch references is
> the line:
> 
>   if (strlen(setname) >= sizeof(req_adt_get.set.name))
> 
> There's an off by one error here, but it's not as bad as Coverity
> claims. On the other hand, one might point out that if we're taking the
> length of setname here, you might as well optimize and use memcpy
> instead of strncpy.

You are right. I somehow overlooked it. It is OK then.

Regard,

Tomas Hozza

> > Signed-off-by: Tomas Hozza <thozza at redhat.com>
> > ---
> >  src/ipset.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipset.c b/src/ipset.c
> > index f175fa4..fa262d5 100644
> > --- a/src/ipset.c
> > +++ b/src/ipset.c
> > @@ -170,7 +170,8 @@ static int old_add_to_ipset(const char *setname, const
> > struct all_addr *ipaddr,
> >    
> >    req_adt_get.op = 0x10;
> >    req_adt_get.version = 3;
> > -  strcpy(req_adt_get.set.name, setname);
> > +  strncpy(req_adt_get.set.name, setname, IPSET_MAXNAMELEN - 1);
> > +  req_adt_get.set.name[IPSET_MAXNAMELEN - 1] = '\0';
> >    size = sizeof(req_adt_get);
> >    if (getsockopt(ipset_sock, SOL_IP, 83, &req_adt_get, &size) < 0)
> >      return -1;
> > --
> > 1.8.1.4
> > 
> > 
> > _______________________________________________
> > Dnsmasq-discuss mailing list
> > Dnsmasq-discuss at lists.thekelleys.org.uk
> > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
>