[CWE-401] cbortojson: fix memory leak when realloc() is fails

[GermanAizek]

@thiagomacieira, I accidentally found it while I was studying the source code.

About security patch changes:

It is better to close this possible leak before assign CVE id and score, and also protect yourself from possible attack vectors by intruders.

This commit also have identical issue as real CVE-2019-17178 vulnerability in FreeRDP project:

Fix commit - FreeRDP/FreeRDP@9fee4ae

Issue - FreeRDP/FreeRDP#5645

More info about CWE metrics:

https://cwe.mitre.org/data/definitions/401.html

[thiagomacieira]

@thiagomacieira, I accidentally found it while I was studying the source code.

Hello @GermanAizek. Thank you for your contributions.

This commit also have identical issue as real CVE-2019-17178 vulnerability in FreeRDP project:

Fix commit - FreeRDP/FreeRDP@9fee4ae

This is not the same. In the FreeRDP change, the resulting code is freeing the previous buffer that had been allocated:

		tmp2 = (LPSTR)realloc(tmp, ds * sizeof(CHAR));
		if (!tmp2)
			free(tmp);
		tmp = tmp2;

That's not the case here: we are just returning CborErrorOutOfMemory. It's up to the caller to free the buffer and it will:

            err = escape_text_string(str, &alloc, &offset, chunk, len);
    }

    if (likely(err == CborErrorNoMoreStringChunks)) {
... doesn't get run ...
    }

    cbor_free(*str);
    *str = NULL;
    return err;

[thiagomacieira]

Even if there is a problem with freeing, this patch isn't solving it. buf is a local variable in this function, so adding yet another temporary is not going to help. This code will compile to exactly the same assembly before and after the change.

[GermanAizek]

@thiagomacieira, how do you propose a change so that its more secure?

[thiagomacieira]

I'm saying it's already secure.

[thiagomacieira]

I'm not seeing a problem here. Can you show an example of where there would be an issue?