| Message ID | 1435047135-31647-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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?
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
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
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
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
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.
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
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
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
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);
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(+)