NULL ptr dereferences found with Calysto static checker

[Ken Raeburn]

On Jun 6, 2007, at 19:20, Domagoj Babic wrote:
> I've ran my static checker Calysto ( http://www.calysto.org/ ) on  
> krb 5.1.6.
> Here's the postprocessed report:

Thanks.  I hadn't looked at Calysto yet, but I've been wanting to get  
some static analysis tools applied to the code.

> + krb5-1.6/src/util/support/fake-addrinfo.c:1336
> krb5int_getaddrinfo is a function with external linkage, which calls
> getaddrinfo (fake-addrinfo.c:1097), passing aip as the fourth  
> parameter
> (received as **result). Without checking whether result is NULL or  
> not,
> getaddrinfo passes it to system_getaddrinfo (which is actually
> getaddrinfo in netdb.h). system_getaddrinfo can set result to NULL if
> the system is out of memory. Code at
> krb5-1.6/src/util/support/fake-addrinfo.c:1143 dereferences result,
> without checking it.

system_getaddrinfo could set *result to NULL, not result itself,  
which was passed in by a caller.  The line after the  
system_getaddrinfo(..., result) call checks for an error return or a  
null pointer stored at *result.  If the system getaddrinfo runs out  
of memory, it should return a non-zero result, so we won't bother  
looking at what it might have put in *result.  And it's an error for  
the caller to provide a null pointer as the result argument; that's  
not something I think we need to check, any more than I'd expect  
strcpy to check for null pointers.

> + krb5-1.6/src/util/support/gmt_mktime.c:54 krb5int_gmt_mktime is a
> function with external linkage, dereferences parameter t without
> checking it.
>
> + krb5-1.6/src/util/support/errors.c:155 same as above, for parameter
> ep.

A lot of these functions, while exported from the support libraries,  
are considered internal functions, so there's not a lot of  
documentation, but often there are arguments that are not allowed to  
be null; if the caller passes a null, a crash is considered an  
acceptable response to this bug in the calling function.  I think all  
the functions you list in util/support are of that flavor, but I'll  
skim them a little more carefully later.  If there are places where  
these functions can get called with null pointers, that'd be more  
interesting.

There are functions in the public API where it'd be nice to check for  
null and return some nice error code, where we don't do that already,  
I expect.  I'm curious how Calysto works, that it reported these in  
the support library, but not some of the libkrb5 functions, like  
krb5_mk_req for example, which doesn't check that the context  
argument is non-null before using it.  (Or did you remove some of  
these already?)

> Note: Calysto reports warnings about unchecked dereferenced parameters
> only if a function F:
> 1) has external linkage,
> 2) parameter is dereferenced in F or any function called by F,
> 3) there is a feasible path from the entry block of F to the statement
> that dereferences the pointer, and
> 4) F is not called from any other function - in that case, Calysto  
> has no
> context information about the parameters, and has to consider them to
> be undefined.

Ah... but krb5_mk_req isn't called by anything else in libkrb5.  And  
if you're looking across library boundaries, functions like  
krb5int_getaddrinfo are called from code in libkrb5.

I'm not sure that last constraint is entirely logical.  If F is  
called from another function in the same library with a non-null  
argument, but is exported from the library and is intended to support  
a null argument as well, it'd be useful to know if it's dereferencing  
without checking.  Of course, whether the function is supposed to get  
a null pointer is less a matter of the code than of the  
specification, so heuristics are the best you're going to be able to do.

If Calysto is figuring out for itself which pointer arguments are  
required to be non-null and which are allowed to be null, I'd be  
curious to see those results.

> None of the functions mentioned above seem to be called from any
> other function in the compiled binary (compiled with llvm-gcc
> http://llvm.org/ ), although in the source I see that some are called
> from the code that didn't end up in the binary for some reason.
> Hence, Calysto assumes that those functions are library-like  
> functions.

Which binary?  We build a number of them.  If the results are only  
from one of them, that might explain the missing functions.

If you have to pick one to analyze, the krb5kdc or kadmind binaries  
are probably best.  (But if you find any problems there that look  
like they could crash the programs and possibly be triggered  
remotely, please report them to krbcore-security at mit first, rather  
than this list, so we have a chance to put out an update before  
script kiddies start crashing KDCs everywhere...)

> I'd appreciate if you could let me know whether you consider these to
> be bugs or not and why.
>
>
> Besides these reports, there seem to be no other unckecked  
> dereferences
> in krb, which certainly says a lot about the code quality - other  
> open source
> projects I've checked so far have a larger number of non-trivial  
> NULL ptr
> dereferences.

Another reason to suspect a bunch of issues have escaped your  
analysis... ;-)

Ken