SIGSEGV in binn_load() when fed with random data about binn HOT 20 CLOSED

Yeah!

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

Nice!

from binn.

Wow!

Thank you mannol.

It was nice to work with you

from binn.

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.

from binn.

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.

from binn.

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?

from binn.

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.

from binn.

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.

from binn.

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?

from binn.

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.

from binn.

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

from binn.

Good point!

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

from binn.

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.

from binn.

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

from binn.

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.

from binn.

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.

from binn.

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

from binn.

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

from binn.

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

from binn.

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 ;)

from binn.

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

from binn.