Amiberry: Buffer overflow regarding file paths

[fogobogo]

Amiberry, Amiberry SDL2, UAE4ARM, really anything based on the pandora code blows up with a buffer overflow in conjunction with filepaths.

Since archlinux arm ships with a hardening wrapper for gcc / glibc theres also quite a few compile time warnings regarding that.

Looking at the code turns up that MAX_PATH and MAX_DPATH are defined as 256 in sysconfig.h. I presume that is because on the pandora runs on a FAT32 filesystem?
Some Amiga filesystem limitation? ext4 has PATH_MAX defined as 4096.

I tried to fix my issues by expanding the buffers to PATH_MAX but I'm not familiar enough with the code so it kinda works sometimes.

The issues at hand:

-Buffer overflows. Many of them. with TOSEC having a quite wordy naming convention plus unix filesystem paths (e. g. /home/user/.../.../amiberry-sdl2) 256 bytes just aren't enough.

• MAX_PATH and MAX_DPATH are the same basically (Both defined as 256). I have no clue what the disctinction is supposed to do.

• There are a lot, and I mean a lot of instances of:
  strncpy(...);
  strncat(...);
  strncat(...);
  Although sometimes it's strcpy / strcat. That belongs into a proper function.

• There are sneaky declarations like start_path_data[256] and filename[256] still in there. Need to be replaced with PATH_MAX. Again, limits.h is your friend.

• querying d_type for listing a directory is linux only. May want to guard that with an ifdef and use stat for posix systems. Though it probably won't run anywhere else anytime soon.

Heres is a build log (just stderr) for the plain vanilla amiberry-sdl2 branch on archlinux arm with an ext4 filesystem.
Not so plain vanilla actually since I removed the -ltmalloc -ltprofiler LDFLAGS and ifdef'd gperftools.h. Don't have that installed here and that header needs an ifdef DEBUG.
Look out for:
/usr/include/bits/string3.h:155:71: warning: call to char* __builtin___strncat_chk(char, const char, unsigned int, unsigned int) might overflow destination buffer

As it stands I'm amazed it runs at all even on DietPi / Raspbian since the way I see it it totally completely smashes the stack. The way it is now file handling is flimsy at best.

Steps to reproduce:
-Build amiberry with hardening-wrapper.
-Try opening a directory listing.
-???
-Enjoy core dump

build.txt

[midwan]

@fogobogo
Thank you for the detailed report. This helps a lot in making things better. :)

I don't have an Arch installation to test with currently, but I'll see if I can set one up for future reference. Meanwhile, I'll definitely look into what you mentioned above and start improving things.

[fogobogo]

Thanks a lot. In fact using the hardening-wrapper should be enough to get the same results. Not sure if DietPi / Raspbian supplies one. apt search might help.

[midwan]

@fogobogo
I've pushed some updates that should fix the buffer overflows. If you could test from your side as well, it would be great.

I'm not sure why there's a MAX_PATH and MAX_DPATH, these two came from WinUAE originally. Even in FS-UAE, there is a size difference between them, MAX_DPATH = 1000 and MAX_PATH = PATH_MAX (from limits.h). I've updated the code in Amiberry to be the same, but if we find any problems with this I can easily make them the same size (which then makes one of the two obsolete also).

[fogobogo]

Well, it dies instantly:
* buffer overflow detected *: ./amiberry-sdl2 terminated
Aborted (core dumped)

I'll do a build with debug symbols and see if I can find out more. Still a lot warnings about potential buffer overflows too.

[fogobogo]

Oddly enough, the debug build (DEBUG=1) runs just fine. So I did a build with debug symbols and without striping. ( -g and removing $STRIP ) this had the... uhm, desired(?) result.

Using host libthread_db library "/usr/lib/libthread_db.so.1".

* buffer overflow detected *: /home/fogobogo/Builds/amiberry-debug/amiberry-sdl2 terminated

Program received signal SIGABRT, Aborted.
0x768a5a38 in raise () from /usr/lib/libc.so.6
(gdb) bt

0 0x768a5a38 in raise () from /usr/lib/libc.so.6

1 0x768a7148 in abort () from /usr/lib/libc.so.6

2 0x768e2a40 in __libc_message () from /usr/lib/libc.so.6

3 0x769625ac in __fortify_fail () from /usr/lib/libc.so.6

4 0x76960310 in __chk_fail () from /usr/lib/libc.so.6

5 0x7695f8f0 in __vsnprintf_chk () from /usr/lib/libc.so.6

