libFuzzer: Prevent a potential shift overflow - #4435
Conversation
The type of |base_offset| in get_delta_base() is `git_off_t`, which is a signed `long`. That means that we need to make sure that the 8 most significant bits are zero (instead of 7) to avoid an overflow when it is shifted by 7 bits. Found using libFuzzer.
|
PTAL. This one I'm not 100% sure this is the right fix. The other option being using an unsigned long to store the intermediate computation. |
|
ugh, ubsan also complains about the |
| if (left <= used) | ||
| return GIT_EBUFS; | ||
| base_offset += 1; | ||
| if (!base_offset || MSB(base_offset, 7)) |
There was a problem hiding this comment.
To me, it looks like the author of this code already assumed base_offset being unsigned. Otherwise base_offset += 1; if (!base_offset) /* Overflow */; wouldn't make any sense at all. So I think using size_t instead and then having a check whether its value is greater than the maximum value for git_off_t should be good.
| if ((size_t)delta_obj_offset <= unsigned_base_offset) | ||
| return 0; /* out of bound */ | ||
| base_offset = delta_obj_offset - unsigned_base_offset; | ||
| if (base_offset >= delta_obj_offset) |
There was a problem hiding this comment.
This can only be true iff unsinged_base_offset equals 0:
base_offset >= delta_obj_offset
<=> delta_obj_offset - unsigned_base_offset >= delta_obj_offset
The above could only be true iff unsinged_base_offset is greater than delta_obj_offset or 0, where the first case was filtered out in line 947 and the second case shouldn't ever happen. So you could just reformulate this condition as unsinged_base_offset == 0.
| } | ||
| base_offset = delta_obj_offset - base_offset; | ||
| if (base_offset <= 0 || base_offset >= delta_obj_offset) | ||
| if (unsigned_base_offset == 0 || (size_t)delta_obj_offset <= unsigned_base_offset) |
There was a problem hiding this comment.
Can delta_obj_offset ever be negative?
There was a problem hiding this comment.
A cursory glance through the code suggests that the answer to this question is no. But along those lines, should we really return 0 here or should we raise an error in the out-of-bound case?
There was a problem hiding this comment.
Oh, 0 is an error, but seems to only be checked occasionally. 😢
|
Thanks for doing this - this appears to be an improvement. |
The type of |base_offset| in get_delta_base() is
git_off_t, which is asigned
long. That means that we need to make sure that the 8 mostsignificant bits are zero (instead of 7) to avoid an overflow when it is
shifted by 7 bits.
Found using libFuzzer.