We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3428 - Potential buffer overrun in SDL_GetWindowWMInfo()
Summary: Potential buffer overrun in SDL_GetWindowWMInfo()
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: 2.0.5
Hardware: All Linux
: P2 normal
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-18 21:33 UTC by Frank
Modified: 2017-06-11 04:51 UTC (History)
2 users (show)

See Also:

Attachments
Testcase to show the overflow (1.03 KB, text/x-c++src)
2017-06-09 12:30 UTC, Daniel Scharrer 		Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Frank 2016-09-18 21:33:25 UTC 
The SDL_SYSWmInfo structure can vary in size depending upon the configuration flags set when the headers were configured and the local library was built.  Under the right circumstances, this can cause an overflow in SDL_GetWindowWMInfo() if the system on which the application is currently running is configured differently from the build system.

For example:

An application is built on system A which only defines SDL_VIDEO_DRIVER_X11 in the header configuration.  The size of the SDL_SysWMInfo struct passed by the application in this case will be 24 bytes.

System B, however, has a version of the SDL shared library which also enables SDL_VIDEO_DRIVER_WAYLAND and thus assumes that the size of the SDL_SysWMInfo struct is 32 bytes due to an extra pointer being returned in the Wayland system info.  However, the application is only passing a 24 byte struct into SDL_GetWindowWMInfo() which will cause an overflow if SDL is using the Wayland backend.

Adding a size parameter SDL_GetWindowWMInfo() would address this but break binary compatibility in the process.  A noninvasive method would be to replace the "int dummy" variable in the SDL_SysWMInfo union with something like "char dummy[64]" or some value which is at least as big as the largest window system struct so that it can't be too small, no matter how the header is configured.  It wouldn't fix old software, but anything compiled against it going forward should be OK.
Comment 1 Daniel Scharrer 2017-06-09 12:30:11 UTC 
Created attachment 2758 [details]
Testcase to show the overflow

I just came across this bug when trying an app that was compiled with older SDL2 under Wayland (with a Wayland-enabled SDL2).

Attached is a small testcase - running `sh sdlsyswminfo.c` under Wayland will trigger the assert.

> Adding a size parameter SDL_GetWindowWMInfo() would address this but break binary compatibility in the process.  A noninvasive method would be to replace the "int dummy" variable in the SDL_SysWMInfo union with something like "char dummy[64]" or some value which is at least as big as the largest window system struct so that it can't be too small, no matter how the header is configured.  It wouldn't fix old software, but anything compiled against it going forward should be OK.

Since this is and ABI break, SDL should also be changed to not write the info.wl.shell_surface member (or fail the SDL_GetWindowWMInfo() call entirely) unless the version member is at least as high as whatever SDL version is going to add the above padding. This would require apps that need the Wayland handles to be re-compiled against newer SDL, but would at least not break older apps that have optional X11 code but can otherwise run fine with Wayland-enabled SDL2.
Comment 2 Ryan C. Gordon 2017-06-09 21:25:43 UTC 
We'll run under the assumption that X11 is always available on Unix builds as a baseline, so you've got a C union with a pointer (Display *), and something that's pointer-sized (Window).

In this configuration, the problem pieces are:

- DirectFB (does this target even compile? There haven't been Ubuntu packages for it for years, if ever).
- Wayland.

Mir is rarely used, but fits in the same space as X11 in any case. Likewise for Vivante. Other platforms use more, but are generally the only target for those platforms, like UIKit on iOS.

So I think your idea is good: we'll pad the union to 64 bytes (8 64-bit pointers), and have wayland and directfb fail if not passed a version of at least 2.0.6 (which will definitely have this fix). I think that'll be Good Enough.

--ryan.
Comment 3 Ryan C. Gordon 2017-06-11 04:51:07 UTC 
Fixed in https://hg.libsdl.org/SDL/rev/69452f9839d5, thanks!

--ryan.