6 0x7695f7e0 in __snprintf_chk () from /usr/lib/libc.so.6

7 0x54adcfd0 in snprintf (__fmt=0x54e596f8 "%s/saves/default.ads", __n=4096, __s=) at /usr/include/bits/stdio2.h:65

8 main (argc=1, argv=0x7efffa04) at src/osdep/amiberry.cpp:705

Turns out the line in question is
snprintf(savestate_fname, MAX_PATH, "%s/saves/default.ads", start_path_data);

However start_path_data is MAX_DPATH so it tried to read MAX_PATH aka 4096 bytes from a 1000 byte buffer.

I changed MAX_PATH to MAX_DPATH. Starts up now and browsing folders didn't crash anything yet so far. Also send a pull request ( I think. Haven't done much like that on Github yet )

[solskogen]

Probably related:
Adding a directory as a hard drive will crash when the emulator starts.

[fogobogo]

Just tried that, however it dies on me with:

...
processed: 1988487660
IP: 0x7685e9ec [e79e3103] 2730573824
processed: 1988487664
* stack smashing detected *: ./amiberry-sdl2 terminated
IP: 0x76f4c4d4 [e592e000] 2730573824
processed: 1995752664
IP: 0x76f4c4f0 [e5123008] 2730573816
processed: 1995752692
IP: 0x76f4c4d4 [e592e000] 2730573824
processed: 1995752664
IP: 0x76f4c4f0 [e5123008] 2730573816
processed: 1995752692
IP: 0x76f4c4d4 [e592e000] 2730573824
processed: 1995752664
IP: 0x76f4c4f0 [e5123008] 2730573816
processed: 1995752692
IP: 0x76f4c4d4 [e592e000] 2730573824
processed: 1995752664
IP: 0x76f4c4f0 [e5123008] 2730573816
processed: 1995752692
Aborted (core dumped)

doubt it's related though. Looks more like the emulation core has trouble

[midwan]

@fogobogo
Looks like I'll need to setup a similar environment to see all these errors, as it works fine in the systems I've been testing with.

Note: You mentioned you modified the compile flags, to remove gperftools. If you do that, you will also need to remove the line that disables the built-in malloc functions, since tcmalloc would replace them normally.

Perhaps that is related to the errors you see above? Not sure, since I can't recreate it without a similar environment.

[fogobogo]

After looking at the gui code some more I'm semi-tempted to write my own. There's just so much wrong with it.

-Hardcoded values everywhere, e.g. I try to fix a compiler warning about overflowing the buffer, turns out changed_prefs.description is hardcoded to 255. Possibly others too. Fine if the NAME_MAX is 255 in limits.h (which it is, but can potentially blow up and MAX_DPATH is 1000 now.). Also, there is diskname[31] = '\0'. No checks in place.

-MAX_PATH and MAX_DPATH with little rhyme or reason. Sometimes it seems to be a filename, sometimes a filepath. start_path_data[MAX_DPATH] is a path and the next minute you have filename[MAX_DPATH]. I presume the original idea was to use PATH_MAX / NAME_MAX. However it's all mixed up.

-Theres so much string handling in triplicates over and over. Major effort to weasel them all out and replacing them with a solid function. method. whatever.

-I don't seem to be able to use uppercase characters? Shift key does nothing. Anyone else?

On a brighter note, the emulation core seems to be grand. When it runs, it runs splendid.
It's just that the gui code looks like hacked together in a drunken night with copious use of copy/paste and bugtested by "it didn't crash in the last 5 minutes". There's certainly much to do. Currently pondering if the interface code as it is worth salvaging.

[midwan]

@fogobogo
I agree that the code could be improved. Much of this has been inherited from WinUAE and the rest from Pandora and I haven't really gone through all of the areas yet. I just started working on the GUI recently, mostly with the aim to enable custom mapping on the controls.

Pull requests are welcome! :)

[fogobogo]

Alright, I commented out that line in the Makefile.

Results so far:

Hard drive folder still crashes. (same error.). Compiler warning about potential buffer overflow shows no changes.

However, I did note some changes (for the better). Floppy creation dialog now asks for a filename, before it wouldn't show up, but crash nothing either. Silent error there.

I completely missed that line. Thanks for the tip!

[fogobogo]

