On Tue, Oct 22, 2013 at 09:42:44PM +0100, Eric Blake wrote:
On 10/22/2013 04:59 PM, Jim Fehlig wrote:
> Michal pushed the other one that Daniel ACKed. One thing that concerned
> me, which I forgot to mention in the mail or commit message, was this
> comment above libvirt_setuid_rpc_client in src/Makefile.am
>
> # Since virt-login-shell will be setuid, we must do everything
> # we can to avoid linking to other libraries.
We can't eliminate every library, but we can assume that some very basic
libraries are capable of being safely used in setuid apps (selinux,
apparmor, and libc among that list).
Meanwhile, we do have proof that other libraries are not so friendly;
among them, Daniel analyzed using LD_PRELOAD that at least libnspr.so
(used by libcurl.so) has at least one unprotected getenv() within a
constructor that could be used merely by loading the shared library into
memory as a way to cause an overwrite of an unintended file in a setuid
app. So maybe the comment could be tweaked to say "we avoid linking to
all but very basic and well-audited libraries".
The goal of virt-login-shell was to avoid linking to every library
except those strictly required for its work. This boiled down to
libxml and libselinux. It makes sense that libselinux would be
replaced by libapparmour on relevant distros.
If you want to be doubly paranoid though, look at the libapparmour
sources and verify that there is no function that is marked with
__attribute__((constructor)). If there is such a function, then
it should be audited to make sure its code is safe, in particular
wrt environment variables.
Daniel
--
|:
http://berrange.com -o-
http://www.flickr.com/photos/dberrange/ :|
|:
http://libvirt.org -o-
http://virt-manager.org :|
|:
http://autobuild.org -o-
http://search.cpan.org/~danberr/ :|
|:
http://entangle-photo.org -o-
http://live.gnome.org/gtk-vnc :|