[Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Eduardo Otubo]

This option was requested by virt-test team so they can run tests with
Qemu and "-sandbox on" set without breaking whole test if host doesn't
have support for seccomp in kernel. It covers two possibilities:

 1) Host kernel support does not support seccomp, but user installed Qemu 
    package with sandbox support: Libseccomp will fail -> qemu will fail 
    nicely and won't stop execution.

 2) Host kernel has support but Qemu package wasn't built with sandbox
    feature. Qemu will fail nicely and won't stop execution.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 vl.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/vl.c b/vl.c
index b0399de..a0806dc 100644
--- a/vl.c
+++ b/vl.c
@@ -967,13 +967,11 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
 #ifdef CONFIG_SECCOMP
         if (seccomp_start() < 0) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                          "failed to install seccomp syscall filter in the kernel");
-            return -1;
+                          "failed to install seccomp syscall filter in the kernel, disabling it");
         }
 #else
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                      "sandboxing request but seccomp is not compiled into this build");
-        return -1;
+                      "sandboxing request but seccomp is not compiled into this build, disabling it");
 #endif
     }
 
@@ -3808,9 +3806,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0)) {
-        exit(1);
-    }
+    qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0);
 
 #ifndef _WIN32
     if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Daniel P. Berrange]

On Mon, Dec 09, 2013 at 03:20:52PM -0200, Eduardo Otubo wrote:
> This option was requested by virt-test team so they can run tests with
> Qemu and "-sandbox on" set without breaking whole test if host doesn't
> have support for seccomp in kernel. It covers two possibilities:
> 
>  1) Host kernel support does not support seccomp, but user installed Qemu 
>     package with sandbox support: Libseccomp will fail -> qemu will fail 
>     nicely and won't stop execution.
> 
>  2) Host kernel has support but Qemu package wasn't built with sandbox
>     feature. Qemu will fail nicely and won't stop execution.
> 
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>  vl.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index b0399de..a0806dc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -967,13 +967,11 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>  #ifdef CONFIG_SECCOMP
>          if (seccomp_start() < 0) {
>              qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                          "failed to install seccomp syscall filter in the kernel");
> -            return -1;
> +                          "failed to install seccomp syscall filter in the kernel, disabling it");
>          }
>  #else
>          qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                      "sandboxing request but seccomp is not compiled into this build");
> -        return -1;
> +                      "sandboxing request but seccomp is not compiled into this build, disabling it");
>  #endif
>      }
>  
> @@ -3808,9 +3806,7 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0)) {
> -        exit(1);
> -    }
> +    qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0);
>  
>  #ifndef _WIN32
>      if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {

This change is really dubious from a security POV. If the admin requested
sandboxing and the host or QEMU build cannot support it, then QEMU really
*must* exit.

IMHO the test suite should probe to see if sandbox is working or not, and
just not use the "-sandbox on" arg if the host doesn't support it.

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 :|

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Eduardo Otubo]

On 12/09/2013 03:33 PM, Daniel P. Berrange wrote:
> On Mon, Dec 09, 2013 at 03:20:52PM -0200, Eduardo Otubo wrote:
>> This option was requested by virt-test team so they can run tests with
>> Qemu and "-sandbox on" set without breaking whole test if host doesn't
>> have support for seccomp in kernel. It covers two possibilities:
>>
>>   1) Host kernel support does not support seccomp, but user installed Qemu
>>      package with sandbox support: Libseccomp will fail -> qemu will fail
>>      nicely and won't stop execution.
>>
>>   2) Host kernel has support but Qemu package wasn't built with sandbox
>>      feature. Qemu will fail nicely and won't stop execution.
>>
>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>> ---
>>   vl.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index b0399de..a0806dc 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -967,13 +967,11 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>   #ifdef CONFIG_SECCOMP
>>           if (seccomp_start() < 0) {
>>               qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> -                          "failed to install seccomp syscall filter in the kernel");
>> -            return -1;
>> +                          "failed to install seccomp syscall filter in the kernel, disabling it");
>>           }
>>   #else
>>           qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> -                      "sandboxing request but seccomp is not compiled into this build");
>> -        return -1;
>> +                      "sandboxing request but seccomp is not compiled into this build, disabling it");
>>   #endif
>>       }
>>
>> @@ -3808,9 +3806,7 @@ int main(int argc, char **argv, char **envp)
>>           exit(1);
>>       }
>>
>> -    if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0)) {
>> -        exit(1);
>> -    }
>> +    qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0);
>>
>>   #ifndef _WIN32
>>       if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {
>
> This change is really dubious from a security POV. If the admin requested
> sandboxing and the host or QEMU build cannot support it, then QEMU really
> *must* exit.

I think an admin must know what he's doing. If he requested sandbox but 
without kernel support he need to step back a little and understand what 
he's doing. This patch won't decrease the security level, IMHO.

>
> IMHO the test suite should probe to see if sandbox is working or not, and
> just not use the "-sandbox on" arg if the host doesn't support it.

But I think this could be done on virt-test as well :)

-- 
Eduardo Otubo
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Paul Moore]

On Monday, December 09, 2013 03:51:36 PM Eduardo Otubo wrote:
> On 12/09/2013 03:33 PM, Daniel P. Berrange wrote:
> > On Mon, Dec 09, 2013 at 03:20:52PM -0200, Eduardo Otubo wrote:
> >> This option was requested by virt-test team so they can run tests with
> >> Qemu and "-sandbox on" set without breaking whole test if host doesn't
> >> 
> >> have support for seccomp in kernel. It covers two possibilities:
> >>   1) Host kernel support does not support seccomp, but user installed
> >>   Qemu
> >>   
> >>      package with sandbox support: Libseccomp will fail -> qemu will fail
> >>      nicely and won't stop execution.
> >>   
> >>   2) Host kernel has support but Qemu package wasn't built with sandbox
> >>   
> >>      feature. Qemu will fail nicely and won't stop execution.
> >> 
> >> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> >> ---
> >> 
> >>   vl.c | 10 +++-------
> >>   1 file changed, 3 insertions(+), 7 deletions(-)

{snip}

> > This change is really dubious from a security POV. If the admin requested
> > sandboxing and the host or QEMU build cannot support it, then QEMU really
> > *must* exit.
> 
> I think an admin must know what he's doing. If he requested sandbox but
> without kernel support he need to step back a little and understand what
> he's doing. This patch won't decrease the security level, IMHO.

NACK

For the reasons Daniel already mentioned.  Mistakes happen, a lot, and if the 
user explicitly requests security functionality and we can't provide it we 
need to fail in a manner that doesn't increase the user's risk.

> > IMHO the test suite should probe to see if sandbox is working or not, and
> > just not use the "-sandbox on" arg if the host doesn't support it.
> 
> But I think this could be done on virt-test as well :)

This would be ideal, but if you must have a fallback mechanism in QEMU proper, 
make it separate from '-sandbox on' so that it doesn't break with the current 
behavior and also makes it is obvious that the functionality is not 
guaranteed, e.g. '-sandbox try' or similar.

-- 
paul moore
security and virtualization @ redhat

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Corey Bryant]

