[Qemu-devel] [PATCH] block/nfs: add support for setting debug level

[Qemu-devel] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

upcoming libnfs versions will support logging debug messages. Add
support for it in qemu through an URL parameter.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/nfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/nfs.c b/block/nfs.c
index ca9e24e..f7388a3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
         } else if (!strcmp(qp->p[i].name, "readahead")) {
             nfs_set_readahead(client->context, val);
 #endif
+#ifdef LIBNFS_FEATURE_DEBUG
+        } else if (!strcmp(qp->p[i].name, "debug")) {
+            nfs_set_debug(client->context, val);
+#endif
         } else {
             error_setg(errp, "Unknown NFS parameter name: %s",
                        qp->p[i].name);
-- 
1.9.1

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
> upcoming libnfs versions will support logging debug messages. Add
> support for it in qemu through an URL parameter.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/nfs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index ca9e24e..f7388a3 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>          } else if (!strcmp(qp->p[i].name, "readahead")) {
>              nfs_set_readahead(client->context, val);
>  #endif
> +#ifdef LIBNFS_FEATURE_DEBUG
> +        } else if (!strcmp(qp->p[i].name, "debug")) {
> +            nfs_set_debug(client->context, val);
> +#endif
>          } else {
>              error_setg(errp, "Unknown NFS parameter name: %s",
>                         qp->p[i].name);

Untrusted users may be able to set these options since they are encoded
in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.

A verbose debug level spams stderr and could consume a lot of disk
space.

(The uid and gid options are probably okay since the NFS server cannot
trust the uid/gid coming from QEMU anyway.)

I think we can merge this patch for QEMU 2.4 but I'd like to have a
discussion about the security risk of encoding libnfs options in the
URI.

CCed Eric Blake in case libvirt is affected.

Has anyone thought about this and what are the rules?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>> upcoming libnfs versions will support logging debug messages. Add
>> support for it in qemu through an URL parameter.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/nfs.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/block/nfs.c b/block/nfs.c
>> index ca9e24e..f7388a3 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>>           } else if (!strcmp(qp->p[i].name, "readahead")) {
>>               nfs_set_readahead(client->context, val);
>>   #endif
>> +#ifdef LIBNFS_FEATURE_DEBUG
>> +        } else if (!strcmp(qp->p[i].name, "debug")) {
>> +            nfs_set_debug(client->context, val);
>> +#endif
>>           } else {
>>               error_setg(errp, "Unknown NFS parameter name: %s",
>>                          qp->p[i].name);
> Untrusted users may be able to set these options since they are encoded
> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
>
> A verbose debug level spams stderr and could consume a lot of disk
> space.
>
> (The uid and gid options are probably okay since the NFS server cannot
> trust the uid/gid coming from QEMU anyway.)
>
> I think we can merge this patch for QEMU 2.4 but I'd like to have a
> discussion about the security risk of encoding libnfs options in the
> URI.
>
> CCed Eric Blake in case libvirt is affected.
>
> Has anyone thought about this and what are the rules?

Good point. In general I think there should be some kind of sanitization of the parameters
before they are passed on to Qemu. In our use case the user cannot pass any kind of URIs himself,
but this might be different in other backends. The readahead value is as dangerous as well
if not sanitized.

I had a discussion with Ronnie in the past if we should encode parameters in the URI or via environment
like it is done in libiscsi. If I remember correctly we came up with the URI parameters for better usability,
but hadn't attack scenarios in mind.

I am also open to only allow uncritical parameters in the URI and pass others via a new -nfs cmdline option.
Or limit max readahead and max debug level settable via URI.

Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2795 bytes --]

