[Bug] Double Free in TinyCBOR #258
Comments
|
I disagree that you should dereference a pointer when the |
|
I don't understand how this condition can be reached, at least not with in-memory buffers. With the following test: Inside of If you have a testcase that can reach those points, please provide them. |
We were returning from the function with the memory we had allocated and freed, if the second iteration over the string produced a failure that didn't happen on the first one. This can't happen with pure memory buffers, but can happen with an external data source that fails to produce the same contents twice. I'm documenting that the values in all error conditions except for OOM are undefined, so one mustn't attempt to use them, even to free. This does not change behaviour of the library, just documents. But this commit does make it clear the OOM condition will return a valid `*buflen` and `next`, the latter of which is new behaviour with this commit. Fixes intel#258. Signed-off-by: Thiago Macieira <[email protected]>
We were returning from the function with the memory we had allocated and freed, if the second iteration over the string produced a failure that didn't happen on the first one. This can't happen with pure memory buffers, but can happen with an external data source that fails to produce the same contents twice. I'm documenting that the values in all error conditions except for OOM are undefined, so one mustn't attempt to use them, even to free. This does not change behaviour of the library, just documents. But this commit does make it clear the OOM condition will return a valid `*buflen` and `next`, the latter of which is new behaviour with this commit. Fixes intel#258. Signed-off-by: Thiago Macieira <[email protected]>
|
crash-9a7c570f506fb55b4166b7dcfada2e08cee83d79.zip This file would be helpful. |
That file contains exactly the string "[6.4]". Are you sure you attached the correct file for this bug report? That's the string that was the source of the #259 issue. In any case, I need a full reproducer: source application and data input both. As I said above, the issue is that parsing the same bytes twice should produce the same error twice, so we shouldn't have reached the point where we call |
|
The input file is right. |
|
libfuzzer/ AFL only drives the TinyCBOR API. I want to know the portion of the code that did use the API and how the inputs were constructed. I wonder if this problem is a result of #259 because you're using the json2cbor code with an old (buggy) version of TinyCBOR. The following code has no problems for me: #include <cbor.h>
int main()
{
static const unsigned char buf[] = {
0x5b, 0x36, 0x2e, 0x34, 0x5d
};
CborParser parser;
CborValue it;
CborError err = cbor_parser_init(buf, sizeof(buf), 0, &parser, &it);
if (err)
return err;
err = cbor_value_advance(&it);
return err;
}Neither ASan nor Valgrind complain. With #261 updating the source and the documentation, I consider the problem fixed. |
|
Do you think whether it is a real security issue or not? |
|
I do not. First, the problem as described lies in the caller code for freeing a pointer after the function returned an error, so I've clarified the documentation to be clearer. Second, as far as I can tell and with all the testcases you've supplied and the library's own, it's impossible to reach that point in the code. So even if we consider the API to be dangerous, I don't see how the situation of the double free can arise in the first place. So, no, this is a doc issue. |
the following function has side-effect if it fails - makes illusion of memory allocation, while in fact this memory is freed. Then external code tries to free it (because the pointer clearly changed NULL -> some value) and crashes at double free.
Client code:
Root cause is here:
tinycbor/tinycbor/src/cborparser_dup_string.cThis implementation shows bad practice: making irreversible changes to external entities no matter how the call ends.
This is natural expectation that if a transaction (here: function call) fails, then no side-effects will happen outside of the boundaries of this transaction. In this case
_cbor_value_copy_string()fails but external pointer*bufferbecomes non-NULL anyway, which causes natural wish to free it (especially if the developer does not check return values).Crash value given by libFuzzer to cause this error is: 22 bytes
Sanitizer says:
The text was updated successfully, but these errors were encountered: