[dns-operations] CVE 2015-8000 actively exploited yesterday

[Robert Edmonds]

David C Lawrence wrote:
> Paul Vixie writes:
> > i knew one thing for sure, which i made into an unbreakable law for
> > the bind9 team: no remote code execution errors. robert halley then
> > offered an eiffel-styled assertion syntax, which was then used
> > everywhere:
> 
> "Design by contract."  As one of that team, personally I think it was
> a good call and still think of it as one of Bob's excellent design
> decisions.
> 
> It is not without an occasional flaw that you can rationally blame it
> for causing a crash that would not otherwise have been a problem (as
> seen in one recent CVE), but on balance it is very successful for
> constraining assumptions and ferreting out genuine problems.

I'm not quite sure the DbC-style assertions are a cause so much as a
symptom in some of these cases.  The root cause may be that some parts
of BIND9's internal API may be a little too brittle, in which case the
DbC assertions are catching real problems that could lead to RCE in some
cases.  So, perhaps the problem is not so much the DbC assertions
themselves, but the fact that it's very easy to write BIND code that
trips the assertions.

E.g., taking my favorite BIND9 "nastygram" vulnerability CVE-2011-1910,
git commit b335299 added the following change to lib/dns/ncache.c, which
writes an additional byte to a buffer:

@@ -183,18 +184,20 @@ dns_ncache_addoptout(dns_message_t *message, dns_db_t *cache,
                                        /*
                                         * Copy the type to the buffer.
                                         */
                                        isc_buffer_availableregion(&buffer,
                                                                   &r);
                                        if (r.length < 2)
                                                return (ISC_R_NOSPACE);
                                        isc_buffer_putuint16(&buffer,
                                                             rdataset->type);
+                                       isc_buffer_putuint8(&buffer,
+                                                            rdataset->trust);

This introduced an off-by-one error which could cause an assertion with
very large RRSIG RRsets.  The fix in commit e65a164 adjusted the length
check a few lines above to match the number of bytes actually written to
the buffer:

@@ -184,11 +184,11 @@ dns_ncache_addoptout(dns_message_t *message, dns_db_t *cache,
                                        /*
                                         * Copy the type to the buffer.
                                         */
                                        isc_buffer_availableregion(&buffer,
                                                                   &r);
-                                       if (r.length < 2)
+                                       if (r.length < 3)
                                                return (ISC_R_NOSPACE);
                                        isc_buffer_putuint16(&buffer,
                                                             rdataset->type);
                                        isc_buffer_putuint8(&buffer,
                                               (unsigned char)rdataset->trust);

Basically, the broken code said, "Make sure the buffer has at least two
bytes of space available, since I'm about to write three bytes into it."
The fixed code says, "Make sure the buffer has at least three bytes of
space available, since I'm about to write three bytes into it."

I call this "brittleness", because it seems to me that this kind of
"pack bytes into a buffer" operation can be done without forcing the
caller to perform repetitive bookkeeping on the exact number of bytes
written into the buffer.

-- 
Robert Edmonds