probonopd commented 5 months ago

Closes https://github.com/AppImage/type2-runtime/issues/31

NOTE: The code in this PR is currently wrong. We should do what is described in https://github.com/AppImage/type2-runtime/pull/32#issuecomment-2206987685 instead. Coding help is welcome.

UPDATE: Consider merging

37

instead.

probonopd commented 4 months ago

Build for testing: https://github.com/AppImage/type2-runtime/actions/runs/9014294337/artifacts/1487021420

Hi @Samueru-sama, can you confirm that this runtime fixes the issue at https://github.com/ivan-hc/Chrome-appimage/issues/3?

Ping @TheAssassin, can you please do a review?

Samueru-sama commented 4 months ago

It works! (htop2 is the one that I made manually by making the squashfsimage and then adding this runtime with cat).

However I don't think it is needed to reinstate the look in $PATH, what needs to be changed is instead looking for /bin/fusermount, /bin/fusermount3, etc because that location will always exist in all distros, just that some symlink it to /usr/bin

No distro will ever get rid of the /bin symlink, that would break millions of scripts.

2011 commented 4 months ago

However I don't think it is needed to reinstate the look in $PATH, what needs to be changed is instead looking for /bin/fusermount, /bin/fusermount3, etc because that location will always exist in all distros, just that some symlink it to /usr/bin

No distro will ever get rid of the /bin symlink, that would break millions of scripts.

Gentoo does not have /bin/fusermount or /bin/fusermount3. Those binariies exist only in /usr/bin. If I understand the problem (at least as it affects me), AppImages should test to see if they can execute /usr/bin/fusermount or /usr/bin/fusermount3, not test if they can read them.

Samueru-sama commented 4 months ago

However I don't think it is needed to reinstate the look in $PATH, what needs to be changed is instead looking for /bin/fusermount, /bin/fusermount3, etc because that location will always exist in all distros, just that some symlink it to /usr/bin No distro will ever get rid of the /bin symlink, that would break millions of scripts.

Gentoo does not have /bin/fusermount or /bin/fusermount3. Those binariies exist only in /usr/bin. If I understand the problem (at least as it affects me), AppImages should test to see if they can execute /usr/bin/fusermount or /usr/bin/fusermount3, not test if they can read them.

You're right, when I said look I meant execute as I did the in the screenshot of alpine, sorry I didn't make that clear.

Do you still have the problem with the testing build? If you need the htop2 I made with it to test it let me know.

probonopd commented 4 months ago

AppImages should test to see if they can execute /usr/bin/fusermount or /usr/bin/fusermount3, not test if they can read them.

If fusermount* is on the $PATH but cannot be executed, then I'd say it's a broken FUSE setup?

Bqleine commented 4 months ago

This patch is odd, firstly it calls access() after already checking the access rights using fopen("r"), secondly the char fullPath[256] might work but there's no guarantee paths can't be longer than this. And lastly why are we not using execvp, which is guaranteed to search PATH correctly, and even more?

p - execlp(), execvp(), execvpe() These functions duplicate the actions of the shell in searching for an executable file if the specified filename does not contain a slash (/) character. The file is sought in the colon-separated list of directory pathnames specified in the PATH environment variable. If this variable isn't defined, the path list defaults to a list that includes the direc‐ tories returned by confstr(_CS_PATH) (which typically returns the value "/bin:/usr/bin") and possibly also the current working directory; see NOTES for further details.

probonopd commented 4 months ago

Thanks @Bqlein . fusermountXX is a setuid root program, so we must check whether it is executable by root, not by the current user.

So to my understanding we should:

Use execvp for executing programs, which handles setuid permissions and PATH searching correctly.
Avoid implementing custom checks like stat, findBinaryOnPath specifically for setuid execution verification if using execvp.
Ensure dynamic memory allocation or sufficient buffer sizes when constructing file paths to avoid issues with fixed path lengths. Especially we should not use char fullPath[256]
Focus on robust error handling around execvp calls to manage cases where execution fails due to permissions, missing binaries, or other errors.

Does this sound about right?

Bqleine commented 4 months ago

Thanks @Bqlein . fusermountXX is a setuid root program, so we must check whether it is executable by root, not by the current user.

No, that is not how setuid works.

So to my understanding we should:

* Use `execvp` for executing programs, which handles setuid permissions and PATH searching correctly.

* Avoid implementing custom checks like `stat`, `findBinaryOnPath` specifically for setuid execution verification if using `execvp`.

* Ensure dynamic memory allocation or sufficient buffer sizes when constructing file paths to avoid issues with fixed path lengths. Especially we should not use `char fullPath[256]`

* Focus on robust error handling around `execvp` calls to manage cases where execution fails due to permissions, missing binaries, or other errors.

Does this sound about right?

Yes

probonopd commented 2 months ago

@TheAssassin wdyt?

TheAssassin commented 2 months ago

I think someone had some thoughts on the security of such an approach and advocated for hardcoding a list. I think you remember the discussion. I can't find it right now. Do you have a link?

probonopd commented 2 months ago

Please, lets not hardcode a list. Every time we do this there is at least one example where it doesn't work. Let's use execvp for executing programs, which (according to this discussion) handles setuid permissions and PATH searching correctly. Let's not worry about malicious binaries in the PATH, because if they are there I can imagine much, much easier attack vectors than anything related to AppImage. Really.

TheAssassin commented 2 months ago

That does not really answer my question, though. Do you remember those concerns? I'd like to review them myself.

probonopd commented 2 months ago

If I remember correctly, these were not concerns voiced by someone, but we deduced this from the fact that fuse compiles absolute paths in at compile time rather than searching at runtime.

probonopd commented 2 months ago

Converted this PR to a draft because the code in this PR is currently wrong. We should do what is described in https://github.com/AppImage/type2-runtime/pull/32#issuecomment-2206987685 instead. Coding help is welcome.

probonopd commented 2 months ago

https://www.reddit.com/r/linux/comments/1d60t1r/comment/lh1qxf2/

on alpine fusermount is in /bin instead of /usr/bin, and currently the static runtime doesn't work as result.

Finding it dynamically using execvp should (hopefully) fix this.

Samueru-sama commented 2 months ago

https://www.reddit.com/r/linux/comments/1d60t1r/comment/lh1qxf2/

on alpine fusermount is in /bin instead of /usr/bin, and currently the static runtime doesn't work as result.

Finding it dynamically using execvp should (hopefully) fix this.

Or you could just hardcode the path to fusermount to /bin instead of /usr/bin.

Because when fusermount is on /usr/bin then /bin would be a symlink to /usr/bin and everything will still work, like it does for the millions of scripts that have the #!/bin/sh shebang.

However because it is hardcoded to /usr/bin and that location is never symlinked to /bin it breaks as result in the very few distros that still keep the bin separation.

(I'm the person of the reddit comment btw).

probonopd commented 2 months ago

Note to self: mount.c.zip (Editing diff files is not fun...)

probonopd commented 2 months ago

@Samueru-sama pretty sure there will be the odd distribution out there placing it in another place. Hence we want to use execvp to try fusermount3, fusermount2, fusermount2, fusermount4...99, in this order. execvp should take care of finding the binary on the PATH and should also take care of the fusermount* binary being setuid root.

Samueru-sama commented 2 months ago

@Samueru-sama pretty sure there will be the odd distribution out there placing it in another place. Hence we want to use execvp to try fusermount3, fusermount2, fusermount2, fusermount4...99, in this order. execvp should take care of finding the binary on the PATH and should also take care of the fusermount* binary being setuid root.

I don't think such distro actually exists, if it did it would have bigger problems than just appimages not working. Also the writing is on the wall and every distro will eventually merge everything under /usr/bin (with the /bin symlink), Gentoo recently posted some updates about no longer supporting a separated bin not related to this issue.

So far the only distros where this has been a problem are alpine and an old version of linuxmint, because they have fusermount in /bin, that version of linux mint is one year away from having its support ended, so only alpine will remain and that one isn't known for being appimage compatible anyway lol.

There is also some weird issue on Gentoo, which seems to be lack of read permissions on /usr/bin by the regular user, I'm not very sure what is going on there.

Right now I would say that the current way the runtime is written covers 96% of linux users, if it gets changed to look in /bin it would cover 99.99999% instead.

One also has to weight if it is worth it to restore the look in PATH just for that small possibility. I personally don't think it is a big deal, but I also think that if it is done for such very odd possibility it isn't good either.

And also if the hardcoded path is going to be kept, I think the FUSERMOUNT_PROG check has to go away as well, because any attacker can use that env variable instead of PATH.

probonopd commented 2 months ago

I'd not be surprised if GoboLinux and NixOS put stuff elsewhere. Hence, we want to let execvp figure that out, and not specify any predetermined path.

Samueru-sama commented 2 months ago

I'd not be surprised if GoboLinux and NixOS put stuff elsewhere. Hence, we want to let execvp figure that out, and not specify any predetermined path.

NixOS is explicitly non-FHS compliant, appimages will never work there even if the runtime gets the right path.

On Nix instead appimages have to be launched with an utility that simulates an FHS enviroment iirc.

@mgord9518 Sorry for the ping again, but iirc you are a Nix user, what do you think of this entire discussion?

mgord9518 commented 2 months ago

@Samueru-sama I like discussions so no need to apologize.

Yeah, NixOS installation locations are not only non-FHS, they're practically arbitrary. You can install multiple versions of any application/library, and every version will have a different path under /nix/store. That being said, it is possible to support Nix, it would probably just take extra work. I've done similar patches to some of my projects, you basically just have to "grep" /nix/store for the name of the package, then pick the latest version

Samueru-sama commented 2 months ago

Yeah, NixOS installation locations are not only non-FHS, they're practically arbitrary. You can install multiple versions of any application/library, and every version will have a different path under /nix/store. That being said, it is possible to support Nix, it would probably just take extra work. I've done similar patches to some of my projects, you basically just have to "grep" /nix/store for the name of the package, then pick the latest version

Interesting, I wonder why don't you use the appimage launch thingy that nixos has?

Also may I know what happens if you run which fusermount on nix?

The current issue we have is that the static runtime is hardcoded to look for fusermount* in /usr/bin which covers most distros but there are a few exceptions where fusermount is in /bin instead.

I think the simple solution is to change the hardcoded path to /bin/fusermount*, because after all the problem is the distros that keep bin separate, the ones that have it unified have /bin being a symlink to /usr/bin and it isn't a problem therefore.

Another way to fix it is to restore the look in PATH, that has some security implications though, I don't think they are a big deal but they are there nonetheless.

mgord9518 commented 2 months ago

@Samueru-sama to be completely honest, I don't really use AppImages much anymore, when I do it's typically something on a secondary system that doesn't run Nix. I work 12s so I don't really have a lot of free-time to test stuff but I'll mess around in a couple days when I'm off and come back with my findings

I can say off the top of my head though that which fusermount points to somewhere in /nix/store

Bqleine commented 2 months ago

I think someone had some thoughts on the security of such an approach and advocated for hardcoding a list. I think you remember the discussion. I can't find it right now. Do you have a link?

You might be thinking about #37. It is a new version of the patch I made with only looking for fusermount and fusermount3 on execvp. Which is IMO the best solution to this problem, which should work on every distro.

I really don't understand why #37, which fixes problems in this PR and others is not getting attention as it is ready to be used and already better than the existing solution. probonopd has concerns about not searching for further versions, but we don't think its a good idea to try to use something that

doesn't exist
users would never have expected to exist
will probably not be compatible with the runtime anyways because it would be a major version change.
will get symlinked by distros from the previous version if its compatible just like they already symlink fusermount to fusermount3.

probonopd commented 2 months ago

37 does not look for fusermount4...99 yet, which is an important part of the solution we want. Could you add that to #37 @bqleine? Then we probably could use #37 instead of reworking #32. Thanks!

Bqleine commented 2 months ago

I'm definitely against doing that, but I guess I will update my patch for the sake of making things advance.

I don't like how you keep ignoring the arguments we made against it though.

mgord9518 commented 2 months ago

@probonopd Like @Bqleine said, brute-forcing different versions of fusermount isn't guaranteeing that AppImages will be future proof. What if it actually ends up giving detrimental side-effects like crashes or worse? The application devs would be given wrongful bug reports

We can't rely on libfuse following this pattern of having compatible binaries under different names, it just doesn't really make sense. libfuse is probably at least a half decade from a new version and if distros decide to change the way this core system works from beneath our feet, it'll just have to be dealt with then

probonopd commented 2 months ago

My logic is very simple and unlikely to change:

If we don't search for future fusermount versions, then AppImages made today will break at some time with virtually 0 percent chance that they will continue to work
If we at least try to search for fusermount future versions, then AppImages made today have at least a larger than 0 percent chance to still work in the future
If that logic will turn out to be problematic, it can always be overridden by explicitly setting a different FUSERMOUNT_PROG variable

Hence I would highly appreciate if you could update this patch with a logic to search for future versions of fusermount. Thank you very much for your consideration.

Unfortunately FUSE doesn't make any explicit guarantees about future versions; most likely they don't understand yet why such a guarantee would be important:

https://github.com/libfuse/libfuse/issues/670

The real solution would be to convince the FUSE team to give us one stable name for that binary that will never change, and will always continue to work with all versions of fusermount (backward compatibility).So instead of trying to convince me to give up on trying to be forward compatible, it would be more productive to convince the FUSE team to be backward compatible (which is engineering best practice). Then we would not need to try to be forward compatible (which is essentially just hoping that things won't break).

I have opened a discussion on FUSE GitHub Discussions regarding this topic, maybe you would like to chime in:

https://github.com/libfuse/libfuse/discussions/1007

probonopd commented 2 months ago

In the meantime, I would really highly appreciate if someone could update this PR to include the forward-looking search logic. I'd consider to remove it once a version of fusermount with backward compatibility guarantees has landed in the oldest still-supported Linux distributions.

Samueru-sama commented 2 months ago

If we don't search for future fusermount versions, then AppImages made today will break at some time with virtually 0 percent chance that they will continue to work

Once fusermount4 comes, which is unlikely since the development of libfuse has stalled, distros that will only ship fusermount4 will symlink it to fusermount, like they already do with fusermount3.

Unfortunately FUSE doesn't make any explicit guarantees about future versions

They don't really have to, since that is why the change in the binary name would happen, however you might wanna tell them the problem that distros will just replace one for the other like a drop-in replacement and expect the devs to deal with the issues like it happened with wget2 and hopefully they will see why it is a problem.

For what is worth I don't have a problem with looking for future versions of fusermount, but I highly doubt that it will ever work.

Not to mention that there is the possibility that the binary name could change completely, a while ago I had this issue where qdbus-qt5 changed its name to qdbus6.

probonopd commented 2 months ago

Once fusermount4 comes, which is unlikely since the development of libfuse has stalled, distros that will only ship fusermount4 will symlink it to fusermount, like they already do with fusermount3.

Is it true or is it an urban myth that distrbutions symlink fusermount3 to fusermount? As far as I remember, libfuse3 is meant to be installable alongside libfuse2, and if that is true, wouldn't such a symlink create package conflicts? In any case, is there any guarantee that all distributions currently have and in the future will have such a symlink?

probonopd commented 2 months ago

Once fusermount4 comes, which is unlikely since the development of libfuse has stalled

Being a project with 20 years worth of history, AppImage thinks in long-term timeframes. When we started, no one talked about libfuse3. On the same day(!) it appeared on Ubuntu Live ISOs, they already dropped libfuse2, giving exactly 0 days transition period. I'd like to prepare better for the next such "transition".

For what is worth I don't have a problem with looking for future versions of fusermount, but I highly doubt that it will ever work.

At least better than not even trying imho.

Samueru-sama commented 2 months ago

Is it true or is it an urban myth that distrbutions symlink fusermount3 to fusermount? As far as I remember, libfuse3 is meant to be installable alongside libfuse2, and if that is true, wouldn't such a symlink create package conflicts?

Yes, you can actually see it in the fuse3 package in debian it contains a fusermount symlink to fusermount3.

Arch linux does not have it, but that is because archlinux has separate fuse2 and fuse3 packages that can be installed together.

In fact before there was this issue, that the appimages with the static runtime wouldn't work without the fusermount symlink to fusermount3

Also fun fact, on archlinux despite being a rolling release distro, fuse2 is still the "main" fuse used for ntfs-3g and mtpfs.

In any case, is there any guarantee that all distributions currently have and in the future will have such a symlink? At least better than not even trying imho.

Okay, it is possible for the symlink to be missing on fuse4-only distros in the future, but that would be because they realized that it doesn't actually work and that it causes more problems with the applications that expect fusermount.

probonopd commented 2 months ago

Arch linux does not have it, but that is because archlinux has separate fuse2 and fuse3 packages that can be installed together.

That's what I mean. We cannot rely on a symlink to be there. In fact, adding the symlink breaks being able to install multiple versions of libfuse alongside each other, something that libfuse is explicitly designed to do.

Bqleine commented 2 months ago

Wouldn’t that mean a user in the future could just install fuse3 to run his appimage if he only has fuse4?

mgord9518 commented 2 months ago

The issue is that AppImages are intended to just work. It's an uphill battle though, the Linux kernel is solid as a rock but userspace libraries don't give a rat's ass about binary compatibility

probonopd commented 2 months ago

Wouldn’t that mean a user in the future could just install fuse3 to run his appimage if he only has fuse4?

Probably. But if distributions do what they did last time, they will drop fuse3 from the ISOs as soon as they have fuse4 packaged.

mgord9518 commented 2 months ago

Alright, I've been doing a little experimenting. I noticed that for some reason my libfuse-zig example project works on NixOS even with static linking. This initially only gave me more questions as my system doesn't even have /usr/local/bin, which is how I have it configured to build libfuse.

I took a peek at libfuse's code and found this. A fairly recent change that has it search the PATH if fusermount cannot be executed by the full pathname that's configured at compile time

eli-schwartz commented 2 months ago

@Samueru-sama,

I don't think such distro actually exists, if it did it would have bigger problems than just appimages not working. Also the writing is on the wall and every distro will eventually merge everything under /usr/bin (with the /bin symlink), Gentoo recently posted some updates about no longer supporting a separated bin not related to this issue.

Hi, I'm the author of that update. Gentoo is not going to support having /usr be un-mounted during early boot, and will no longer hack packages into a million pieces to replace their shared libraries with linker scripts that make it work by moving shared libraries to /lib instead (which wreaks havoc on the compiler as it cannot handle having the libfoo.a in one directory but libfoo.so in another directory).

The hacks that were tried ended up breaking userland (stuff like dlopen() of PAM, for example) and weren't worth the pain.

We continue to install /bin/bash and /bin/sh rather than /usr/bin/*sh. More generally we continue to install any binary that was historically in /bin on Gentoo, in the place that it was expected to be. This doesn't apply to every single binary, of course.

There are no plans at the current time for us to remove support for split /usr as long as you handle prompt mounting of the partition at early boot (e.g. by using an initramfs), so we are consequently committed to maintaining all existing infrastructure for ensuring that binaries which belong in /bin end up installed there.

I'm not sure off the top of my head what the history of fusermount on split-usr systems was, so I cannot answer the question of whether it is in /bin or /usr/bin on Gentoo, not can I answer the question of where it is "supposed to" be installed.

TheAssassin commented 2 months ago

I took a peek at libfuse's code and found this. A fairly recent change that has it search the PATH if fusermount cannot be executed by the full pathname that's configured at compile time

So, I guess security wise that makes sense. I've been thinking about it in the past few days and I can't think of an attack vector that wouldn't require access much worse than installing a fake fusermount binary. I don't mind searching it on the $PATH either.

I'm a bit sad that unmounting requires privileged syscalls which in turn requires a setuid binary (or maybe PolKit). But then again, if FUSE even do it, it should be fine. Again, I cannot think of an attack vector. And I can't find that old discussion, so I can't judge how much sense it made.

TheAssassin commented 2 months ago

P.S.: We probably don't even need to patch it in then, we just need to update libfuse. Have those changes been released yet?

Edit: nope. But I'm willing to include the upstream change as a patch in the repository until a new release is available.

Edit 2: one thing to consider: the libfuse patch expects the fusermount program to have a specific hardcoded name, but according to @probonopd it may or may not have a 3 version number suffix.

eli-schwartz commented 2 months ago

There is a common and obvious attack vector for a library that doesn't exist for an application. Libraries should be safe to link into a setuid binary.

Not a libfuse issue, actually a golang binding for FUSE issue, but... e.g. this is a thing that happened: https://keybase.io/docs/secadv/kb002

It's an attack scenario not because you could run a command in $PATH that you didn't expect, but because a setuid binary could run a command in $PATH when it thought it was running the one in /usr/bin or /bin. One solution for this is sanitizing $PATH to a known "safe default", such as /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin.

I don't think this matters to appimage anyway because appimage is not setuid, so PATH injection doesn't allow smuggling code past a permission boundary.

eli-schwartz commented 2 months ago

I'm not sure off the top of my head what the history of fusermount on split-usr systems was, so I cannot answer the question of whether it is in /bin or /usr/bin on Gentoo, not can I answer the question of where it is "supposed to" be installed.

The upstream libfuse codebase simply uses autoconf / meson --libdir to set the location of fusermount / fusermount3. On gentoo this installs to /usr/bin.

mount.fuse does get installed for fuse2 as MOUNT_FUSE_PATH=${EPREFIX}/sbin, which is a Gentoo Prefix compatible way to do what the builtin autotools makefiles did, namely, default MOUNT_FUSE_PATH to /sbin.

with fuse3, mount.fuse3 gets installed to the location of --sbindir rather than enforcing or even recommending /sbin. Gentoo doesn't override this.

So on Gentoo, you get:

/usr/bin/fusermount
/usr/bin/fusermount3
/sbin/mount.fuse
/usr/sbin/mount.fuse3

mgord9518 commented 2 months ago

If the goal is maximum forwards-compatibility, maybe the runtime should have a baked-in way to fix itself in the (likely) case libfuse breaks compatibility again?

Let's say a user attempts to launch an AppImage and the mount fails due to "missing" fusermount, a popup then appears asking the user how they want to fix it. The options would be something like "Attempt to update runtime", "Extract and run" or "Quit"

For fixing the runtime, it would download a presumably maintained runtime version from GitHub or somewhere else stable then patch the AppImage. This would only ever happen if the AppImage wouldn't be able to start anyway, hopefully saving the user from a headache

Samueru-sama commented 2 months ago

@mgord9518 I was talking about that here but probono doesn't like the idea.

TheAssassin commented 2 months ago

I'd rather avoid linking UI code into the runtime, this would introduce a lot of bloat. We already link to libnotify IIRC, we could show a notification instead. At least tell the user the AppImage couldn't launch.

Do we need fusermount just for unmounting or for mounting? I'd assume the latter.

mgord9518 commented 2 months ago

@TheAssassin Just mounting. You can unmount a fuse FS using the umount command (unprivileged) on every distro I've tested (Arch, Pop, NixOS)

As far as I understand, using SIGINT on the fuse process also gracefully unmounts

mgord9518 commented 2 months ago

Even without libnotify, I suppose a few commands like notify-send, zenity, kdialog could be tried

mgord9518 commented 2 months ago

Been doing some digging, came across this very interesting article. Gonna mess around and see if I can get anywhere without fusermount

AppImage / type2-runtime

Find fusermount using execvp #32

37

37 does not look for fusermount4...99 yet, which is an important part of the solution we want. Could you add that to #37 @bqleine? Then we probably could use #37 instead of reworking #32. Thanks!