On 12/09/2013 12:51 PM, Eduardo Otubo wrote:
>
>
> On 12/09/2013 03:33 PM, Daniel P. Berrange wrote:
>> On Mon, Dec 09, 2013 at 03:20:52PM -0200, Eduardo Otubo wrote:
>>> This option was requested by virt-test team so they can run tests with
>>> Qemu and "-sandbox on" set without breaking whole test if host doesn't
>>> have support for seccomp in kernel. It covers two possibilities:
>>>
>>>   1) Host kernel support does not support seccomp, but user installed
>>> Qemu
>>>      package with sandbox support: Libseccomp will fail -> qemu will
>>> fail
>>>      nicely and won't stop execution.
>>>
>>>   2) Host kernel has support but Qemu package wasn't built with sandbox
>>>      feature. Qemu will fail nicely and won't stop execution.
>>>
>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>> ---
>>>   vl.c | 10 +++-------
>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index b0399de..a0806dc 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -967,13 +967,11 @@ static int parse_sandbox(QemuOpts *opts, void
>>> *opaque)
>>>   #ifdef CONFIG_SECCOMP
>>>           if (seccomp_start() < 0) {
>>>               qerror_report(ERROR_CLASS_GENERIC_ERROR,
>>> -                          "failed to install seccomp syscall filter
>>> in the kernel");
>>> -            return -1;
>>> +                          "failed to install seccomp syscall filter
>>> in the kernel, disabling it");
>>>           }
>>>   #else
>>>           qerror_report(ERROR_CLASS_GENERIC_ERROR,
>>> -                      "sandboxing request but seccomp is not
>>> compiled into this build");
>>> -        return -1;
>>> +                      "sandboxing request but seccomp is not
>>> compiled into this build, disabling it");
>>>   #endif
>>>       }
>>>
>>> @@ -3808,9 +3806,7 @@ int main(int argc, char **argv, char **envp)
>>>           exit(1);
>>>       }
>>>
>>> -    if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox,
>>> NULL, 0)) {
>>> -        exit(1);
>>> -    }
>>> +    qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox,
>>> NULL, 0);
>>>
>>>   #ifndef _WIN32
>>>       if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd,
>>> NULL, 1)) {
>>
>> This change is really dubious from a security POV. If the admin requested
>> sandboxing and the host or QEMU build cannot support it, then QEMU really
>> *must* exit.
>
> I think an admin must know what he's doing. If he requested sandbox but
> without kernel support he need to step back a little and understand what
> he's doing. This patch won't decrease the security level, IMHO.
>

Preventing qemu from exiting is definitely not the right approach. 
Please feel free to run code by me ahead of time in the future.

>>
>> IMHO the test suite should probe to see if sandbox is working or not, and
>> just not use the "-sandbox on" arg if the host doesn't support it.
>
> But I think this could be done on virt-test as well :)
>

This would make sense.

Although it sounds like Lucas was looking for an error message when 
seccomp kills qemu.  Maybe virt-test could grep the audit log for the 
existence of a "type=SECCOMP" record within the test's time of 
execution, and issue a message based on that.

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Lucas Meneghel Rodrigues]

On 12/10/2013 01:20 AM, Corey Bryant wrote:
>>> IMHO the test suite should probe to see if sandbox is working or not,
>>> and
>>> just not use the "-sandbox on" arg if the host doesn't support it.
>>
>> But I think this could be done on virt-test as well :)
>>
>
> This would make sense.
>
> Although it sounds like Lucas was looking for an error message when
> seccomp kills qemu.  Maybe virt-test could grep the audit log for the
> existence of a "type=SECCOMP" record within the test's time of
> execution, and issue a message based on that.

It's a valid idea. The problem I see with it is that not every distro 
out there uses SELinux. Not getting into the merits of whether they 
should, ideally it'd be nice to have this working on distros that won't 
use SELinux.

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Paul Moore]

On Tuesday, December 10, 2013 04:48:54 PM Lucas Meneghel Rodrigues wrote:
> On 12/10/2013 01:20 AM, Corey Bryant wrote:
> >>> IMHO the test suite should probe to see if sandbox is working or not,
> >>> and
> >>> just not use the "-sandbox on" arg if the host doesn't support it.
> >> 
> >> But I think this could be done on virt-test as well :)
> > 
> > This would make sense.
> > 
> > Although it sounds like Lucas was looking for an error message when
> > seccomp kills qemu.  Maybe virt-test could grep the audit log for the
> > existence of a "type=SECCOMP" record within the test's time of
> > execution, and issue a message based on that.
> 
> It's a valid idea. The problem I see with it is that not every distro
> out there uses SELinux. Not getting into the merits of whether they
> should, ideally it'd be nice to have this working on distros that won't
> use SELinux.

Minor point of clarification, but audit and SELinux and independent subsystems 
in the kernel.

Also, and I don't have a non-audit kernel built at the moment to verify this, 
but on non-audit kernels the audit messages should be sent to syslog so you 
*should* still be able to search for SECCOMP records regardless.

-- 
paul moore
security and virtualization @ redhat

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Lucas Meneghel Rodrigues]

On 12/10/2013 05:31 PM, Paul Moore wrote:
> On Tuesday, December 10, 2013 04:48:54 PM Lucas Meneghel Rodrigues wrote:
>> On 12/10/2013 01:20 AM, Corey Bryant wrote:
>>>>> IMHO the test suite should probe to see if sandbox is working or not,
>>>>> and
>>>>> just not use the "-sandbox on" arg if the host doesn't support it.
>>>>
>>>> But I think this could be done on virt-test as well :)
>>>
>>> This would make sense.
>>>
>>> Although it sounds like Lucas was looking for an error message when
>>> seccomp kills qemu.  Maybe virt-test could grep the audit log for the
>>> existence of a "type=SECCOMP" record within the test's time of
>>> execution, and issue a message based on that.
>>
>> It's a valid idea. The problem I see with it is that not every distro
>> out there uses SELinux. Not getting into the merits of whether they
>> should, ideally it'd be nice to have this working on distros that won't
>> use SELinux.
>
> Minor point of clarification, but audit and SELinux and independent subsystems
> in the kernel.
>
> Also, and I don't have a non-audit kernel built at the moment to verify this,
> but on non-audit kernels the audit messages should be sent to syslog so you
> *should* still be able to search for SECCOMP records regardless.

Ok, my bad, thanks for the clarification! We'll look into checking the 
audit log.

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Eduardo Otubo]

On 12/10/2013 04:48 PM, Lucas Meneghel Rodrigues wrote:
> On 12/10/2013 01:20 AM, Corey Bryant wrote:
>>>> IMHO the test suite should probe to see if sandbox is working or not,
>>>> and
>>>> just not use the "-sandbox on" arg if the host doesn't support it.
>>>
>>> But I think this could be done on virt-test as well :)
>>>
>>
>> This would make sense.
>>
>> Although it sounds like Lucas was looking for an error message when
>> seccomp kills qemu.  Maybe virt-test could grep the audit log for the
>> existence of a "type=SECCOMP" record within the test's time of
>> execution, and issue a message based on that.
>
> It's a valid idea. The problem I see with it is that not every distro
> out there uses SELinux. Not getting into the merits of whether they
> should, ideally it'd be nice to have this working on distros that won't
> use SELinux.
>
>
>

Completely misunderstanding, I feel sorry for that.

While we can't rely on the fact that every distro will have audit log 
working properly, I can start working on some support for virt-test to 
detect if the host machine has support for seccomp or if the Qemu binary 
has this feature built in.

Again, sorry for the mess. Please disconsider this patch.

-- 
Eduardo Otubo
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Lucas Meneghel Rodrigues]

On 12/09/2013 03:20 PM, Eduardo Otubo wrote:
> This option was requested by virt-test team so they can run tests with
> Qemu and "-sandbox on" set without breaking whole test if host doesn't
> have support for seccomp in kernel. It covers two possibilities:
>
>   1) Host kernel support does not support seccomp, but user installed Qemu
>      package with sandbox support: Libseccomp will fail -> qemu will fail
>      nicely and won't stop execution.
>
>   2) Host kernel has support but Qemu package wasn't built with sandbox
>      feature. Qemu will fail nicely and won't stop execution.

It seems there was a misunderstanding of what we wanted here. The 
problem we had there happened on a -sandbox bug on Fedora 19 that got 
one of our team members confused, since qemu did not give any sort of 
useful output that would allow him to identify the bug was related to 
-sandbox (qemu was accessing a syscall outside of the whitelist on 
Fedora 19).

He took a while until he figured out -sandbox was the problem, due to 
the lack of any clues of what was going on. I was not affected due to 
the fact I was already on Fedora 20 by that time.

I assume Eduardo thought that we somehow wanted qemu to just carry on 
when seccomp requirements could not be fulfilled, but that was not our 
point. What I thought I'd commented with hum was that some more useful 
message could be printed to stderr, and perhaps make the qemu process to 
exit with rc != 0 on such errors, instead of just going dead silently.

I still couldn't quite grasp why that could not be done, but if it 
can't, so be it. No big deal.

Lucas