Commit id '740e4d70' altered the logic to fetch the sysconf values and
added a new virConfGetValueStringList which returns -1 on failure, 0 if
missing, and 1 if the value was present.
However, the caller only checked !shargv which caught Coverity's attention
since the following VIR_ALLOC_N(*shargv, 2) would be a NULL ptr deref
Signed-off-by: John Ferlan <jferlan@redhat.com>
Prior to joining the namespaces of the container, move the
process into the containers' cgroups, so that the shell that
is subsequently launched is under the container resource
constraints.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Currently the shell must be looked up from the config setting in
/etc/libvirt/virt-login-shell.conf. This is inflexible if there
are containers where different users need different shells. Add
add a new 'auto-shell' config parameter which instructs us to
query the containers' /etc/passwd for the shell to be exec'd.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virt-login-shell environment will be initialized with
an arbitrary number of environment variables determined
by the SSH daemon and PAM configuration. Most of these are
not relevant inside the container, and at best they are
noise and at worst they'll break apps. For example if
XDG_RUNTIME_DIR is leaked to the container, it'll break
any apps using it, since the directory it points to is
only visible to the host OS filesystem, not the container
FS.
Use clearenv() to blank out everything and then set known
good values for PATH, SHELL, USER, LOGNAME HOME and TERM.
Everything else is left up to the login shell to initialize.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virDomainFree / virConnectClose methods will reset the
last error handle, so we must save the error during cleanup
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Currently the shell config file parameter must be a list
giving the shell path and args. Allow it to be a plain
string argument as well.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Currently we request a login shell by passing the -l argument
to the shell. This is either hardcoded, or required to be
specified by the user in the virt-login-shell.conf file.
The standard way for login programs to request a shell run
as a login shell is to modify the argv passed to execve()
so that argv[0] contains the relative shell filename
prefixed with a zero. eg instead of doing
const char **shellargs = ["/bin/bash", "-l", NULL];
execve(shellargs[0], shellargs, env);
We should be doing
const char **shellargs = ["-bash", NULL];
execve("/bin/bash", shellargs, env);
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virt-login-shell program is supposed to look like a
regular shell to clients. Login services like sshd
expect the shell to accept a '-c cmdstring' argument to
specify a command to launch instead of presenting an
interactive prompt.
We can implement this by simply passing the '-c cmdstring'
data straight through to the real shell we use. This does
not open any security holes, since the command is not run
until we're inside the container namespaces. This allows
scp to work for users with virt-login-shell.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The manpage for sysconf() suggest including unistd.h as the
function is declared there. Even though we are not hitting any
compile issues currently, let's include the correct header file
instead of relying on some hidden include chain.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Coverity complained that checking the return of virDomainCreate()
was not consistent amongst the callers - so added the return check
to the objecteventtest.c and adjust the virt-login-shell to compare
< 0 rather than just non zero for the failure condition.
Coverity spotted that 'nfdlist' (ssize_t) could be -1, but that we
were using 'i' (size_t) to iterate over the list at cleanup, with
crashing results because it promotes to a really big unsigned number.
* tools/virt-login-shell.c (main): Avoid treating -1 as unsigned.
Signed-off-by: Eric Blake <eblake@redhat.com>
virt-login-shell was exiting with status 0, regardless of what the
wrapped shell returned. This is unkind to users; we should behave
more like env(1), nice(1), su(1), and other wrapper programs, by
preserving the invoked application's status (which includes the
distinction between death due to signal vs. normal death).
* tools/virt-login-shell.c (main): Pass through child exit status.
* tools/virt-login-shell.pod: Document exit status.
Signed-off-by: Eric Blake <eblake@redhat.com>
Note that 'virsh lxc-enter-namespace' must double-fork, for two
reasons: some namespaces can only be done from a single thread,
while virsh is multithreaded; and because virsh can be run in
batch mode where we must not corrupt the namespace of that
execution upon return from the subsidiary command.
When virt-login-shell was first written, it blindly copied from
'virsh lxc-enter-namespace', including the double-fork. But
neither of the reasons for double forking apply to
virt-login-shell (we are single-threaded, and we have nothing to
do after the child completes that would require us to preserve a
namespace), so we can simplify life by using a single fork.
In turn, this will make it easier for a future patch to pass the
child's exit status on to the invoking shell.
In flattening to a single fork, note that closing the fds must
be done after fork, because the parent process still needs to
use fds to control the virConnectPtr; meanwhile, chdir can be
done prior to forking (in fact, it's easier to report errors
on anything attempted before forking).
* tools/virt-login-shell.c (main): Single rather than double fork.
(virLoginShellFini): Delete, by inlining actions instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
The old semantics of virFork() violates the priciple of good
usability: it requires the caller to check the pid argument
after use, *even when virFork returned -1*, in order to properly
abort a child process that failed setup done immediately after
fork() - that is, the caller must call _exit() in the child.
While uses in virfile.c did this correctly, uses in 'virsh
lxc-enter-namespace' and 'virt-login-shell' would happily return
from the calling function in both the child and the parent,
leading to very confusing results. [Thankfully, I found the
problem by inspection, and can't actually trigger the double
return on error without an LD_PRELOAD library.]
It is much better if the semantics of virFork are impossible
to abuse. Looking at virFork(), the parent could only ever
return -1 with a non-negative pid if it misused pthread_sigmask,
but this never happens. Up until this patch series, the child
could return -1 with non-negative pid if it fails to set up
signals correctly, but we recently fixed that to make the child
call _exit() at that point instead of forcing the caller to do
it. Thus, the return value and contents of the pid argument are
now redundant (a -1 return now happens only for failure to fork,
a child 0 return only happens for a successful 0 pid, and a
parent 0 return only happens for a successful non-zero pid),
so we might as well return the pid directly rather than an
integer of whether it succeeded or failed; this is also good
from the interface design perspective as users are already
familiar with fork() semantics.
One last change in this patch: before returning the pid directly,
I found cases where using virProcessWait unconditionally on a
cleanup path of a virFork's -1 pid return would be nicer if there
were a way to avoid it overwriting an earlier message. While
such paths are a bit harder to come by with my change to a direct
pid return, I decided to keep the virProcessWait change in this
patch.
* src/util/vircommand.h (virFork): Change signature.
* src/util/vircommand.c (virFork): Guarantee that child will only
return on success, to simplify callers. Return pid rather than
status, now that the situations are always the same.
(virExec): Adjust caller, also avoid open-coding process death.
* src/util/virprocess.c (virProcessWait): Tweak semantics when pid
is -1.
(virProcessRunInMountNamespace): Adjust caller.
* src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Likewise.
* tools/virt-login-shell.c (main): Likewise.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
* tests/commandtest.c (test23): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Right now, a caller waiting for a child process either requires
the child to have status 0, or must use WIFEXITED() and friends
itself. But in many cases, we want the middle ground of treating
fatal signals as an error, and directly accessing the normal exit
value without having to use WEXITSTATUS(), in order to easily
detect an expected non-zero exit status. This adds the middle
ground to the low-level virProcessWait; the next patch will add
it to virCommand.
* src/util/virprocess.h (virProcessWait): Alter signature.
* src/util/virprocess.c (virProcessWait): Add parameter.
(virProcessRunInMountNamespace): Adjust caller.
* src/util/vircommand.c (virCommandWait): Likewise.
* src/util/virfile.c (virFileAccessibleAs): Likewise.
* src/lxc/lxc_container.c (lxcContainerHasReboot)
(lxcContainerAvailable): Likewise.
* daemon/libvirtd.c (daemonForkIntoBackground): Likewise.
* tools/virt-login-shell.c (main): Likewise.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
* tests/testutils.c (virtTestCaptureProgramOutput): Likewise.
* tests/commandtest.c (test23): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Our fixes for CVE-2013-4400 were so effective at "fixing" bugs
in virt-login-shell that we ended up fixing it into a useless
do-nothing program.
Commit 3e2f27e1 picked the name LIBVIRT_SETUID_RPC_CLIENT for
the witness macro when we are doing secure compilation. But
commit 9cd6a57d checked whether the name IN_VIRT_LOGIN_SHELL,
from an earlier version of the patch series, was defined; with
the net result that virt-login-shell invariably detected that
it was setuid and failed virInitialize.
Commit b7fcc799 closed all fds larger than stderr, but in the
wrong place. Looking at the larger context, we mistakenly did
the close in between obtaining the set of namespace fds, then
actually using those fds to switch namespace, which means that
virt-login-shell will ALWAYS fail.
This is the minimal patch to fix the regressions, although
further patches are also worth having to clean up poor
semantics of the resulting program (for example, it is rude to
not pass on the exit status of the wrapped program back to the
invoking shell).
* tools/virt-login-shell.c (main): Don't close fds until after
namespace swap.
* src/libvirt.c (virGlobalInit): Use correct macro.
Signed-off-by: Eric Blake <eblake@redhat.com>
I noticed a few odd things in 'virt-login-shell --help' output.
* tools/virt-login-shell.c (usage): At most one option accepted,
drop trailing colon.
Signed-off-by: Eric Blake <eblake@redhat.com>
The virt-login-shell binary shouldn't need to execute programs
relying on $PATH, but just in case set a fixed $PATH value
of /bin:/usr/bin
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
We don't want to inherit any FDs in the new namespace
except for the stdio FDs. Explicitly close them all,
just in case some do not have the close-on-exec flag
set.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Address a number of code, style and docs issues identified
in review of virt-login-shell after it was merged.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virLoginShellAllowedUser method must not free the 'groups'
parameter it is given, as that is owned by the caller.
The virLoginShellAllowedUser method should be checking
'!*ptr' (ie empty string) rather than '!ptr' (NULL string)
since the latter cannot be true.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
virt-login-shell.c was failing to compile with
CC virt_login_shell-virt-login-shell.o
virt-login-shell.c: In function 'main':
virt-login-shell.c:205:5: error: implicit declaration of function 'setlocale' [-Werror=implicit-function-declaration]
virt-login-shell.c:205:5: error: nested extern declaration of 'setlocale' [-Werror=nested-externs]
virt-login-shell.c:205:20: error: 'LC_ALL' undeclared (first use in this function)
Add a virt-login-shell binary that can be set as a user's
shell, such that when they login, it causes them to enter
the LXC container with a name matching their user name.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