On Thu, Jun 25, 2015 at 03:26:46PM +0200, Peter Lieven wrote:
> Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
> >On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
> >>upcoming libnfs versions will support logging debug messages. Add
> >>support for it in qemu through an URL parameter.
> >>
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >>  block/nfs.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >>diff --git a/block/nfs.c b/block/nfs.c
> >>index ca9e24e..f7388a3 100644
> >>--- a/block/nfs.c
> >>+++ b/block/nfs.c
> >>@@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
> >>          } else if (!strcmp(qp->p[i].name, "readahead")) {
> >>              nfs_set_readahead(client->context, val);
> >>  #endif
> >>+#ifdef LIBNFS_FEATURE_DEBUG
> >>+        } else if (!strcmp(qp->p[i].name, "debug")) {
> >>+            nfs_set_debug(client->context, val);
> >>+#endif
> >>          } else {
> >>              error_setg(errp, "Unknown NFS parameter name: %s",
> >>                         qp->p[i].name);
> >Untrusted users may be able to set these options since they are encoded
> >in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
> >
> >A verbose debug level spams stderr and could consume a lot of disk
> >space.
> >
> >(The uid and gid options are probably okay since the NFS server cannot
> >trust the uid/gid coming from QEMU anyway.)
> >
> >I think we can merge this patch for QEMU 2.4 but I'd like to have a
> >discussion about the security risk of encoding libnfs options in the
> >URI.
> >
> >CCed Eric Blake in case libvirt is affected.
> >
> >Has anyone thought about this and what are the rules?
> 
> Good point. In general I think there should be some kind of sanitization of the parameters
> before they are passed on to Qemu. In our use case the user cannot pass any kind of URIs himself,
> but this might be different in other backends. The readahead value is as dangerous as well
> if not sanitized.
> 
> I had a discussion with Ronnie in the past if we should encode parameters in the URI or via environment
> like it is done in libiscsi. If I remember correctly we came up with the URI parameters for better usability,
> but hadn't attack scenarios in mind.
> 
> I am also open to only allow uncritical parameters in the URI and pass others via a new -nfs cmdline option.
> Or limit max readahead and max debug level settable via URI.

I'd feel safer if the option was in runtime_opts instead.  The the
management tool has to pass them explicitly and the end user cannot
influence them via the URI.

If an option is needed both at open and create time, then it must also
be parsed from nfs_file_create() opts.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

Am 26.06.2015 um 11:14 schrieb Stefan Hajnoczi:
> On Thu, Jun 25, 2015 at 03:26:46PM +0200, Peter Lieven wrote:
>> Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
>>> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>>>> upcoming libnfs versions will support logging debug messages. Add
>>>> support for it in qemu through an URL parameter.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>  block/nfs.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/block/nfs.c b/block/nfs.c
>>>> index ca9e24e..f7388a3 100644
>>>> --- a/block/nfs.c
>>>> +++ b/block/nfs.c
>>>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>>>>          } else if (!strcmp(qp->p[i].name, "readahead")) {
>>>>              nfs_set_readahead(client->context, val);
>>>>  #endif
>>>> +#ifdef LIBNFS_FEATURE_DEBUG
>>>> +        } else if (!strcmp(qp->p[i].name, "debug")) {
>>>> +            nfs_set_debug(client->context, val);
>>>> +#endif
>>>>          } else {
>>>>              error_setg(errp, "Unknown NFS parameter name: %s",
>>>>                         qp->p[i].name);
>>> Untrusted users may be able to set these options since they are encoded
>>> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
>>>
>>> A verbose debug level spams stderr and could consume a lot of disk
>>> space.
>>>
>>> (The uid and gid options are probably okay since the NFS server cannot
>>> trust the uid/gid coming from QEMU anyway.)
>>>
>>> I think we can merge this patch for QEMU 2.4 but I'd like to have a
>>> discussion about the security risk of encoding libnfs options in the
>>> URI.
>>>
>>> CCed Eric Blake in case libvirt is affected.
>>>
>>> Has anyone thought about this and what are the rules?
>> Good point. In general I think there should be some kind of sanitization of the parameters
>> before they are passed on to Qemu. In our use case the user cannot pass any kind of URIs himself,
>> but this might be different in other backends. The readahead value is as dangerous as well
>> if not sanitized.
>>
>> I had a discussion with Ronnie in the past if we should encode parameters in the URI or via environment
>> like it is done in libiscsi. If I remember correctly we came up with the URI parameters for better usability,
>> but hadn't attack scenarios in mind.
>>
>> I am also open to only allow uncritical parameters in the URI and pass others via a new -nfs cmdline option.
>> Or limit max readahead and max debug level settable via URI.
> I'd feel safer if the option was in runtime_opts instead.  The the
> management tool has to pass them explicitly and the end user cannot
> influence them via the URI.
>
> If an option is needed both at open and create time, then it must also
> be parsed from nfs_file_create() opts.

Ok, I will send a patch that follows this approach. And also a second one to
limit the readahead size to a reasonable value.

Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>> upcoming libnfs versions will support logging debug messages. Add
>> support for it in qemu through an URL parameter.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/nfs.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/block/nfs.c b/block/nfs.c
>> index ca9e24e..f7388a3 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>>           } else if (!strcmp(qp->p[i].name, "readahead")) {
>>               nfs_set_readahead(client->context, val);
>>   #endif
>> +#ifdef LIBNFS_FEATURE_DEBUG
>> +        } else if (!strcmp(qp->p[i].name, "debug")) {
>> +            nfs_set_debug(client->context, val);
>> +#endif
>>           } else {
>>               error_setg(errp, "Unknown NFS parameter name: %s",
>>                          qp->p[i].name);
> Untrusted users may be able to set these options since they are encoded
> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
>
> A verbose debug level spams stderr and could consume a lot of disk
> space.
>
> (The uid and gid options are probably okay since the NFS server cannot
> trust the uid/gid coming from QEMU anyway.)
>
> I think we can merge this patch for QEMU 2.4 but I'd like to have a
> discussion about the security risk of encoding libnfs options in the
> URI.
>
> CCed Eric Blake in case libvirt is affected.
>
> Has anyone thought about this and what are the rules?

As I hadn't time to work further on the best way to add options for NFS (and other
protocols), would it be feasible to allow passing debug as an URL parameter, but
limit the maximum debug level to limit a possible security impact (flooding logs)?

If a higher debug level is needed it can be set via device specific options as soon
there is a common scheme for them.

Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

On 06/25/2015 07:18 AM, Stefan Hajnoczi wrote:
> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>> upcoming libnfs versions will support logging debug messages. Add
>> support for it in qemu through an URL parameter.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/nfs.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>

> 
> Untrusted users may be able to set these options since they are encoded
> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
> 
> A verbose debug level spams stderr and could consume a lot of disk
> space.
> 
> (The uid and gid options are probably okay since the NFS server cannot
> trust the uid/gid coming from QEMU anyway.)
> 
> I think we can merge this patch for QEMU 2.4 but I'd like to have a
> discussion about the security risk of encoding libnfs options in the
> URI.
> 
> CCed Eric Blake in case libvirt is affected.

Libvirt doesn't (yet) support XML describing debug parameters, and its
current XML does not let the user specify a raw URL, but rather the
individual pieces that libvirt then concatenates into the URL.
Basically, libvirt already uses a structured request, the way we
eventually want working for QMP blockdev-add for NFS images, with all
features broken into individual parameters within the struct rather than
a URL.  So from that perspective, I don't think exposing a debug
parameter in the NFS URL will hurt libvirt, but it doesn't answer
whether you'd have a security (log-filling) issue for uses of the URL
outside of libvirt.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

Am 22.09.2015 um 08:13 schrieb Peter Lieven:
> Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
>> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>>> upcoming libnfs versions will support logging debug messages. Add
>>> support for it in qemu through an URL parameter.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>   block/nfs.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/block/nfs.c b/block/nfs.c
>>> index ca9e24e..f7388a3 100644
>>> --- a/block/nfs.c
>>> +++ b/block/nfs.c
>>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>>>           } else if (!strcmp(qp->p[i].name, "readahead")) {
>>>               nfs_set_readahead(client->context, val);
>>>   #endif
>>> +#ifdef LIBNFS_FEATURE_DEBUG
>>> +        } else if (!strcmp(qp->p[i].name, "debug")) {
>>> +            nfs_set_debug(client->context, val);
>>> +#endif
>>>           } else {
>>>               error_setg(errp, "Unknown NFS parameter name: %s",
>>>                          qp->p[i].name);
>> Untrusted users may be able to set these options since they are encoded
>> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
>>
>> A verbose debug level spams stderr and could consume a lot of disk
>> space.
>>
>> (The uid and gid options are probably okay since the NFS server cannot
>> trust the uid/gid coming from QEMU anyway.)
>>
>> I think we can merge this patch for QEMU 2.4 but I'd like to have a
>> discussion about the security risk of encoding libnfs options in the
>> URI.
>>
>> CCed Eric Blake in case libvirt is affected.
>>
>> Has anyone thought about this and what are the rules?
>
> As I hadn't time to work further on the best way to add options for NFS (and other
> protocols), would it be feasible to allow passing debug as an URL parameter, but
> limit the maximum debug level to limit a possible security impact (flooding logs)?
>
> If a higher debug level is needed it can be set via device specific options as soon
> there is a common scheme for them.

Any objections?

Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Stefan Hajnoczi]

