SIGSEGV in binn_load() when fed with random data

[mannol]

ASAN:DEADLYSIGNAL
=================================================================
==19372==ERROR: AddressSanitizer: SEGV on unknown address 0x7ffdef02fd7f (pc 0x5567a79c235b bp 0x7ffd966db680 sp 0x7ffd966db660 T0)
    #0 0x5567a79c235a in AdvanceDataPos /home/mannol/Documents/Programming/B/src/binn.c:456
    #1 0x5567a79c5443 in binn_is_valid /home/mannol/Documents/Programming/B/src/binn.c:1237
    #2 0x5567a79c1f03 in binn_load /home/mannol/Documents/Programming/B/src/binn.c:340
    #3 0x5567a79ade2c in message_from_binn_raw /home/mannol/Documents/Programming/B/src/message.c:800
    #4 0x5567a7936b53 in test_message_fuzz /home/mannol/Documents/Programming/B/src/tests.c:877
    #5 0x5567a79275f5 in run_test /home/mannol/Documents/Programming/B/src/tests.c:50
    #6 0x5567a79bd57d in list_for_each /home/mannol/Documents/Programming/B/src/list.c:62
    #7 0x5567a792851f in run_tests /home/mannol/Documents/Programming/B/src/tests.c:3202
    #8 0x5567a794a1b8 in main /home/mannol/Documents/Programming/B/src/tests.c:3289
    #9 0x7f909e67d3f0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x203f0)
    #10 0x5567a7927389 in _start (/home/mannol/Documents/Programming/B/build/D/tests+0x2c389)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/mannol/Documents/Programming/B/src/binn.c:456 in AdvanceDataPos
==19372==ABORTING

EDIT: Another one in a similar place:

ASAN:DEADLYSIGNAL
=================================================================
==19481==ERROR: AddressSanitizer: SEGV on unknown address 0x7ffed27d8404 (pc 0x55c48d1c240b bp 0x7ffea42b34a0 sp 0x7ffea42b3320 T0)
    #0 0x55c48d1c240a in binn_is_valid /home/mannol/Documents/Programming/B/src/binn.c:1223
    #1 0x55c48d1bef03 in binn_load /home/mannol/Documents/Programming/B/src/binn.c:340
    #2 0x55c48d1aae51 in message_from_binn_raw /home/mannol/Documents/Programming/B/src/message.c:800
    #3 0x55c48d133b53 in test_message_fuzz /home/mannol/Documents/Programming/B/src/tests.c:877
    #4 0x55c48d1245f5 in run_test /home/mannol/Documents/Programming/B/src/tests.c:50
    #5 0x55c48d1ba57d in list_for_each /home/mannol/Documents/Programming/B/src/list.c:62
    #6 0x55c48d12551f in run_tests /home/mannol/Documents/Programming/B/src/tests.c:3202
    #7 0x55c48d1471b8 in main /home/mannol/Documents/Programming/B/src/tests.c:3289
    #8 0x7fba7fa233f0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x203f0)
    #9 0x55c48d124389 in _start (/home/mannol/Documents/Programming/B/build/D/tests+0x2c389)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/mannol/Documents/Programming/B/src/binn.c:1223 in binn_is_valid
==19481==ABORTING

Steps to reproduce (FUZZ):

while (1)
{
    uint8_t data[15000];
    // fill data[] with random bytes
    binn obj[1];
    binn_load((void*)data, obj); // << wait until it crashes
}

[mannol]

The binn_load() function is unsafe in a sense that it doesn't check the bounds for the data buffer. It should receive another parameter for the size and make sure it doesn't read outside that size limit.

[kroggen]

Indeed.

When the data buffer is correct (with data output from binn_ptr) it contains the size in it. So it reads the size from it.

This is the reason why it does not use a size parameter.

It is not intended to be used with other kind of data.

A previous version carried a "magic" identification in the header, but then it was removed as it is not used in most of the serialization libraries and it was increasing the size of the output.

I guess that this is no more used because we must use some kind of checksum in the transport protocol (most already have so we don't need to care). The checksum takes care of the entire packet instead of just the header, so it is a better approach.

Off course it does not cover the cases in which the serialized data is saved on a file and the storage gets corrupted with garbage data instead of the actual one. For these cases we must implement a checksum (maybe CRC32) and save it within the file too.

[mannol]

Oh. So what you are saying is that: I can check the validity of the size in the header and if it matches the buffer size it should be okay? In other words: I can change the binn_load() to accept size argument and compare it with header value?

[kroggen]

It does not mean that the entire serialized content is OK, but at least it would avoid accessing data outside of the buffer bounds.

I could make a binn_is_valid_ex function for this as the binn_is_valid function does not accept a size parameter, it reads the size from the buffer.

[mannol]

Ok, I just added a size argument to bin_load() and ran a fuzzing test and it didn't segfault. Having binn_is_valid_ex() in the API would be great.

[mannol]

Actually, I did another test with size data valid but everything else invalid and it crashes at the ~same spot because count is invalid. Is there a way to calculate count from the received data?

[kroggen]

The count is also inside the buffer, but if the content is random or corrupted then it will fail.

The best thing to use to check integrity of the whole data buffer is a checksum. A basic crc32 would solve it.

But note that the transport protocols already have it, so maybe you will not need to implement another one.

[mannol]

What stops a potential attacker from faking checksum by providing valid checksum for invalid data?

[kroggen]

Good point!

I'll see what I can do to avoid a SEGFAULT in these cases.

[mannol]

What a timing! I have just ran a test on a modified binn_is_valid() using a correct size but everything else invalid and worked out a fix: 1) valid size of the buffer must be known to the binn_is_valid() 2) verify that the header size and the valid size match 3) check for the buffer overflow/underflow on every increment of the p buffer iterator in binn_is_valid() such that p + increment is always < plimit

NOTE: increment can sometimes be a negative value so that should be taken into consideration.

[kroggen]

Yeah!

This is exactly what I am doing. Inside the AdvanceDataPos() function too.

Nice!

[kroggen]

But I am not doing underflow checking. It is a good point too as an internal content size can be malformed (negative). Thanks!

[kroggen]

mannol, if you wanna contribute you can send a PR. Or share code here.

I'm afraid to change the way the buinn_is_valid() function works, as many people already use it.

We could make it consider the value that is in the *psize argument is the size, but current code work without initializing it, like this:

int size;
if (binn_is_valid(buf, NULL, NULL, &size) == FALSE) xxx;

to get the current size. This is the reason I guess we should use another (additional) function. And one can even call the other to reduce code.

[mannol]

This is my dirty working function. Basically, there is no way to keep API backward compatibility with the existing function, hence a new function is needed. Either way it should be pointed out that this version must be used for verifying data received from network.

[kroggen]

Thank you! I uploaded some modifications too, to the AdvancedataPos() function and where it is called.

[mannol]

I "reviewed" your commit and added some comments. Looks much cleaner than my solution.

[kroggen]

OK, I will need to exit now. But today at night I continue the work.

[kroggen]

I sent an update containing the binn_is_valid_ex() function.

You can check some usage info here.

Let's check if it passes the fuzzy test now ;)

[mannol]

Fix confirmed! Tests I've ran:

• Parsing of a fully random data
• Parsing of a random data with valid size field
• Parsing of a partially valid data with garbage

[kroggen]

Wow!

Thank you mannol.

It was nice to work with you