How to safely use c-string in C embedded applications-CodePudding

After realizing that String type (with capital S) on Arduino was a big source of troubles (cf. https://hackingmajenkoblog.wordpress.com/2016/02/04/the-evils-of-arduino-strings/), I am trying to deal with c-string to be more safe and robust for embedded applications.

However, I am facing some issues regarding the safety. To illustrate my problem, let's take a function of md5 hashing that will receive a string message, concatenate a private key and then compute and return the hash as a string.

I came to this function:

#define MD5_PRIVATE_KEY   "my_private_key"

void ComputeMd5(const char* msg, char* hashBuffer, uint8_t hashBufferSize)
{
    if( (hashBuffer == NULL) || (hashBufferSize == 0) )
    {
        /* INVALID ARG */
        *hashBuffer = '\0';
        return;
    }

    if(hashBufferSize <= HASH_SIZE)
    {
        /* SIZE ERROR */
        *hashBuffer = '\0';
        return;
    }

    uint16_t toHashSize = strlen(msg)   strlen(MD5_PRIVATE_KEY)   1;
    char toHash[toHashSize] = "";

    strcat(toHash, MD5_PRIVATE_KEY);
    strcat(toHash, msg);

    strncpy(hashBuffer, MD5(toHash, HASH_SIZE), hashBufferSize);
}

With this function, calls to strlen(msg) and strcat(toHash, msg) are not safe since we don't know the length of msg to use strnlen() and strncat() instead, and we don't even know if msg is a valid null-terminated string.

My question is, would it be a good practice to add the msg length in the prototype such as void ComputeMd5(const char* msg, uint16_t msgSize, char* hashBuffer, uint8_t hashBufferSize) in order to use the 'n' version of strlenand strcat? And, is it acceptable to rely on the caller to provide a valid null-terminated string or is it the responsibility of this function to make check (if yes, how?).

Maybe there is a complete different design to do it and I don't know it (but I still want to avoid dynamic allocation since it's considered as not safe for embedded applications).

Sorry if this isn't very clear, it is still confuse in my head. I'm looking for a discussion about best practices to use c-string in the safer way.

Thanks.

CodePudding user response：

You may have a problem even if the parameters are perfectly valid. For example:

    uint16_t toHashSize = strlen(msg)   strlen(MD5_PRIVATE_KEY)   1;
    char toHash[toHashSize] = "";

VLAs may be (and to the best of my knowledge usually are) allocated on stack. How much stack you have, and whether it will fit in it in the case of a particular msg you don't know and don't check (ignoring the initialization issue for a second). So in your attempts to avoid a vulnaribility you just created a new one.

So yes, if you add length to the parameters you could validate that the msg is in fact a null-terminated string of that length, but that's assuming you can trust the caller to provide the correct length. But if you trust the caller to provide a correct length - you should be able to trust them to provide a null-terminated string to begin with, shouldn't you?

Or, just don't treat it as a string but rather as a random binary buffer that you're hashing, in which case length parameter is required and \0 terminator is not. I don't know which MD5 implementation you're using, but you can hash a random binary buffer, it doesn't have to be a string (your MD5 call seems to be expecting a string).

CodePudding user response：

Arduino is not C only C ;

There is no 100% safe way of dealing with pointers and arrays (including null character terminated char arrays called C strings).

Your attempt to make them "safe" is extremely unsafe.

    if( (hashBuffer == NULL) || (hashBufferSize == 0) )
    {
        /* INVALID ARG */
        *hashBuffer = '\0';
        return;
    }

If hashBuffer == NULL it is undefined behaviour.

You do not check if msg is not NULL
You do not check if toHashSize is a reasonable sized.
strncpy is not a safe function. If MD5 function returns a string longer or the same size as hashBufferSize the string will not be null character terminated.