[Dnsmasq-discuss] Backport fix for CVE-2017-14491 to dnsmasq v2.57
Simon Kelley
simon at thekelleys.org.uk
Tue Oct 10 22:29:48 BST 2017
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.steglich at 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 at lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20171010/2a5b1145/attachment.sig>
More information about the Dnsmasq-discuss
mailing list