-
Notifications
You must be signed in to change notification settings - Fork 18
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
Find fusermount using execvp #32
Conversation
Build for testing: Hi @Samueru-sama, can you confirm that this runtime fixes the issue at ivan-hc/Chrome-appimage#3? Ping @TheAssassin, can you please do a review? |
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 No distro will ever get rid of the |
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. |
If |
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?
|
Thanks @Bqlein . So to my understanding we should:
Does this sound about right? |
No, that is not how setuid works.
Yes |
@TheAssassin wdyt? |
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? |
+ if (fileExists(binaryPath)) { | ||
+ return binaryPath; | ||
+ } | ||
+char* findBinaryOnPath(const char* binaryName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we agreed on using execvp
instead of manually tracing the PATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are spot on. We should use that instead. As I summarized in #32 (comment). If you agree with the points there, could you please help me implement them? That would really be a great contribution. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can provide the implementation, but it'll take a while until I'll get to it.
If we choose to utilize execvp, those points need to be considered.
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 |
That does not really answer my question, though. Do you remember those concerns? I'd like to review them myself. |
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. |
Converted this PR to a draft because the code in this PR is currently wrong. We should do what is described in #32 (comment) instead. Coding help is welcome. |
https://www.reddit.com/r/linux/comments/1d60t1r/comment/lh1qxf2/
Finding it dynamically using execvp should (hopefully) fix this. |
Or you could just hardcode the path to fusermount to Because when fusermount is on However because it is hardcoded to (I'm the person of the reddit comment btw). |
Note to self: mount.c.zip (Editing diff files is not fun...) |
Yes, you can actually see it in the fuse3 package in debian it contains a Arch linux does not have it, but that is because archlinux has separate In fact before there was this issue, that the appimages with the static runtime wouldn't work without the Also fun fact, on archlinux despite being a rolling release distro,
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. |
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. |
Wouldn’t that mean a user in the future could just install fuse3 to run his appimage if he only has fuse4? |
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 |
Probably. But if distributions do what they did last time, they will drop fuse3 from the ISOs as soon as they have fuse4 packaged. |
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 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 |
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 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. |
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 I'm a bit sad that unmounting requires privileged syscalls which in turn requires a |
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 |
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 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. |
The upstream libfuse codebase simply uses autoconf / meson
with fuse3, So on Gentoo, you get:
|
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 |
@mgord9518 I was talking about that here but probono doesn't like the idea. |
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 |
@TheAssassin Just mounting. You can unmount a fuse FS using the As far as I understand, using SIGINT on the fuse process also gracefully unmounts |
Even without libnotify, I suppose a few commands like |
Been doing some digging, came across this very interesting article. Gonna mess around and see if I can get anywhere without fusermount |
Sorry for the spam, but at least on first look this seems very promising: ┌┤ ~/Git/squashfuse-zig │ ok │
└─ nix-shell --pure -p util-linux
[nix-shell:~/Git/squashfuse-zig]$ unshare -pfr --user --mount --kill-child bash
[root@framework:~/Git/squashfuse-zig]# ./zig-out/bin/squashfuse test/tree_xz.sqfs mnt/
[root@framework:~/Git/squashfuse-zig]# ls mnt
1
2
block_device
...
[root@framework:~/Git/squashfuse-zig]# which fusermount fusermount3
which: no fusermount in (/home/mgord9518/.local/bin:/home...
which: no fusermount3 in (/home/mgord9518/.local/bin:/home...
[root@framework:~/Git/squashfuse-zig]# umount mnt I know very little about namespaces, but assuming I'm not somehow incorrectly testing, this might be exactly what we need |
@mgord9518 Is your idea related to this? I have bad news if that's the case. |
@Samueru-sama Just about some distros disabling them or something else? |
Yes ubuntu and most forks have it disabled now. and that caused a bunch of bug reports on all electron based applications as result. I think only solus and mint so far have disabled the ubuntu restriction. |
@Samueru-sama Yeah, that does throw a wrench in this a bit. Even still, it's probably worth implementing even if most Ubuntu-based distros won't benefit by default |
I'm ok with the method, however if it is implemented I think there has to be something that would detect that the restriction is in place and ask the user with pkexec or similar to disable it. |
I was more thinking having this as a fallback if |
VERY interesting @Mgord. 💯 I think you are up to something. That suid helper binary always bothered be to begin with. Does anyone know how to actually implement this, in code? Any help appreciated 👍 --> let's continue this topic at |
Point in case why we cannot assume the binaries to be in certain directories - in Guix, they put Hence we need to search on |
Closes #31
NOTE: The code in this PR is currently wrong. We should do what is described in #32 (comment) instead. Coding help is welcome.
UPDATE: Consider merging
instead.