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

Jump to bottom

Fix memory allocation size overflows #4747

Closed
DorpsGek opened this issue Aug 25, 2011 · 5 comments
Closed

Fix memory allocation size overflows #4747

DorpsGek opened this issue Aug 25, 2011 · 5 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

monoid opened the ticket and wrote:

There are several locations where the size of memory to be allocated uses a calculation that can overflow, which can lead to a smaller amount of memory than expected to be allocated, allowing possible heap modification. Indeed, this is currently possible with a crafted BMP image of certain dimensions.

The allocation helper routines in alloc_func.hpp alone suffer from this flaw; namely in the calculation of num_elements * sizeof(T).

This patch attempts to fix this by:
* Checking the internal size calculations in alloc_func.hpp helpers for overflow
* Changing the size parameter for these helpers from size_t to int64, which allows the use of OverflowSafeInt64's in the calculations of other sizes
* Using OverflowSafeInt64s as just mentioned, in size calculations that maybe/are vulnerable to overflow.
* A few size calculations are manually checked.

I have attempted to keep the changes to usages of the alloc_func.hpp helpers minimal but still correct; variables involved in relevant size calculations are simply wrapped with OSI64() (a typedef for OverflowSafeInt64). However, this is still a decently sized patch, and am open to suggestions on how to rework it differently.

Attachments

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/4747
@DorpsGek
Copy link
Member Author

Rubidium wrote:

Under which circumstances can either of the width or height component of a resolution become more than UINT16 max? The setting is truncated to UINT16_MAX when being read from the configuration file.

Under which circumstances can the width or height component of the chatbox's size become more than UINT16 max? The width is limited to UINT16 max by both its type as the setting validation, the height is limited by the UINT8 max (type and setting validation) of the number of lines setting, and the height of a line is limited by a setting validation to 72. So where would this overflow.

The BMP bit I can agree on, but it might be easier to reject such files just outright. They are not useful; just diallow anything above 16384x16384 would suffice I'd say.

SIZE_MAX is a size_t (unsigned long). Casting it to int64 on x64 makes it effectively -1, disallowing all allocations on x64. If there is an alloca that would be anywhere near SIZE_MAX, that would be very bad. You can never allocate that much. The stack is a few MB on Windows.

So what I am seeing is just a generic: oh, might be wrong so replace it with something that looks much better... but it's actually pointless and makes the code harder to read.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4747#comment10232

@DorpsGek
Copy link
Member Author

Rubidium wrote:

More detailed comments per fix. In my opinion it is much better to document this and put some limitations on (primarily) image loading. Then most, if not all, will never overflow.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/4747#comment10244

@DorpsGek
Copy link
Member Author

monoid wrote:

I agree, I'm not sure what I was doing with so many changes, and forgetting about 64 bit archs.

I took your detailed comments to produce a more focused patch.
The only bit I wasn't entirely sure about was the Squirrel array GetParam helper - that should be checked by someone in the know.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/4747#comment10246

@DorpsGek
Copy link
Member Author

monoid wrote:

Added a few more checks - the original sound in-file size is a dword read straight from the file, so can easily overflow.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/4747#comment10249

@DorpsGek
Copy link
Member Author

DorpsGek commented Sep 2, 2011

michi_cc closed the ticket.

Reason for closing: Implemented

In r22873 and r22874.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4747

@DorpsGek DorpsGek closed this as completed Sep 2, 2011
@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek