connectedhomeip icon indicating copy to clipboard operation
connectedhomeip copied to clipboard

Out Of Bound Write in chip-shell’s Read-EvalPrint Loop Parser

Open robszewczyk opened this issue 3 years ago • 2 comments

Problem

With specifically crafted input, TokenizeLine() code could write four or eight (pointer size) bytes of 0 after the end of tokens[] array.

int TokenizeLine(char * buffer, char ** tokens, int max_tokens)
{
    size_t len = strlen(buffer);
    int cursor = 0;
[...]
    // The first token starts at the beginning.
    tokens[cursor++] = &buffer[i];
    for (; i < len && cursor < max_tokens; i++)
    {
    [...]
        tokens[cursor++] = &buffer[i + 1];
    [...]
    }
    tokens[cursor] = nullptr;

While there is a check against max_tokens in the for loop above, it does not take into account the final nullptr write into tokens[] array.

Proposed Solution

A simple fix would be to not use a nullptr guard and rely on callers to use argc when determining size of argv .

Alternatively there would need to be a check before nullptr assignment.

    // Too many arguments, overwrite the last entry with the guard.
    if (cursor == max_tokens)
        cursor = max_tokens-1;
    tokens[cursor] = nullptr;

Note that currently TokenizeLine() will write at least two entries into the tokens[] array, regardless of max_tokens value. This could be surprising to function users.

Relatively low priority. Could be fixed post 1.0

robszewczyk avatar Jun 08 '22 22:06 robszewczyk

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Dec 10 '22 06:12 stale[bot]

This stale issue has been automatically closed. Thank you for your contributions.

stale[bot] avatar Dec 21 '22 06:12 stale[bot]

This stale issue has been automatically closed. Thank you for your contributions.

stale[bot] avatar Dec 30 '22 21:12 stale[bot]