xorl %eax, %eax

CVE-2009-1755: NSD off-by-one Overwrites

with 2 comments

These bugs were discovered by Ilja van Sprundel of IOActive and affect NSD 3.2.1 release and NSD 2.3.7. NSD is a popular open source name server implementation. The bug of 3.2.1 release is this:

2  * packet.c -- low-level DNS packet encoding and decoding functions.
 ...
259 int packet_read_query_section(buffer_type *packet,
260                         uint8_t* dst,
261                         uint16_t* qtype,
262                         uint16_t* qclass)
263 {
264         uint8_t *query_name = buffer_current(packet);
265         uint8_t *src = query_name;
 ...
268         while (*src) {
269                 /*
270                  * If we are out of buffer limits or we have a pointer
271                  * in question dname or the domain name is longer than
272                  * MAXDOMAINLEN ...
273                  */
274                 if ((*src & 0xc0) ||
275                     (src + *src + 1 > buffer_end(packet)) ||
276                     (src + *src + 1 > query_name + MAXDOMAINLEN))
277                 {
278                         return 0;
279                 }
280                 memcpy(dst, src, *src + 1);
281                 dst += *src + 1;
282                 src += *src + 1;
283         }
284         *dst++ = *src++;
 ...
298 }

This is a common byte per byte copy while loop. It keeps iterating and copying bytes as long as src (line 265) is not NULL (line 268) or character to be copied is \ff (line 274), or the character to be copied plus one is larger than the end of the buffer (line 275), or if the character to be copied plus one is larger than query_name plus the maximum allowed length (line 276). If this is not the case, it performs the copy of the next byte. However, when any of the above conditions is met another copy is being made (line 284). This clearly can lead to an off-by-one overwrite. To fix this, the following patch was applied:

 if ((*src & 0xc0) ||
-		    (src + *src + 1 > buffer_end(packet)) || 
-		    (src + *src + 1 > query_name + MAXDOMAINLEN))
+		    (src + *src + 2 > buffer_end(packet)) || 
+		    (src + *src + 2 > query_name + MAXDOMAINLEN))
 {


This code can be reached by two different code paths. The first one is through query.c which is in my humble opinion the easiest one, just check this out:

271 /*
272  * Parse the question section of a query.  The normalized query name
273  * is stored in QUERY->name, the class in QUERY->klass, and the type
274  * in QUERY->type.
275  */
276 static int
277 process_query_section(query_type *query)
278 {
279         uint8_t qnamebuf[MAXDOMAINLEN];
 ...
282         /* Lets parse the query name and convert it to lower case.  */
283         if(!packet_read_query_section(query->packet, qnamebuf,
284                 &query->qtype, &query->qclass))
 ...
289 }


It’s not that hard to reach it from that entry point. The second alternative is through this XFR daemon routine:

1508 void
1509 xfrd_handle_passed_packet(buffer_type* packet, int acl_num)
1510 {
 ...
1517         buffer_skip(packet, QHEADERSZ);
1518         if(!packet_read_query_section(packet, qnamebuf, &qtype, &qclass)) {
 ...
1565 }


The second vulnerability, the one at 2.3.7 is this:

219 /*
220  * Parse the question section of a query.  The normalized query name
221  * is stored in QUERY->name, the class in QUERY->klass, and the type
222  * in QUERY->type.
223  */
224 static int
225 process_query_section(query_type *query)
226 {
227         uint8_t qnamebuf[MAXDOMAINLEN];
 ...
230         uint8_t *query_name = buffer_at(query->packet, QHEADERSZ);
231         uint8_t *src = query_name;
 ...
234         /* Lets parse the query name and convert it to lower case.  */
235         while (*src) {
236                 /*
237                  * If we are out of buffer limits or we have a pointer
238                  * in question dname or the domain name is longer than
239                  * MAXDOMAINLEN ...
240                  */
241                 if ((*src & 0xc0) ||
242                     (src + *src + 1 > buffer_end(query->packet)) ||
243                     (src + *src + 1 > query_name + MAXDOMAINLEN))
244                 {
245                         return 0;
246                 }
247                 memcpy(dst, src, *src + 1);
248                 dst += *src + 1;
249                 src += *src + 1;
250         }
251         *dst++ = *src++;
 ...
268 }


Which is more or less the same concept. This was patched by applying this patch:

 if ((*src & 0xc0) ||
-                   (src + *src + 1 > buffer_end(query->packet)) ||
-                   (src + *src + 1 > query_name + MAXDOMAINLEN))
+                   (src + *src + 2 > buffer_end(query->packet)) ||
+                   (src + *src + 2 > query_name + MAXDOMAINLEN))
 {


One kind of surprising thing was the official statement which surprised me a little.. I’m talking about this:

It is highly unlikely that the one-byte-off
issue can lead to other (system) exploits.

We are in 2009 and there are still developers who believe off-by-one overflows are not exploitable conditions. I am speechless.

Written by xorl

May 23, 2009 at 00:07

Posted in bugs

2 Responses

Subscribe to comments with RSS.

  1. DNS compression pointers are denoted in a label by setting the highest two bits of an octet two one.

    The NDS code tests this in the following way:

            if ((*src & 0xc0) ||
    

    This is clearly incorrect, as this tests wether any of the two highest bits are set, and not both. It should read:

            if ((*src & 0xc0) == 0xc0) ||
    

    The NDS code is cleanly written, but from a security point of view it seems a tad messy to me, especially when one can find multiple bugs in the same snippet.

    dividead

    May 23, 2009 at 20:11

  2. Indeed, you are right. Great finding btw!

    xorl

    May 24, 2009 at 11:58


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s