Re: (ITS#8240) OpenLDAP ber_get_next denial of service vulnerability

[michael@stroeder.com]

hyc@symas.com wrote:
> h.b.furuseth@usit.uio.no wrote:
>> On 12/09/15 16:24, michael@stroeder.com wrote:
>>> I've compiled with CFLAGS="-DNDEBUG" (also tried CPPFLAGS) but this did not
>>> help. slapd still crashes when hitting the assert.
>>
>> Yes, portable.h #undefs it by default.  OpenLDAP has always conflated
>> logging, debug output and asserts behind LDAP_DEBUG.  We've been saying
>> for some time that we really ought to do something about that someday...
> 
> Yes, and that's more obviously a bug that we can fix.

Is it an easy fix?

I think that in opposite to the OpenLDAP project most people out there
consider this to be a really serious bug to be fixed really soon.

For now with my own builds I apply the patch removing the assert statement.
But I wonder how many asserts statements are in the code which can be hit by
invalid input leading to a crash.

>> Even ignoring that, demanding -NDEBUG is backwards in so many ways:
>>
>> Using C's features like <assert.h> is not the user's job, it's
>> OpenLDAP's (i.e. configure and portable.hin).  The person building
>> OpenLDAP might not even be a C programmer who knows about the C
>> language quirk that it has a feature makes errors crash by default.
> 
> It is standard practice in C code. assert() and NDEBUG are part of the C 
> standard. A person who doesn't know C has no business building the code. 
> Certainly the libraries are of no use to them if they're not C programmers 
> already.

This is a black-and-white-only statement which does not meet 90% of the cases
out there.

>> A simple "./configure --prefix=/whatever" ought to be a reasonable way
>> to build OpenLDAP, like with most other packages.  There are
>> installation instructions and they do not mention NDEBUG.

I strongly concur with Hallvard here.

> Every use of assert is "assert(the code is correct)" - but that often depends 
> on dynamic state, not just the statically written code.

Yes, dynamic state including invalid input.  But IMO "assert(the code is
correct)" should never be hit no matter how bad the input was.  And it should
definitely not crash the server (with system's ressource limits being a
unavoidable exception).  Rephrasing: The meaning of the statement "the code is
correct" should also include "invalid input is properly handled as error" - no
matter what.

Ciao, Michael.