creating a hardfile shows me:
Path: [ #@!#w&@$%^#go/Builds/amiberrry-sdl ]
(something like that. isn't exactly easy to make a screenshot when you can't alt-tab)

Obviously some buffer gets corrupted there or did overflow. I'll have a look later, guess its something hardcoded.

[midwan]

@fogobogo
I'll try to setup an ArchLinux installation tonight to get a first-hand experience on this.
Any details I should go through to get it up to the same level as yours? Or is everything kind of "standard" ?
I haven't installed Arch before so it will be a bit of a learning experience also. :)

[fogobogo]

Oh my, that's a bit of an effort. Arch doesn't come with a neat desktop or anything.

-Most important is probably that I built sdl2 for the pi.
The one from the repos is just bog standard with opengl enabled which doesn't sit so well if you want opengles.
-$PATH must be modified to include /opt/vc.
-sshd must be enabled if you use that. In fact you will want that.
-be sure to change the systemctl / journalctl. settings if you don't want to eternally wait for core dumps (outright disable it.)
-And don't forget to modify your fstab for the proper mounting of your partitions.
-base-devel is the package containing the compiler and binutils.

Now, setting up Arch is a big of an undertaking especially if you haven't done it before. The simplest is probably to just stick with the tty console for the moment.

The Arch wiki is your best friend there. Can't stress that enough. Beginners Guide especially! Theres also a Raspberry Pi page there but its pretty basic and a tad outdated.

For the Pi you need to apply as neccessary. I honestly think that will take you longer than rewriting the whole gui. It's pretty rewarding in the end but especially on ARM Arch doesn't come with Pi tailored packages like Raspbian or Retropie does.

[midwan]

@fogobogo
No worries, I'll give it a try and see how it goes... :)

[fogobogo]

Almost forgot about the AUR. with abs (basically an rsync script) you can request any PKGBUILD (bascially a script with build instructions for makepkg) from the repos and modify it to your heart's content. makepkg will then build you an package out of it.

The AUR on the other hand is where anyone really can dump his PKGBUILDs for otheres to use.

Don't miss out on that on the wiki

[solskogen]

You must compile SDL2 your self, or else the rpi output will not work.
Just add --host=arm-raspberry-linux-gnueabihf to the configure line.

On Wed, Apr 19, 2017 at 2:52 PM, fogobogo [e-mail] wrote:

Almost forgot about the AUR. with abs (basically an rsync script) you can
request any PKGBUILD (bascially a script with build instructions for
makepkg) from the repos and modify it to your heart's content. makepkg
will then build you an package out of it.

The AUR on the other hand is where anyone really can dump his PKGBUILDs
for otheres to use.

Don't miss out on that on the wiki

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/midwan/amiberry/issues/85#issuecomment-295259157, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA-P6ORdAaaYQsjkkEVt10uZ2BHK0oSXks5rxgOYgaJpZM4NACvH
.

[midwan]

@solskogen
Thanks, I was compiling SDL2 myself anyway to enable OpenGLES2 support from the console so that shouldn't be a problem.

[midwan]

OK, silly question...

I have Arch up and running, already compiled SDL2, SDL2_image and trying to compile SDL2_ttf (the final piece).
It had a dependency on freetype2which I installed, but now during building it complains that it can't find -lGL. I noticed that it's using a -DHAVE_OPENGL declaration also.

Did you install OpenGL to get that one to compile or is there a way to configure it so that it doesn't look for it?

[fogobogo]

I installed libgl and freetype / sdl2_ttf is just the standard from the repo. package name is mesa I think.

[midwan]

@fogobogo
Thanks, with package mesait compiled fully.

Now compiling Amiberry-SDL2 to see how it goes.
Is the hardening wrapper installed with the base-devel package or do I have to enable it somehow? I'm not getting any warnings/errors so far (though it's not finished yet).

[midwan]

Compilation finished.

Running Amiberry-SDL2 and testing file / folder requesters:

• Hard drive directory selector: no problem
• Floppy file selector: no problem

Which scenario gives you a core dump currently?

[fogobogo]

Should come as default. Otherwise hardening-wrapper is the package name.

[fogobogo]

That was before you upped the defines for MAX_PATH and MAX_DPATH. However, try opening a CD32 image. See you after the reboot.

[midwan]

Thanks, I'll try that

[solskogen]

I use the sdl2 branch (latest and greatest) and it still crashes (Why can't I see the commits done by fogobogo?!)
This is my config https://pastebin.com/NVML73UL

[midwan]

@solskogen
The changes are not merged in yet... :)

[midwan]

@fogobogo
Merged