-
Notifications
You must be signed in to change notification settings - Fork 569
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
disable-shell.inc breaks AppImages #3530
Comments
If i run the appimage like so: Possibly related to #2690 ? |
Where is the AI stored? To sum-up:
|
Do you mean where do i run it from? just from $HOME
Yes exactly. Not sure if this is related to the issue i mentioned. |
The last may work with |
firejail '--ignore=noexec ${HOME}' --profile=keepassxc ./KeePassXC-2.5.4-x86_64.AppImage
|
Comment |
@kortewegdevries thank you, it works. What is the significance of |
Also if we disable it, what are the security risks? |
My question: Why does it work?!! There is no shell in its private-bin! |
@rusty-snake so is this a bug? If i keep it disabled will that mean that the sandbox is less secure? |
The reason is that firejail/src/firejail/sandbox.c Lines 910 to 911 in fb145c3
firejail/src/firejail/sandbox.c Lines 930 to 931 in fb145c3
That's probably because, the way it works right now, a shell is needed to run the AppImage. |
Maybe it would be good if Firejail could also print a warning. |
Can we do something like this? if (arg_appimage && strcmp(fname, "disable-shell.inc"))
return; |
Then is there a point in adding |
|
@svc88 Security does not degrade with regards to 0.9.62. As a matter of fact a shell is needed currently, so there is no degree of freedom anyway. |
I tried my Idea and it is not working (as I expected). skip-disable-shell-if-appimage.patchiff --git a/src/firejail/profile.c b/src/firejail/profile.c
index a8722282..8d9a8d5d 100644
--- a/src/firejail/profile.c
+++ b/src/firejail/profile.c
@@ -1607,6 +1607,11 @@ static int include_level = 0;
void profile_read(const char *fname) {
EUID_ASSERT();
+ if (arg_appimage && strcmp(fname, "disable-shell.inc") == 0) {
+ fprintf(stderr, "Skipping disable-shell because of --appimage\n");
+ return;
+ }
+
// exit program if maximum include level was reached
if (include_level > MAX_INCLUDE_LEVEL) {
fprintf(stderr, "Error: maximum profile include level was reached\n"); @netblue30 @smitsohu This fact make me thinking about
|
Add an option --allow-shell (arg_allow_shell) and force it whenever arg_appimage is set and skip like 1633-1641? |
|
|
patch and Obvious patching is no solution for the majority, but it would be nice to have this patch in (after someone provided feedback). |
👍
Where do you want to add |
Turns out it's not so straightforward with |
Or we do go through the conditionals at the end, after all other command line and profile options, and offer negated conditionals in addition to what we have now, something like This would sacrifice some flexibility, but make a cleaner interface. There could be a Honestly I'm somewhat at a loss what to do here. |
For now we could enforce that |
Fixed! We were doing something similar for --allow-debuggers and disable-devel.inc. Give it a try, thanks. |
Bug and expected behavior
When i upgraded to 0.9.63 from 0.9.62, i started having issues with keepassxc appimage.
The appimage didnt open up keepassxc, instead i saw an error in the log saying:
No profile or disabling firejail
firejail --noprofile PROGRAM
in a shell?It runs
Environment
Additional context
This didnt happen with 0.9.62
The text was updated successfully, but these errors were encountered: