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

Should an exec wrapper be used? #396

Open
darealshinji opened this issue May 13, 2017 · 23 comments
Open

Should an exec wrapper be used? #396

darealshinji opened this issue May 13, 2017 · 23 comments

Comments

@darealshinji
Copy link
Contributor

So KDE AppImages are using these exec wrappers which are preloaded as libraries, right?
It took me a while to find the source code of it:

git clone git://anongit.kde.org/scratch/brauch/appimage-exec-wrapper

There's an interesting comment in the source code about what this is actually meant for:

Whenever your application invokes a child process through execv() or execve(),
this wrapper will intercept the call and see if the child process lies
outside of the bundled appdir. If it does, the wrapper will attempt to undo
any changes done to environment variables before launching the process,
since you probably did not intend to launch it with e.g. the LD_LIBRARY_PATH
you previously set for your application.

This sounds reasonable to me. Do you think this is something worth to implement your upstream binaries?

@TheAssassin
Copy link
Member

It sounds really, really useful considering application security. I can imagine scenarios where a malicious application tries to call system binaries (e.g. accidentally with a setuid bit set) that could do harmful things by preloading with non-system libraries (talking about accidentally broken Appimages here, e.g. such with outdated libraries).

But I strongly recommend to not advertise this as a security feature! We must make sure that desktop environments like KDE won't stop developing features like that independently!

This is caused by the fact that AppImageKit is just the reference implementation of the AppImage standard, and any AppImage vendor might fork it and remove this or even use something completely different. If KDE etc. provide an implementation though, they're independent and will work anyway.

Though, if an attacker really wanted to, they could easily break this feature I guess.

But sure, providing it as an additional security feature, why not implement it upstream, too? If you implement it, I'll support your feature proposal.

@probonopd
Copy link
Member

With linuxeployqt I am trying to remove the need for LD_LIBRARY_PATH environment variables entirely, which to me feels like a cleaner solutions. But for more complex applications like KDevelop additional environment variables need to be set, and for this the appimage-exec-wrapper is very useful (I was in the room when Sven invented it).

@darealshinji what exactly do you mean by "implement your upstream binaries"? Make AppRun.c use it by default?

@TheAssassin
Copy link
Member

He asks if it's worth porting this to AppImageKit, too.

@probonopd
Copy link
Member

He asks if it's worth porting this to AppImageKit, too.

Sure, but how? I mean, what runs inside the AppImage is a matter of the application, right? The AppImage is, after all, just a self-mounting filesystem...

@TheAssassin
Copy link
Member

What do you mean, how? Just do it the way KDE do it: I guess you just have to preload the same libraries.

@probonopd
Copy link
Member

Yes, but that's something the person who makes the AppImage does. I don't know how to "put that into AppImageKit"...

@TheAssassin
Copy link
Member

So you say if @darealshinji came up with a concept you would consider it? @darealshinji: please elaborate.

@probonopd
Copy link
Member

Sure!

@darealshinji
Copy link
Contributor Author

The easiest solution I could think of would be to check if exec_wrapper.so is present, and if it is preload it before starting the app.

@aferrero2707
Copy link

I am very much interested in this feature, as this is currently one of the limitations in the GIMP AppImage I am providing: some GIMP plug-ins, like file-darktable, invoke external executables, and those executables run into library conflicts in some systems. Libraries that are not bundled because not required by GIMP get mixed with bundled ones, with potential symbol conflicts.

The best would be to store somehow the environment at the beginning of the AppImage execution, and use this environment to run external programs.
In the specific case of GIMP, the external programs are executed via g_spawn_async, so I am not sure if the appimage-exec-wrapper solution would work... any idea?

@TheAssassin
Copy link
Member

In your specific case, you can just add the library @darealshinji mentioned to the AppImage, and use a little shell script to launch gimp with LD_PRELOAD=myshinylibrary.so path/to/gimp. You could also just write your own little C wrapper that redefines g_spawn_async and fixes the environment.

More information on LD_PRELOAD can be found here: https://stackoverflow.com/questions/426230/what-is-the-ld-preload-trick

Beware that if you copy paste code from KDE or GIMP project, the code is copyrighted, you need to check their license and probably include a copy of the license and a copyright notice. (Source code should be enough, but this is not legal advice, consult the license about how to do it properly.)

@aferrero2707
Copy link

@darealshinji I have started to look into the appimage-exec-wrapper code, but I am not sure to fully understand the logic... In the preamble one can find this explanation:

For each environment variable you want restored, where {VAR} is the name of the environment
variable (e.g. "PATH"):
  $APPIMAGE_ORIGINAL_{VAR} -- original value of the environment variable
  $APPIMAGE_STARTUP_{VAR} -- value of the variable when you were starting up
                             your application

I guess that ORIGINAL here means outside of the appimage environment and STARTUP means inside the appimage environment, right? But then, which piece of code should take care of setting those variables? Is it AppRun.c or something else? Because those variables need to be set before the environment is modified, as far as I understand...

@probonopd
Copy link
Member

Please have a look at the AppRun script used at https://www.kdevelop.org/download as this is what the appimage-exec-wrapper code was originally developed for.

@aferrero2707
Copy link

Does anyone have an idea if/how I can include a modified version of appimage-exec-wrapper into my GIMP AppImage builder, and do not break copyright?

The exec wrapper code is released under the "GNU Library General Public License".

I need to modify it in order to also include g_spawn_async in the list of supported functions...

@darealshinji
Copy link
Contributor Author

@aferrero2707 Adding a copy of it's source to the AppImage should be fine guess.

@probonopd
Copy link
Member

@aferrero2707 I could imagine that its author Sven Brauch would also discuss other licensing options if required; best contact him directly in case you have special needs or questions.

@TheAssassin
Copy link
Member

@aferrero2707 please be aware that @darealshinji did NOT give you legal advice here (at least I think so).

For legal advice like this, you should consult a lawyer if in doubt. The Free Software Foundation are likely willing to help you with that kind of problem, too. In fact they published an FAQ for people in your situation: https://www.gnu.org/licenses/gpl-faq.html

My opinion (disclaimer: this is not legal advice, and there is no warranty, you can not make me liable for any legal consequences): As you would modify the application, you have to publish the sources (either by embedding them, or adding a link to it in the README or something like that, see the GPL FAQ linked to above). The distribution of the binary doesn't violate the GPL, and since you don't link to the program but create child processes, it won't violate the GPL either. This is why we may redistribute quite some GPL binaries in AppImages, same goes for Linux distributions etc.

See also: https://softwareengineering.stackexchange.com/questions/110380/call-gpl-software-from-non-gpl-software

@easyw
Copy link

easyw commented Aug 14, 2019

Hi @probono,
I'm using FreeCAD AppImage and calling a python code:

subprocess.call(["xdg-open", fnameDemo])

Vshould I use a different code?

@probonopd
Copy link
Member

probonopd commented Aug 14, 2019

del os.environ['LD_LIBRARY_PATH']
del os.environ['APPDIR']
del os.environ['APPIMAGE']
del os.environ['XDG_DATA_DIRS']
del os.environ['...'] # Whatever other environment variables you don't want to "inherit" from the AppImage
subprocess.call(["xdg-open", fnameDemo])

@easyw
Copy link

easyw commented Aug 15, 2019

hi @probonopd
Thx!
it works for me using:

import os, subprocess
del os.environ['LD_LIBRARY_PATH']
subprocess.call(["xdg-open", pdffilename])

Should I restore the os.environ['LD_LIBRARY_PATH'] value after the call?

@TheAssassin
Copy link
Member

What @probonopd posted is highly risky, as it alters the entire process's environment.

You can't simply store and then restore the value as FreeCAD likely uses threading. You're opening pandora's box there. What if some other thread needs access to the environment, but you changed something and it tries to read that?

Python 101: copy your environment, alter it with del or whatever method (although deleting it entirely might be harmful as well, e.g., if the user has set some values before running FreeCAD; selectively replacing any value in there pointing to the mounted AppImage is the right way), then use the copied env object to spawn your process.

I don't know what Python version you use, but call doesn't support using a custom environment. Either use run, which was added in 3.5, otherwise you have to use Popen.

@easyw
Copy link

easyw commented Aug 15, 2019

Thx @TheAssassin
I'm going to use Popen with my_env

@probonopd
Copy link
Member

What @probonopd posted is highly risky, as it alters the entire process's environment.

Oopsie!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants