Validating Untrusted Integer Inputs
Subscribe To Our Feed | Follow Us On Twitter | Get Updates on Email
If you are writing a software which exposes APIs to be used by a third party, then first thing you have to do is to make sure that all the integers parameters have been validated. Every incoming value to your function should be considered as tainted. The function should validate the input value by checking it for all possible malicious value. After the function validates the input, then only any operation on the input value should be performed.
Consider the following code sample:
void func( int size ) { char* str; if(size > 0 ) { str = (char*)malloc(size * sizeof(char*)); size++; /** *some code to operate on this string "str" **/ free(str); } }
I am sure that by now, you would have identified some loop holes in this code. Now, a caller of this function can give different input values which might result in following flaws:
1) The function might get an highest input value which results in a large memory allocation for ‘char* str’ which the function never expected.
2) The function might result in memory allocation failure as there is possiblity of the system running out of memory.
3) The function might have an overflow issue due to an increment in input value which could have been equal to SIZE_MAX.
These scenarios might serve as a boon for a hacker and he/she can instigate either a denial of service or any other buffer overflow errors.
Now, Lets rework the above depicted code again.
#define MAX_SIZE_OF_STRING ( 100 ) void func( int size ) { char* str; if(size > 0 && size <= MAX_SIZE_OF_STRING) { if(str == null) { str = (char*)malloc( size * sizeof(char*)); if(size < SIZE_MAX) { size++; } /** *some code to operate on this string "str" **/ } if(str != null) { free(str); } } }
Please try to notice few defensive points from the above given code.
1) We have defined a maximum size for the string. We need to do this to make sure that large chunks of memory do not get allocated. This will result in optimized memory usage and longer life of the program.
2) We have validated the input value for its range comparing against its minimum and maximum value. By doing this, We sufficed the purpose of defining the size of the string.
3) Again, we check the input variable size for its value less than SIZE_MAX (maximum value possible for an integer). By doing this, we safeguarded against an overflow. Now, the size variable can never incremented beyond the maximum value.
4) Checking for ’size > 0′ helps in making sure that non zero number of bytes are allocated in memory, in turn, saving us from memory corruption.
By adding extra defense checks or safeguards, you might contribute towards addition in code size. But isn’t it better to have a secure code rather than less code which is vulnerable to exploits.
The point I am trying to make is very simple and is not a great deal. Everyone of us know of it but we tend to ignore these minor things resulting in misuse of code. Keeping these small points in mind while coding and adding these defense checks or safeguards will definitely result in robust and secure code.
© Safer Code | Validating Untrusted Integer Inputs
|
Liked this post? Get FREE Updates Subscribe to RSS feed 
|
Related posts
Tags: buffer overflow, C, denial of service, input validation, malloc, memory allocation, overflow errors, Safety, Security, untrusted inputs
There is an error in the 2nd example. An assumption is made that the newly declared pointer str is null before it is malloc’d. Since the pointer is not initialized, that may not reliably be so (depending on the compiler used I suppose).
It would be better to add an initialization to the declaration :
char *str = (char *)null;
Otherwise the way it is coded, it is possible that the free call will cause a seg fault.
By testing for str to be equal to null, you introduce code that might run sometimes, and not some other times.
The ANSI C standard doesn’t say anything about local variables being initialized to a certain value (you’re assuming the compiler sets it to null, but it not necessary true).
Also, you fail to test if the malloc() was succesful (though in case it is not, there isn’t much you can do but exit).
Regards,
Mihail.
Why not just limit what size can be by anding:
size =& 0×07ff;
Size can not exceed that value. If it is bigger when passed in it will be reduced, and if signed will be converted in to an unsigned int. Trying to use a signed it to malloc memory is a not recommended. How many people need -1 bytes
Also as others have said initialise all local variables to a know value, check all returns (even printf) and take no short cuts.
@Lee, Mihail, stu,
Thanks for correcting the code. But In this post, I wanted to emphasize on the usage of integer inputs (for example: ’size’ variable).
I do understand your concerns of not validating the return values or codes of the function calls but then, that was not the main agenda in this code segment. and Ofcourse, I did not expect a stringent code review
str = (char*)malloc( size * sizeof(char*));
Are you really trying to allocate an array of pointers to char?
This is so bad, it has to be a joke.
What is null? It isn’t defined anywhere.
The parameter given to malloc() is most certainly not what you meant. That is also an opportinity for an overflow in general. You would have to check that size is smaller than SIZE_MAX divided by the element size. Otherwise, the multiplication my wrap and you request less from malloc than intented and cause a heap overflow when writing beyond the resulting array.
SIZE_MAX is the maximum value for size_t and unsigned integer type. There’s likely no platform where INT_MAX is larger or equal to SIZE_MAX. Hence you don’t prevent an integer overflow at all. There’s no point to check size at that place anyway. What you have to check there is the returned pointer by malloc.
Likewise your documentation and code should make clear whether the size and MAX_SIZE_OF_STRING include the terminating NUL or not. Big difference and another typical cause of a buffer overrun.
There’s little to no point in checking a pointer against NULL – whatever null may be – before passing it to free(). free() handles NULL just fine by definition.
There’s also no reason at all to cast the result of malloc(). Casts should be avoided whenever possible because they suppress useful compiler warnings.