Skip to content
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

profiles: telegram: allow opening links (xdg-open) #4783

Merged
merged 1 commit into from
Dec 19, 2021

Conversation

YorkZ
Copy link
Contributor

@YorkZ YorkZ commented Dec 19, 2021

Without this fix, clicking hyperlinks in Telegram had no effect.

@SkewedZeppelin
Copy link
Collaborator

This doesn't seem right?

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

@SkewedZeppelin What did you mean?

@glitsj16
Copy link
Collaborator

Without this fix, clicking hyperlinks in Telegram had no effect.

The telegram profile uses private-bin telegram,Telegram,telegram-desktop. Allowing ${PATH}/bash and ${PATH}/sh doesn't change that. I'm wondering how this change can fix anything if these aren't added to private-bin too. Maybe that's what @SkewedZeppelin's hinting at.

BTW, we have allow-bin-sh.inc to override 'include disable-shell.inc' in a more maintainable way compared to adding seperate noblacklist paths.

To get to the bottom of this it might be helpful to open an issue ticket where we can try to reproduce and discuss things before jumping in with a PR that - for now - offers little context. Just my 2 cents, nothing personal.

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

@glitsj16 Open an issue would be a good idea, and I can do that. Meanwhile, I did have the issue that I wasn't able to open hyperlinks in Telegram because when Telegram tried to open a link, it invokes xdg-open which wouldn't work if bash and sh have been blacklisted. After noblacklist bash and sh the problem went away.
If you wondered about the issue and the proposed fix, can you try it out on your system (if you use Telegram)?

@glitsj16
Copy link
Collaborator

I don't use telegram. But I can definately see how it would rely on xdg-open, like so many apps do, to open URI's in your user's preferred application. And the latter indeed needs bash/sh, there's no confusion about that part. What I cannot (yet) understand is how unblocking bash and sh can help if they are not also added to private-bin. You should need xdg-open there too.

Usually I don't mind installing an app I will never use myself for firejail testing purposes, done so many times. As far as I know Telegram requires a phone number for account creation. So to test its hyperlink functionality under firejail I'd have to supply something I personally regard as very private indeed and that's where I draw the testing line. Maybe other collaborators use it, I don't know. Would be helpful if they chimed in :-)

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

As far as I know Telegram requires a phone number for account creation

Fair enough. I've opened the issue #4784 as per your suggestion.

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

@glitsj16

The telegram profile uses private-bin telegram,Telegram,telegram-desktop. Allowing ${PATH}/bash and ${PATH}/sh doesn't change that. I'm wondering how this change can fix anything if these aren't added to private-bin too.

You are actually right. The reason this PR worked for me was because in my version (installed with Arch Linux pacman), the private-bin command has been commented out:

#private-bin telegram,Telegram,telegram-desktop

But in the git version, the above line was uncommented:

private-bin telegram,Telegram,telegram-desktop

When I uncommented this command, the PR didn't fix the issue.

So I added xdg-open to private-bin:

private-bin telegram,Telegram,telegram-desktop,xdg-open

But the hyperlink still could not be opened, and I got the following message as I clicked the link:

Launch failed (/usr/local/bin/xdg-open https://images.app.goo.gl/s6pqdz6AApaaNWTF6)

I don't know why it tried to execute xdg-open in /usr/local/bin which didn't exist. Any idea?

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

Adding bash and sh to private-bin worked. I've updated the commit.

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

Can anyone explain to me why "Profile Checks" failed? Thank you.

@glitsj16
Copy link
Collaborator

Crossing communications... I noticed the differences between the git profile and the one from 0.9.66 too. Bottomline, change the private-bin line to:
private-bin bash,sh,telegram,Telegram,telegram-desktop,xdg-open

I don't know why it tried to execute xdg-open in /usr/local/bin which didn't exist. Any idea?

One thing I noticed that might explain this is this warning on your issue debug log:

[...]
Warning: an existing sandbox was detected. /usr/bin/telegram-desktop will run without any additional sandboxing features
[...]

That warning usually is an indication that there's been a mix-up in the paths when starting a sandbox. If you use firecfg, that places a telegram symlink under /usr/local/bin for 'ease-of-use' or what's refered to as desktop integration in man firejail. During your testing you might have used $ firejail --debug telegram, which boils down to trying to sandbox telegram twice (hence the warning). Try adding the full path to the telegram executable: $ firejail --debug /usr/bin/telegram and notice the difference.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor changes needed befor we can commit this.

@@ -8,6 +8,10 @@ include globals.local
noblacklist ${HOME}/.TelegramDesktop
noblacklist ${HOME}/.local/share/TelegramDesktop

# Allow opening hyperlinks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: use our inc file specifically designed for situations like this (instead of the two noblacklists)

include allow-bin-sh.inc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: use our inc file specifically designed for situations like this (instead of the two noblacklists)

include allow-bin-sh.inc

Nice suggestion, thank you @glitsj16.

@@ -41,7 +45,7 @@ seccomp.block-secondary
shell none

disable-mnt
private-bin telegram,Telegram,telegram-desktop
private-bin bash,telegram,Telegram,telegram-desktop,sh
Copy link
Collaborator

@glitsj16 glitsj16 Dec 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add xdg-open too and respect ordering (the latter might explain the Profile Checks failure):
private-bin bash,sh,telegram,Telegram,telegram-desktop,xdg-open

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

Thank you very much for the help @glitsj16, I'm very new to firejail.

You are right about the reason of the warning:

Warning: an existing sandbox was detected. /usr/bin/telegram-desktop will run without any additional sandboxing features

which, as you pointed out, was because I had a symbolic link in /usr/local/bin and I tested according to the suggestion of the issue template.

add xdg-open too

Do we still need to add xdg-open to private-bin list? I asked because it worked here without having to add it.

and respect ordering:
private-bin bash,sh,telegram,Telegram,telegram-desktop,xdg-open

This order worked, but I don't understand the reason behind this order, could you explain?

@glitsj16
Copy link
Collaborator

glitsj16 commented Dec 19, 2021

Thank you very much for the help @glitsj16, I'm very new to firejail.

You're welcome. We've all been new to firejail and it's a complex application (code-wise). So keep asking for help whenever the need arises, that's very wise in regards to a security/privacy related app IMO. Also consult the wiki pages and issue tracker etc. and you'll get the hang of it soon enough.

This order worked, but I don't understand the reason behind this order, could you explain?

There's no strictly technical reason for ordering option values alphabetically. Just one of these social conventions to help avoiding profile breakage (typo's, doubled entrees, ...) and make things more manageable I guess.

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

There's no strictly technical reason for ordering option values alphabetically

Have no idea why I thought s was after t in alphabetical ordering :-). Thank you.

@glitsj16
Copy link
Collaborator

Have no idea why I thought s was after t in alphabetical ordering :-). Thank you.

Heh, happens to all of us. That's exactly the reason for having a CI check on stuff like this :-)

@glitsj16
Copy link
Collaborator

Do we still need to add xdg-open to private-bin list? I asked because it worked here without having to add it.

Yep, we need it there too. Run a search in your editor of choice on xdg-open in /etc/firejail/. You'll notice we add it in lots of profiles to guarantee hyperlink functionality.

@@ -41,7 +44,7 @@ seccomp.block-secondary
shell none

disable-mnt
private-bin telegram,Telegram,telegram-desktop
private-bin bash,sh,telegram,Telegram,telegram-desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private-bin bash,sh,telegram,Telegram,telegram-desktop,xdg-open

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private-bin bash,sh,telegram,Telegram,telegram-desktop,xdg-open

Done. Thank you.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one xdg-open in private-bin left before we can merge. Thanks!

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

Just the one xdg-open in private-bin left before we can merge. Thanks!

Done. Thank you.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@glitsj16 glitsj16 merged commit f25a9f1 into netblue30:master Dec 19, 2021
@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

@glitsj16 BTW, where do I ask questions?

glitsj16 added a commit that referenced this pull request Dec 19, 2021
@glitsj16
Copy link
Collaborator

@YorkZ You can ask questions via the discussions section here on GH. Not a perfectly labelled section but well, that's out of our control :-)

@YorkZ
Copy link
Contributor Author

YorkZ commented Dec 19, 2021

@YorkZ You can ask questions via the discussions section here on GH. Not a perfectly labelled section but well, that's out of our control :-)

Very nice, thank you.

@@ -41,7 +44,7 @@ seccomp.block-secondary
shell none

disable-mnt
private-bin telegram,Telegram,telegram-desktop
private-bin bash,sh,telegram,Telegram,telegram-desktop,xdg-open
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why bash?

xdg-open is a shell-script, consider sed,xdg-mime,which,mimeopen,grep,egrep,printf,cut,uname,dbus-send,xprop,dirname,cat,…, exo-open,gio,gvfs-open,mate-open,enlightenment_open,gnome-open,dde-open,kde-open,kde-open*,kfmclient,cygstart,kde-config,gnome-default-applications-properties,… or just drop private-bin.

Copy link
Contributor Author

@YorkZ YorkZ Dec 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why bash?

I believe enabling bash is necessary because in Arch Linux, /usr/bin/sh has been symlink'ed to /usr/bin/bash (/usr/bin/sh is provided by package bash). Some customized systems also use bash as sh. I personally frequently use bash as sh in production systems too because bash is so much more featureful than sh. In fact, bash has a sh compatible mode which is probably why I've never had any issue when replacing sh with bash. Finally, in this case, I tested only enabling sh, and confirmed that it didn't work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe enabling bash is necessary because in Arch Linux, /usr/bin/sh has been symlink'ed to /usr/bin/bash

