Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miramon: avoid Unsigned-integer-overflow in MMCreateExtendedDBFIndex() #9776

Merged
merged 1 commit into from Apr 26, 2024

Conversation

rouault
Copy link
Member

@rouault rouault commented Apr 26, 2024

Validate that FirstRecordOffset as computed in
MM_ReadExtendedDBFHeaderFromFile() is not negative. Otherwise it gets later passed to MMCreateExtendedDBFIndex() which casts it to a uint64_t, and thus lead to unsigned integer overflow when doing:

    fseek_function(f,
                   (MM_FILE_OFFSET)offset_1era +
                       (MM_FILE_OFFSET)bytes_acumulats_id_grafic,
                   SEEK_SET);

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68303

@AbelPau Can you check that this fix is correct? Here's the file (to be unzipped first) on which fuzzers/ogr_miramon_fuzzer can trigger the unsigned integer overflow (no crash, but unsigned integer overflows are bugs in 99% of the cases):
ossfuzz_68303.zip

I'm wondering if the ultimate fix would not to change MM_FIRST_RECORD_OFFSET_TYPE to be a unsigned int32 rather than a signed int32. But perhaps not. I guess that it would not be common to have the first record offset beyond 2 GB...

Validate that FirstRecordOffset as computed in
MM_ReadExtendedDBFHeaderFromFile() is not negative. Otherwise it gets
later passed to MMCreateExtendedDBFIndex() which casts it to a uint64_t,
and thus lead to unsigned integer overflow when doing:
```
    fseek_function(f,
                   (MM_FILE_OFFSET)offset_1era +
                       (MM_FILE_OFFSET)bytes_acumulats_id_grafic,
                   SEEK_SET);
```

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68303
@AbelPau
Copy link
Contributor

AbelPau commented Apr 26, 2024

Validate that FirstRecordOffset as computed in MM_ReadExtendedDBFHeaderFromFile() is not negative. Otherwise it gets later passed to MMCreateExtendedDBFIndex() which casts it to a uint64_t, and thus lead to unsigned integer overflow when doing:

    fseek_function(f,
                   (MM_FILE_OFFSET)offset_1era +
                       (MM_FILE_OFFSET)bytes_acumulats_id_grafic,
                   SEEK_SET);

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68303

@AbelPau Can you check that this fix is correct? Here's the file (to be unzipped first) on which fuzzers/ogr_miramon_fuzzer can trigger the unsigned integer overflow (no crash, but unsigned integer overflows are bugs in 99% of the cases): ossfuzz_68303.zip

I'm wondering if the ultimate fix would not to change MM_FIRST_RECORD_OFFSET_TYPE to be a unsigned int32 rather than a signed int32. But perhaps not. I guess that it would not be common to have the first record offset beyond 2 GB...

Let me do this evening. Thanks!

@AbelPau
Copy link
Contributor

AbelPau commented Apr 26, 2024

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68303

I have no permision (Permission denied.) to this Fix: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68303

Should I?

@AbelPau
Copy link
Contributor

AbelPau commented Apr 26, 2024

FirstRecordOffset is read directly from disk (2 bytes).
Whether it's signed or unsigned, the maximum can always be reached.
In our specification it's signed, so, I find a good solution asking if it's positive to make it a possible value.
It's right to think that it would be difficult to find a value so big, but you never know.
So, for me your solution is good.

@rouault
Copy link
Member Author

rouault commented Apr 26, 2024

I have no permision (Permission denied.) to this Fix: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68303

yes, ossfuzz bugs may be a matter of security and have restricted access until they are fixed, or after 90 days of being opened without fixes. I could add your email if you're interested, but this needs to be attached to a Google account (so in practice, a gmail email, or a corporate email linked to a google account)

In our specification it's signed, so, I find a good solution asking if it's positive to make it a possible value.

technically/pedantically, it is illegal in C to cast a value from unsigned to signed, if the unsigned value would lead to a negative signed value (too "smart" compilers could potentially optimize away a check for a negative value, because they would think that it is impossible...). Hence I compute the value on a unsigned, and check if it is not larger than the maximum signed value.

@rouault rouault merged commit a2f4e31 into OSGeo:master Apr 26, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants