Questions about code review (C) [closed]

Question

As part of a Security Workshop course I did last semester (im in Australia) we had to learn about Code Auditing on our own by reading "The Art of Software Security Assessment".

In our final exam we had to audit this code. I only managed to find two vulnerabilities:

Vulnerability 1:

Line 290, format string is from user input string “inputString”. This is the format string vulnerability that can be exploited to run arbitrary code.

While there is a valid character filter, this check does not prevent the printf from running with unsanitized string, so there is no “security” filter.

Vulnerability 2: Line 344. If new array is created and show is immediately called (without the array being initialised), the contents of stack is printed instead.

This could be garbage or information leakage.

Could you guys give practical examples of what other vulnerabilities are in this code and how you went about finding them (thought process)?

Answer 1

I wont go through vulnerabilities present, but I can walk you through my process/thinking.

1. static analysis tools - start with the obvious and have some tools take a look at it, a couple of options include Visual Code Grepper and Cppcheck
2. Next we look at things manually, start with functions that are dangerous, these tend to be string and memory manipulation functions. Follow the code backwards up the call stack to see where they get their data from, checking if that data has been properly validated. If not, then that is likely to be another vulnerability.
3. Next, start at all your program inputs, this include program arguments, anywhere where user input has been entered, network reads etc. etc. Then walk through the code to ensure that all inputted data has been properly validated. Again if not, then this is likely to be another source of vulnerabilties
4. Finally (there may be more steps, but this'll get the bulk of them) look for language or framework idiosyncrasies. Do you have API calls that have the wrong arguments e.g. a realloc where the resultant pointer is the one used in the call. Is there a correct sequence of API calls required to set something up (crypto falls into this category)

That's more or less what I do, though I have a step 0 which is to read the code top to bottom, just to get a feel for it, it's odd, but you can spot things just because they look funny

Answer 2

I approach reviews very differently, and have large expectations. As someone who has to work with the code in the future, I want only code that is easy to maintain. I want to look at code and have it tell me what it does. I don't mean that it needs comments, because comments are usually an indicator of code that is too complex, and are often masking problems (comments are often out of sync with the code, but code doesn't lie.) Instead, I want to see small functions that have self-describing function names that make the intent clear. I want to see unit tests that prove the small functions do exactly what they say they do.

Of course, this approach works better when you have more say over the code. Reviewing ugly code like this, where you can't rewrite it yourself, and you can't yell at the developer for writing ugly code, makes finding problems more difficult than they need to be. This developer is going to continue writing bad code because nobody's correcting his approach. So for now, we have to set the complaining aside and get to work.

First, use tools! Fire up the compiler, and set the warning level to as persnickety as possible. If you have it, run lint. In the Microsoft world, turn on the /ANALYZE switch. If you can afford one, run a commercial static code analyzer. Look carefully at the warnings and errors it emits. Make sure to look for deprecated functions; many old standbys like sprintf() are deprecated because they're unsafe. Anything that a scanning tool can find automatically will free up more time for you to perform an in-depth review. Of course, compilers can find only syntactical or simple logic problems. SCA tools go deeper, but they have limits too.

For security, you need to make sure that you're validating your input properly as a first step. That means using the right input routine (pro tip: scanf() is never the right input routine), then length checks to make sure your buffers can't overflow, followed by checking for whitelisted input characters, and then passing the code to the parsers for actual work.

When I get into the dirt, I look for all kinds of typical coding mistakes. Make sure that all resources that are allocated are properly freed (limiting them to a single scope is a best practice.) Make sure that once a resource is freed that no dangling pointers remain available for use. Pointer conversions and casts are a hint that the developer is trying to shove a square peg in a round hole. Using #define to create macros makes things readable, but macros are not type safe, and bypass the safety of the compiler's type system. In C code, I look to make sure it's const correct. Anything that's dealing with buffer lengths or strlen() can be a problem. And anything that loops can have a boundary condition error; check that the for loops start with the correct index value, and that they don't iterate beyond the length of the buffer.

Let's use the clearString function as a simple example:

void clearString (char* string) {
    //sets the chars of array to string terminator
    int i;
    for (i = 0; i< sizeof(string); i++) {
        string[i] = '\0';
    }
}

I like this function already, because it's small and self describing. That makes it easy to read, and easy to validate. It also makes it easy to spot the error. What does sizeof(string) return, when string is a char*? (Hint: all pointers return the same value for sizeof().) A set of unit tests would show this code is broken. This is a red flag, and the code should be stopped right now and fixed.

Also, look around the input at the sanitization steps. int validCommandString(char* inputString) sounds like it's going to validate a command string, doesn't it? Let's look deeper.

int validCommandString (char* inputString) {
    int valid = TRUE;
    int i;
    for (i = 0; i< strlen(inputString); i++) {
        if (validCharacter(inputString[i]) == FALSE) { //if this character is not valid
            valid = FALSE;
            printf("<< not valid: '");
            printf(inputString);
            printf("'\n");
            break;
        }
    }
    return valid;
}

It starts out kind of weak, because it's not const correct. I don't expect a routine named "validate" should be changing the data, so the input parameter should be const.

A brief glance shows me it's checking byte-by-byte, but it's not parsing anything or checking against a list of commands to see if they're valid. The routine has a poor name, and should be renamed to what it actually does, which is validateInputString()

Next, I see it's assuming everything is rosy by assigning valid=TRUE to start out with. It's a best practice to never assume the input is good; assume it's bad until proven otherwise. But that's mostly a style thing that helps reveal errors; weak, but not necessarily an error.

I see it's using strlen(inputString). This might be OK, or it might not. Depends on if the string was actually cleared before it entered this routine, which clearString() above proved is not a safe assumption. When dealing with unvalidated raw char arrays, it's safer to keep an independent variable to hold the length instead of constantly running strlen() on it. I'm suspicious of this, but I keep going.

Uh, oh. The warning siren is screaming now: the if clause has determined that it's handling a bad character and the response is: printf(inputString); . This means that the code has been handling something suspected of being toxic, it already knows specifically that it's bad, and now it's shoved into stdout where it's free to wreak havoc on any other program dealing with the stream. This is a showstopper bug, and it's good that you caught it.

A lot of time goes into a code review. Use tools when you can, and common sense.

Answer 3

Some problems:

Multiple locations: int is not appropriate for storing sizeof and/or strlen results. It could be to small to hold the value. Use size_t.

Line 227, possibly multiple locations: sizeof from a char pointer is not the string length.

Line 236: Checking if the value is between a and z works like this in ASCII-based encodings, but not in EBCDIC.

Multiple locations: Following pattern

type varname;  
if(a) varname=1;  
else if(b) varname=2;  
return varname;

has a problem if neither a nor b are true (namely, unitialized variable = undefined behaviour).

Multiple locations: Applying strlen, strcpy etc. to char pointer parameters which could contain no null terminator, without stopping where the array ends, is a problem too.

Line 290, the first of your errors.

Line 147/153/156/160, missing terminator.

Line 167/168/169, bad in many ways.

(Overused defines introduce more possibilities for errors.)

(While not necessarily wrong, missing consts where there can be used also introduce more possibilities for security-related errors.)

I don't agree with your line 344 error, because there is no show call etc.