code safety

From Jose Nazario

Comments By Mike Orr, Ben Okopnik, Steve Kemp, Tom Bradley

i was looking through the november issue of linux gazette and something caught my eye. overall the issue had a few things i was pretty happy to see: a piece on mono, elf kernel execution, and adding loadable plugins to code. it's this last piece i have a problem with.

Adding Plugin Capabilities To Your Code:

http://www.linuxgazette.com/issue84/bradley.html

tom bradley's code, while demonstration code, is a perfect example of unreliable code and illustrates why this kind of thing should be avoided. in main.c (truncated to save space):

#define PATH_LENGTH 256
...
        char path[PATH_LENGTH], * msg = NULL;
...
        /* build the pathname for the module */
        getcwd(path, PATH_LENGTH);
        strcat(path, "/");
        strcat(path, argv[1]);

it's quite trivial to overflow path[PATH_LENGTH], even inadvertantly. before you say "look, this isn't setuid root, this isn't anything but demonstration code, don't rush off to bugtraq" i want to say this: for precisely the reason that it is demonstration code it should do bounds checking.

[Ben Okopnik] Agreed, 100%. One of the many security-related sites I read on a regular basis had a "ha-ha-only-serious" quote that's worth paying attention to:

<ironic> Security hint of the day:

        find . -name '*.[ch]' | xargs egrep -l 'sprintf|strcat|strcpy' | xargs rm

</ironic> -- Pavel Kankovsky aka Peak

Funny, but...

[Steve Kemp] There are a few decent scanning tools available, like 'flawfinder', 'rats', and 'its' which are worth using if you want to be scared!

Steve
---
# Debian Security Audit Project
http://www.steve.org.uk/Debian

lots of people are going to code their apps with this as a start and not think twice about the reliability of the foundation of this code. the fact is someone can easily hit this upper limit inadvertantly (think of a well organized person who has a deep directory structure ... suddenly path[] has a lot less overhead).

secondly, bounded string manipulation should just be a habit, and habits develop after repeated application of the effort. crappy, unchecked runtime errors are the bane of software quality, there's no reason you shouldn't always do sanity checks, even in demo code. one reason alone to do it is that you'll get so annoyed you may want to improve the interface to error checked code, benefitting us all.

anyhow, thanks for the november issue.

Forwarding to the author, Tom Bradley <tojabr@tojabr.com>. This message will be in December's LG . Feel free to write a response or a follow-up article if you wish. -- Mike

thanks mike. tom, in all seriousness that article was really cool and timely, and i will definitely be referring to it to make use of it. i just take issue with unchecked errors in code ...

thanks for an otherwise well written piece.

[Tom Bradley] I agree that was setting a bad example on my part, below is a corrected version.

(truncated to only changed partion) ...

char * path, * msg = NULL; int (*entry)(); void * module;

if(argc < 2) {

printf("No module given.\n"); return; }

path = (char*)get_current_dir_name(); path = (char*)realloc(path, strlen(path) + strlen(argv[1]) + 2); strcat(path, "/"); strcat(path, argv[1]); ...

the #define has been removed.

Tom

Meet the Gang   1   2   3   4