On Thu, Oct 22, 2015 at 08:37:19AM +0200, Peter Lieven wrote:
> Am 22.09.2015 um 08:13 schrieb Peter Lieven:
> >Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
> >>On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
> >>>upcoming libnfs versions will support logging debug messages. Add
> >>>support for it in qemu through an URL parameter.
> >>>
> >>>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>>---
> >>>  block/nfs.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>>diff --git a/block/nfs.c b/block/nfs.c
> >>>index ca9e24e..f7388a3 100644
> >>>--- a/block/nfs.c
> >>>+++ b/block/nfs.c
> >>>@@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
> >>>          } else if (!strcmp(qp->p[i].name, "readahead")) {
> >>>              nfs_set_readahead(client->context, val);
> >>>  #endif
> >>>+#ifdef LIBNFS_FEATURE_DEBUG
> >>>+        } else if (!strcmp(qp->p[i].name, "debug")) {
> >>>+            nfs_set_debug(client->context, val);
> >>>+#endif
> >>>          } else {
> >>>              error_setg(errp, "Unknown NFS parameter name: %s",
> >>>                         qp->p[i].name);
> >>Untrusted users may be able to set these options since they are encoded
> >>in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
> >>
> >>A verbose debug level spams stderr and could consume a lot of disk
> >>space.
> >>
> >>(The uid and gid options are probably okay since the NFS server cannot
> >>trust the uid/gid coming from QEMU anyway.)
> >>
> >>I think we can merge this patch for QEMU 2.4 but I'd like to have a
> >>discussion about the security risk of encoding libnfs options in the
> >>URI.
> >>
> >>CCed Eric Blake in case libvirt is affected.
> >>
> >>Has anyone thought about this and what are the rules?
> >
> >As I hadn't time to work further on the best way to add options for NFS (and other
> >protocols), would it be feasible to allow passing debug as an URL parameter, but
> >limit the maximum debug level to limit a possible security impact (flooding logs)?
> >
> >If a higher debug level is needed it can be set via device specific options as soon
> >there is a common scheme for them.
> 
> Any objections?

If you are sure that ERROR and WARN levels (or similar) don't flood the
logs, then it sounds like a solution.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

Am 26.10.2015 um 11:45 schrieb Stefan Hajnoczi:
> On Thu, Oct 22, 2015 at 08:37:19AM +0200, Peter Lieven wrote:
>> Am 22.09.2015 um 08:13 schrieb Peter Lieven:
>>> Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
>>>> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>>>>> upcoming libnfs versions will support logging debug messages. Add
>>>>> support for it in qemu through an URL parameter.
>>>>>
>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>> ---
>>>>>   block/nfs.c | 4 ++++
>>>>>   1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/block/nfs.c b/block/nfs.c
>>>>> index ca9e24e..f7388a3 100644
>>>>> --- a/block/nfs.c
>>>>> +++ b/block/nfs.c
>>>>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>>>>>           } else if (!strcmp(qp->p[i].name, "readahead")) {
>>>>>               nfs_set_readahead(client->context, val);
>>>>>   #endif
>>>>> +#ifdef LIBNFS_FEATURE_DEBUG
>>>>> +        } else if (!strcmp(qp->p[i].name, "debug")) {
>>>>> +            nfs_set_debug(client->context, val);
>>>>> +#endif
>>>>>           } else {
>>>>>               error_setg(errp, "Unknown NFS parameter name: %s",
>>>>>                          qp->p[i].name);
>>>> Untrusted users may be able to set these options since they are encoded
>>>> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
>>>>
>>>> A verbose debug level spams stderr and could consume a lot of disk
>>>> space.
>>>>
>>>> (The uid and gid options are probably okay since the NFS server cannot
>>>> trust the uid/gid coming from QEMU anyway.)
>>>>
>>>> I think we can merge this patch for QEMU 2.4 but I'd like to have a
>>>> discussion about the security risk of encoding libnfs options in the
>>>> URI.
>>>>
>>>> CCed Eric Blake in case libvirt is affected.
>>>>
>>>> Has anyone thought about this and what are the rules?
>>> As I hadn't time to work further on the best way to add options for NFS (and other
>>> protocols), would it be feasible to allow passing debug as an URL parameter, but
>>> limit the maximum debug level to limit a possible security impact (flooding logs)?
>>>
>>> If a higher debug level is needed it can be set via device specific options as soon
>>> there is a common scheme for them.
>> Any objections?
> If you are sure that ERROR and WARN levels (or similar) don't flood the
> logs, then it sounds like a solution.

Thats not the case. I use debug level 2 for quite some time. Mainly to see NFS connection interruptions.

So I would be happy if we could allow for debug <= 2 from the cmdline.

Peter