Subtruct with overflow (maybe not dangerous)

[anfedotoff]

Hi! Using our Sydr checkers we found an overflow here.

thread '<unnamed>' panicked at 'attempt to subtract with overflow', /goblin/src/pe/utils.rs:58:14

I had a brief look, it seems not dangerous, what do you think?

[m4b]

I'm not sure, since that returns some number someone might use, it could end up not being great? However I'm not sure what it would help by adding a saturating_sub in those additions / subtractions, since the bug would still be there, and isn't clear to me if one number converges to 0 if the other additions will just be as equally bad?

That could would also likely suffer readability with saturating forms, though that is fixed easy enough. it's probably fine i guess? I'd be more curious what a good default "zero" value would be there if we're getting bogus numbers that contribute to it as input.

[anfedotoff]

Yes, you are right, readability will suffer if we add some checks. My idea was to stop parsing and return an error if overflow occured. But if we couldn't say now that this overflow could lead to some error, I propose to do nothing with it. As, I said before, goblin is used in our tools (Sydr & Casr). We plan to do some continuous fuzzing for goblin on our side. So, if this overflow will trigger something, we will know and will handle this then. What do you think?

[philipc]

The only way I can see that overflowing is if file_alignment is not a power of 2. Is that what your Sydr checker found?

Secondly, if it does overflow, it will be a large value, and thus ignored by the following compare. I think this would be better than use saturating_sub, because saturating_sub would return a size of 0 which doesn't seem right to me.

What this code needs to do is match whatever the Windows loader does, but that might be hard to figure out.

[anfedotoff]

The only way I can see that overflowing is if file_alignment is not a power of 2. Is that what your Sydr checker found?

Sydr, somehow could overflow file_alignment. Maybe file_alignment is 0, it has usize type so there is a big value when we subtract -1. Here is the input for parse fuzz target: crash-sydr_b28e31470d668a648e83ba8d7f76ee43e245e770_int_overflow_17_unsigned.txt

Btw, we found another overflow here. But It seems, to me it is not dangerous too.

[philipc]

Maybe file_alignment is 0, it has usize type so there is a big value when we subtract -1. Here is the input for parse fuzz target:

Thanks. Yes, that has a file_alignment of 0.

Btw, we found another overflow here. But It seems, to me it is not dangerous too.

I'm not sure what you mean by dangerous. However, overflowing there would indicate a malformed file, and an error should be returned instead.

[anfedotoff]

I'm not sure what you mean by dangerous.

I mean, that there is no panic in release. Maybe return value of the function is checked by caller.

However, overflowing there would indicate a malformed file, and an error should be returned instead.

Agreed!:) I'll try to think some better check than if file_alignment == 0 {return Err();} Maybe you have any ideas?

[anfedotoff]

The alignment factor (in bytes) that is used to align the raw data of sections in the image file. The value should be a power of 2 between 512 and 64 K, inclusive. The default is 512. If the SectionAlignment is less than the architecture's page size, then FileAlignment must match SectionAlignment. I think, I know what to do:)

[koutheir]

Btw, we found another overflow here. But It seems, to me it is not dangerous too.

I think this situation can be triggered by parsing a valid PE executable that is larger than 4 GiB when usize is u32.