[CLUE-Tech] OT: C code security - what to look for?

[Dale K. Hawkins]

Matt Gushee <matt at gushee.net> writes:


> Example: http://havenrock.com/pub/gd4o/gd4o-1.0a2.tar.gz  ;-)
>          (if you do look at this, 'gdstubs.c' and the Makefile
>           will be the most informative)
> Explanation: http://caml.inria.fr/ocaml/htmlman/manual032.html

Now we 0wn your b0x!  :-)

I was looking your code and it does look pretty good (from a cursory
overview).  However, I would point out a couple of issues I noticed if
you are going for absolute-bring-it-on-all-you-h4x0rs code.

Let me further preface this by stating that my familiarity with ocaml
and the ocaml C interface library now consists of looking at the
referenced web page for all of two minutes.

1.  Unchecked allocations:

The first thing that sticks out is the results of the alloc_* calls
are not checked before the result is used.  If nothing else, print an
error message and abort (or raise an exception).  This may point out a
weakness in my understanding of the C binding, but it seems a good
thing to scrutinize.  (A macro might be a big help here; even an
assert).

2.  Unchecked assumptions:

One of the techniques utilized by the truly anal, is to check every
assumption going into and just before returning from a function call
(check your invariants).  The C++ FAQs has a good section on this.
Basically, never take for granted that which you can check.

Random example for your stubs:

> value ml_image_dline(value *argv, int argn) {
>   return
>     ml_image_line_native(argv[0], argv[1], argv[2], argv[3], argv[4],
>                          argv[5], argv[6]);
> }

What is argn?  Why is it not used?  Should the number of arguments
always be exactly right?  Is it OK to be more?  (I can guess, but...)
This is a good opportunity to add some additional checks.

At the very least, I'd do the following:

> value ml_image_dline(value *argv, int argn) {
> 
>   assert(argn == 7 && "Wrong number of arguments given to ml_image_dline");
> 
>   return
>     ml_image_line_native(argv[0], argv[1], argv[2], argv[3], argv[4],
>                          argv[5], argv[6]);
> }

3.  Unchecked bounds:

Here is another "good" one.  What if x and y lie outside the image?
Could I use that to get a memory dump of the current program?  Maybe.

> value ml_get_pixel(value gdw, value x, value y) {
>   return Val_int(gdImageGetPixel(IM_VAL(gdw), Int_val(x), Int_val(y)));
> }


Some other points to consider:

1.  Unchecked file access:

The binding seems to allow the ability to open files (unchecked) any
in the filesystem.  I cannot offer you any practical advise other then
an deep, heartfelt *shudder*!

This is just a really, really big red flag.  I am not saying that it
is wrong to provide such functionality, but this would be an area of
concern WRT hacking.  I have seen may examples of how exactly this
sort of hole was used to crack a box.  The famous was a challenge
which pitted a windoze box v. a linux box about four years ago.  The
linux web server had a cgi script serving up advertisement banners
with unchecked gif images and *whamo*.


2.  Violate your own code

If you don't want to script kiddies in, you need to be like the script
kiddies.  Write a program to which consists of a bunch of tests which
basically violate every assumption and constraint you can possibly
think of for every function call.  Some examples:

A.  Send the run number of parameters to ml_image_dline.

B.  Send an invalid image reference to a given function call (this
    would require creating a way to verify an image handle is truly
    valid before doing any operations).

yadda, yadda, yadda

Well, I guess I'd better get back to writing my unchecked, buggy,
insecure programs.  Hope this is useful.

-Dale