[Dnsmasq-discuss] [PATCH] Error: SECURE_CODING

[Dave Reisner]

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.

> 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