[dns-operations] Robert Graham - A Quick Review of the BIND9 Code.

[Jim Popovitch]

On Thu, Jul 30, 2015 at 3:44 PM, Robert Edmonds <edmonds at mycre.ws> wrote:
> Jim Popovitch wrote:
>> On Thu, Jul 30, 2015 at 1:55 PM, Jim Popovitch <jimpop at gmail.com> wrote:
>> > On Thu, Jul 30, 2015 at 1:43 PM, Paul Vixie <paul at redbarn.org> wrote:
>> >>
>> >>
>> >> Roland Dobbins wrote:
>> >>> <http://blog.erratasec.com/2015/07/a-quick-review-of-bind9-code.html>
>> >>
>> >> he's completely right about the const problems in that code base. const
>> >> is in C what constraints are in SQL-- more is better.
>> >>
>> >> he's completely wrong about turning off assertions in production-compiled code, and in his comments about the performance requirements.
>> >>
>> >> i disagree, stylistically, with his recommendation never to use strcpy, especially when the alternative chosen is often strlcpy, which permits undetected truncation. in his example, the compiler should have flagged the size_t to unsigned int conversion coming out of strlen.
>> >>
>> >> does anyone know of a linter or compiler that can detect opportunities
>> >> to add 'const'?
>> >
>> > IIRC,  gcc -02 level optimization will auto add consts if it detects constants.
>> >
>>
>> Look here:  https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>> search for "-fipa-cp"
>
> 'const' is a type qualifier.  Generally, appropriate use of the 'const'
> qualifier in APIs allows the compiler to detect at compile time certain
> kinds of memory writes that should be forbidden by the API's contract.
> If you, e.g., don't mark a function's parameter as 'const' when it
> should have been, there's nothing the compiler can do if you then write
> to that variable.  The type system says that write is perfectly legal.
>
> gcc's "-fipa-cp" enables "interprocedural constant propagation", which
> "analyzes the program to determine when values passed to functions are
> constants and then optimizes accordingly".  That has pretty much nothing
> to do with detecting opportunities for adding 'const' qualifiers to APIs
> which lack them.
>

Forgive me if I am misunderstanding your statements, but API's don't
interface with the source code, they interface with the object code
that is produced by a compiler.  GCC's -O2 optimizations produce
compiled code that has corrected/added const declarations.   So, even
if the published API docs say the interface func is non-const, the
underlying API object code (if produced by gcc -O2) will treat the
func as being const (which will probably break a poorly documented API
call).   To be clear, I mentioned -fipa-cp and -O2 because Paul was
looking for a way to detect undefined consts.   Gcc -O2 also has a
plethora of other optimizations that pertain to const
optimization/correction, -fipa-cp-clone (-03) would probably address
the issue with API calls.

Back to the article, (which btw doesn't mention API calls), the const
issue was with dns_message_findname(), which gcc -O2 would most likely
auto-declare as const.

A cursory look of Debian's Bind9 build environment shows:
   $ grep CFLAGS Makefile | grep O2
   CFLAGS =        -g -O2 -I/usr/include/libxml2
   BUILD_CFLAGS = -g -O2 -I/usr/include/libxml2

I don't know if ISC publishes recommended compile options, I would
think that would be distro/release specific.

Finally, a quick look at Bind9 v9.8.4 source shows a considerable
amount of type checking/validation occurring i.e.
REQUIRE(VALID_FCTX()), REQUIRE(VALID_IFITER()),
REQUIRE(VALID_SOCKET()), etc.   So just the single lack of a const
declaration in no part means that there is an unchecked variable.
Even this "vulnerability" shows that nothing was at stake other than a
properly working assert/REQUIRE().   Ok, so the assert caused bind9 to
crash... but it's not like it leaked keys.

-Jim P.