Do not believe, test 😉.

private-bin follows symlinks IIRC and even if it don't there are other shells commonly used as /bin/sh (namely dash by Debian and Ubuntu(?)).

Finally, in this case, I tested only enabling sh, and confirmed that it didn't work.

(Fedora Linux 35; sh->bash):

$ firejail --noprofile --private-bin=sh,ls ls -l /usr/bin
bash
ls
sh -> /usr/bin/bash

I guess you had tested with only sh in private-bin and only noblacklist ${PATH}/sh. This does not work blacklist ${PATH}/bash will blacklist the binary used by /usr/bin/sh. FWIW, blacklist follows symlinks too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xdg-open is a shell-script, consider …

On my system for xdg-open to work at all: sh,xdg-open,grep,egrep,<A installed well known browser, e.g. firefox>.
On my system for xdg-open to work correct: sh,xdg-open,grep,egrep,xdg-mime,sed,tr,awk,cut,head,basename,which,readlink,<My browser, e.g. firefox>
For the firefox start script: ...

or just drop private-bin.

yeah, I think this is the way to go.

Copy link
Contributor Author

@YorkZ YorkZ Dec 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you had tested with only sh in private-bin and only noblacklist ${PATH}/sh

That's correct, I think I tested with only sh in private-bin and I only noblacklist sh.

there are other shells commonly used as /bin/sh (namely dash by Debian and Ubuntu(?))

I think dash is indeed an implementation of POSIX sh which is actually sh.

Copy link
Collaborator

@rusty-snake rusty-snake Dec 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this is because firejail hardcodes bash as the default shell.

It don't, read the issues title in your comment:

  • Default shell is guessed from $SHELL, despite manpage specifying /bin/bash

On Artix it fails, but I'm not sure why:

If I get you right (sh->dash; getent passwd $USER | cut -d: -f7: bash), you try to start a program (bash) which isn't inside the sandbox.

IMHO --shell=none should be the default (if you specify a program).

So in my case it does require private-bin bash, even though /bin/sh is
/bin/dash.

Because you still run bash inside the sandbox and it isn't present (as a side-effect).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rusty-snake on Dec 19:

This just lists the files;

@kmk3 it clearly shows that the file /bin/sh points to is
copied/bind-mounted (what does it do?) if you only list sh but not bash
(assuming /bin/sh points to somewhere in {/usr,}/bin).

I don't understand what you mean. A symlink points to a (text) path; it
doesn't actually point to an inode. For example, if /bin/sh points to just
"dash" and you copy the symlink with to /tmp, the copy will not work:

$ readlink /bin/sh
dash
$ /bin/sh -c 'echo yes'
yes
$ cd /tmp
$ cp -P /bin/sh .
$ readlink ./sh
dash
$ ./sh -c 'echo yes'
bash: ./sh: No such file or directory
$ echo 'echo hello world' >dash
$ chmod +x dash
$ ./dash
hello world
$ ./sh
hello world

Do you mean that sh points to just "bash" outside of firejail but inside of it
it points to "/usr/bin/bash"? That might mean that firejail hardcodes its own
/bin/sh symlink inside the sandbox, but whether /bin/bash is bind-mounted or
not is unclear from your example.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I want to tell is that private-bin=sh will copy/bind-mount sh and the program it points to (e.g. bash or dash). No matter if you list bash/dash or not.

I don't care about /bin/sh pointing to somewhere else (e.g. /mnt/extra-program/mysh).

Copy link
Contributor Author

@YorkZ YorkZ Dec 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried removing bash from private-bin, and it worked (the link was opened from Telegram). @kmk3 do you still have problem if removing bash from private-bin?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Continued on #4790)

@glitsj16
Copy link
Collaborator

Why bash?

Good question and fair points. The private-bin enablement after 0.9.66 caused some confusion as you can see (both here and in the issue). I can only find this in the history of telegram.profile. No info on why private-bin got enabled exactly, but I assume it was just good hardening practice. @netblue30 Any thoughts?

@kmk3 kmk3 linked an issue Feb 5, 2022 that may be closed by this pull request
7 tasks
@kmk3 kmk3 changed the title Allow telegram to open hyperlinks profiles: telegram: allow opening links (xdg-open) Sep 5, 2024
@kmk3 kmk3 added the sandbox-ipc Opening links and talking to programs outside of the sandbox (see #6462) label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sandbox-ipc Opening links and talking to programs outside of the sandbox (see #6462)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

telegram: cannot open links in browser
5 participants