Spot the Bug!

strPassHash = (char*)realloc(strPassHash, MAX_HASH_LEN);

This line needs to be changed to:

strPassHash = (char*)realloc(strPassHash, sizeof(char)*MAX_HASH_LEN);

My C knowlegde is a little chaky these days, but you should always qualify the size correctly to ensure you get the right amount of memory allocated.

realloc returns NULL on failure, but leaves the original intact in that case. In low-memory conditions, this will leak even more memory. Not sure how that might qualify as a security vulnerability, though. So maybe it's not what you're looking for.
To handle realloc properly:

char *temp = (char *)realloc(strPassHash, MAX_HASH_LEN);
if (!temp) { /* error */ }
else { strPassHash = temp; }

Other remarks:
- why "sizeof (char)"? -- it returns 1 by definition
- the code doesn't show how the hash is used or defined. If it is a string (which the "str" prefix would indicate), then it might be necessary to reserve space for a trailing NUL byte.
- there should be checking for NULL after the first call to malloc, too.

Hi,

I do have a few remarks for the code snippet.

First, strPasHash should be checked for a NULL value before creating the hash:

if (NULL != strPassHash)
//Create the hash

Second, I'm missing the sizeof(char) part in the realloc function call (strictly spoken not needed for a char, it would be needed for Unicode characters etc):

strPassHash = (char*)realloc(strPassHash, sizeof(char)*MAX_HASH_LEN);

Third, there should be a NULL check after the realloc call too:

if (NULL != strPassHash)
//continue

Fourth, before reaching the following line:

strPassHash = (char*)realloc(strPassHash, sizeof(char)*MAX_HASH_LEN);

strPassHash should still point to the original location (acquired through malloc) and should not be NULL. If it's NULL, a malloc would be used and the original memory wouldn't be freed, therefore leaving stuff in memory that could be useful for an attacker (and would be exploitable thrugh a buffer overrun etc).

Fifth, the code assumes the hashes are NULL-terminated (and don't contain a NULL value inside?) although no explicit code is present to guarantee this:

strPassHash[STD_HASH_LEN - 1] = '\0';
...
strPassHash[MAX_HASH_LEN - 1] = '\0';

If the strPassHash is returned to the caller (assuming the code is in a function/method) NULL-termination is absolutely needed in order for things such as strlen to work and behave correctly and to avoid overruns.

Last but not least, it's best to AVOID realloc for crypto functions because when the data needs to be copied to another location (because in-place expansion is not possible), the old memory won't be zeroed out. Also, the pattern

a = realloc(a, ...);

destroys the original pointer so there is no way to zero out the memory yourself anymore (also because the old memory is freed, thus not directly accessible anymore).

Greetz,
Bart

PingBack from http://stocks-options-trading.info/options-trading/?p=1035

PingBack from http://insomniacuresite.info/story.php?id=2139