The patch looks good to me.

va_start() needs to be called before any use of CHECK_LIMIT because the
code-path if the check fails always calls va_end().

THe two rfc1035.c patches you refer to are cleanups for problems without
security implications. They are in this commit for the mainline:

http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=6a0b00f0d66490e074323d04782e75c4ba8a3f8d

I'd certainly omit them from a backport.

Cheers,

Simon.


On 10/10/17 07:24, Daniel Steglich wrote:
> Hi,
> 
> we have some legacy devices in the field which makes use of dnsmasq.
> Of course our recent products will get the update to dnsmasq 2.78 but in 
> order to provide a minimal patch to fix the latest CVE's we backported the 
> security patch to 2.57.
> 
> The legacy devices I'm talking about are not even IPv6 capable, so I excluded 
> the IPv6 (DHCPv6/SLAAC) related CVE's and only created a backport patch for 
> the CVE-2017-14491.
> 
> Recently I saw a patch popping up for debian wheezy where dnsmasq 2.62 is 
> still in use.
> The version we are using in our legacy products is 2.57 and therefore is 
> pretty close to the debian version.
> 
> I already compared our backport patch with the debian one and it looks pretty 
> similar. But I'm wondering if there is any source for such security related 
> backport patches for dnsmasq.
> 
> If anybody wants to use this backport patch feel free to do so. If anybody 
> could comment on the patch and tell me if I missed something, I would 
> appreciate.
> 
> Please note that the patch proposed by google initially also contains two 
> more changes at rfc1035.c:
> 
> diff -u a/rfc1035.c b/rfc1035.c
> --- a/rfc1035.c       2017-09-13 12:25:39.971673852 -0700
> +++ b/rfc1035.c       2017-09-13 12:21:07.275689168 -0700
> @@ -37,7 +37,7 @@
>       /* end marker */
>       {
>         /* check that there are the correct no of bytes after the name */
> -       if (!CHECK_LEN(header, p, plen, extrabytes))
> +       if (!CHECK_LEN(header, p1 ? p1 : p, plen, extrabytes))
>           return 0;
> 
>         if (isExtract)
> @@ -498,6 +498,8 @@
>           {
>             unsigned int i, len = *p1;
>             unsigned char *p2 = p1;
> +           if ((p1 + len - p) >= rdlen)
> +             return 0; /* bad packet */
>             /* make counted string zero-term  and sanitise */
> 
>             for (i = 0; i < len; i++)
>               {
> 
> Also the patch does not propose to move the va_start() function call up. I 
> adapted our backport to this changes after I had a look into the debian 
> patches because I saw the debian patch was supplied
> by Simon Kelly and he probably knows his code better then me.
> 
> Cheers,
> 
> Daniel
> 
> 
> 
> 
> From cc0e1d567766bd8aed7e4c66272546b6f54c09ab Mon Sep 17 00:00:00 2001
> From: Daniel Steglich <daniel.stegl...@sphairon.com>
> Date: Mon, 9 Oct 2017 15:41:58 +0200
> Subject: [PATCH] Backport of debian patch for CVE-2017-14491
> 
> Backported debian patch for CVE-2017-14491 to be able to apply
> this patch on dnsmasq version 2.57.
> ---
>  src/dnsmasq.h |  2 +-
>  src/rfc1035.c | 42 ++++++++++++++++++++++++++++++---------
>  src/rfc2131.c |  4 ++--
>  src/util.c    |  8 +++++++-
>  4 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 76eab3f..ceb35bb 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -775,7 +775,7 @@ void rand_init(void);
>  unsigned short rand16(void);
>  int legal_hostname(char *c);
>  char *canonicalise(char *s, int *nomem);
> -unsigned char *do_rfc1035_name(unsigned char *p, char *sval);
> +unsigned char *do_rfc1035_name(unsigned char *p, char *sval, char *limit);
>  void *safe_malloc(size_t size);
>  void safe_pipe(int *fd, int read_noblock);
>  void *whine_malloc(size_t size);
> diff --git a/src/rfc1035.c b/src/rfc1035.c
> index 889c1f0..47e68fe 100644
> --- a/src/rfc1035.c
> +++ b/src/rfc1035.c
> @@ -1185,9 +1185,21 @@ static int add_resource_record(struct dns_header 
> *header, char *limit, int *trun
>    long lval;
>    char *sval;
> 
> +#define CHECK_LIMIT(size) \
> +  if (limit && p + (size) > (unsigned char*)limit) \
> +    { \
> +      va_end(ap); \
> +      goto truncated; \
> +    }
> +
>    if (truncp && *truncp)
>      return 0;
> 
> +  va_start(ap, format);   /* make ap point to 1st unamed argument */
> +
> +  /* nameoffset (1 or 2) + type (2) + class (2) + ttl (4) + 0 (2) */
> +  CHECK_LIMIT(12);
> +
>    PUTSHORT(nameoffset | 0xc000, p);
>    PUTSHORT(type, p);
>    PUTSHORT(class, p);
> @@ -1196,13 +1208,12 @@ static int add_resource_record(struct dns_header 
> *header, char *limit, int *trun
>    sav = p;              /* Save pointer to RDLength field */
>    PUTSHORT(0, p);       /* Placeholder RDLength */
> 
> -  va_start(ap, format);   /* make ap point to 1st unamed argument */
> -
>    for (; *format; format++)
>      switch (*format)
>        {
>  #ifdef HAVE_IPV6
>        case '6':
> +        CHECK_LIMIT(IN6ADDRSZ);
>       sval = va_arg(ap, char *);
>       memcpy(p, sval, IN6ADDRSZ);
>       p += IN6ADDRSZ;
> @@ -1210,31 +1221,41 @@ static int add_resource_record(struct dns_header 
> *header, char *limit, int *trun
>  #endif
>       
>        case '4':
> +        CHECK_LIMIT(INADDRSZ);
>       sval = va_arg(ap, char *);
>       memcpy(p, sval, INADDRSZ);
>       p += INADDRSZ;
>       break;
>       
>        case 's':
> +        CHECK_LIMIT(2);
>       usval = va_arg(ap, int);
>       PUTSHORT(usval, p);
>       break;
>       
>        case 'l':
> +        CHECK_LIMIT(4);
>       lval = va_arg(ap, long);
>       PUTLONG(lval, p);
>       break;
>       
>        case 'd':
> -     /* get domain-name answer arg and store it in RDATA field */
> -     if (offset)
> -       *offset = p - (unsigned char *)header;
> -     p = do_rfc1035_name(p, va_arg(ap, char *));
> -     *p++ = 0;
> +        /* get domain-name answer arg and store it in RDATA field */
> +        if (offset)
> +          *offset = p - (unsigned char *)header;
> +        p = do_rfc1035_name(p, va_arg(ap, char *), limit);
> +        if (!p)
> +          {
> +            va_end(ap);
> +            goto truncated;
> +          }
> +        CHECK_LIMIT(1);
> +        *p++ = 0;
>       break;
>       
>        case 't':
>       usval = va_arg(ap, int);
> +        CHECK_LIMIT(usval);
>       sval = va_arg(ap, char *);
>       memcpy(p, sval, usval);
>       p += usval;
> @@ -1245,20 +1266,23 @@ static int add_resource_record(struct dns_header 
> *header, char *limit, int *trun
>       usval = sval ? strlen(sval) : 0;
>       if (usval > 255)
>         usval = 255;
> +        CHECK_LIMIT(usval + 1);
>       *p++ = (unsigned char)usval;
>       memcpy(p, sval, usval);
>       p += usval;
>       break;
>        }
> -
> +#undef CHECK_LIMIT
>    va_end(ap);        /* clean up variable argument pointer */
> 
>    j = p - sav - 2;
> -  PUTSHORT(j, sav);     /* Now, store real RDLength */
> + /* this has already been checked against limit before */
> + PUTSHORT(j, sav);     /* Now, store real RDLength */
> 
>    /* check for overflow of buffer */
>    if (limit && ((unsigned char *)limit - p) < 0)
>      {
> +truncated:
>        if (truncp)
>       *truncp = 1;
>        return 0;
> diff --git a/src/rfc2131.c b/src/rfc2131.c
> index e8e6562..68ce1b6 100644
> --- a/src/rfc2131.c
> +++ b/src/rfc2131.c
> @@ -2329,9 +2329,9 @@ static void do_options(struct dhcp_context *context,
> 
>             if (fqdn_flags & 0x04)
>               {
> -               p = do_rfc1035_name(p, hostname);
> +               p = do_rfc1035_name(p, hostname, NULL);
>                 if (domain)
> -                 p = do_rfc1035_name(p, domain);
> +                 p = do_rfc1035_name(p, domain, NULL);
>                 *p++ = 0;
>               }
>             else
> diff --git a/src/util.c b/src/util.c
> index e64f1a6..8384423 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -202,15 +202,21 @@ char *canonicalise(char *in, int *nomem)
>    return ret;
>  }
> 
> -unsigned char *do_rfc1035_name(unsigned char *p, char *sval)
> +unsigned char *do_rfc1035_name(unsigned char *p, char *sval, char *limit)
>  {
>    int j;
> 
>    while (sval && *sval)
>      {
> +      if (limit && p + 1 > (unsigned char*)limit)
> +        return p;
>        unsigned char *cp = p++;
>        for (j = 0; *sval && (*sval != '.'); sval++, j++)
> +      {
> +     if (limit && p + 1 > (unsigned char*)limit)
> +       return p;
>       *p++ = *sval;
> +      }
>        *cp  = j;
>        if (*sval)
>       sval++;
> 
> 
> 
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
> 


Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to