Libguestfs - Sep 2019 - Re: [PATCH nbdkit 4/4] reflection: Enhance plugin to support client address mode.

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> ---
Short commit message; at a minimum, I'd probably at least mention that
we thought about potential security issues, and didn't see how it could
be abused.
>  .../reflection/nbdkit-reflection-plugin.pod   | 23 ++++-
>  plugins/reflection/reflection.c               | 88 +++++++++++++++++++
>  tests/Makefile.am                             |  2 +
>  tests/test-reflection-address.sh              | 63 +++++++++++++
>  4 files changed, 174 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/reflection/nbdkit-reflection-plugin.pod
b/plugins/reflection/nbdkit-reflection-plugin.pod
> index 1b260b6..7f52c58 100644
> --- a/plugins/reflection/nbdkit-reflection-plugin.pod
> +++ b/plugins/reflection/nbdkit-reflection-plugin.pod
> @@ -1,10 +1,10 @@
>  =head1 NAME
>  
> -nbdkit-reflection-plugin - reflect export name back to the client
> +nbdkit-reflection-plugin - reflect client info back to the client
Could perhaps squash this hunk into patch 1, but it's also fine here.
> +Another use for the reflection plugin is to send back the client's IP
> +address:
> +
> + $ nbdkit reflection mode=address
> + $ nbdsh -u 'nbd://localhost' -c 'print(h.pread(h.get_size(),
0))'
> +
> +which will print something like:
> +
> + b'[::1]:58912'
Do we want a mode that attempts to do DNS lookup to convert an address
back to a name, so that this could result in b'localhost:58912'?

> +
> +  case AF_UNIX:
> +    /* We don't want to expose the socket path because it's a host
> +     * filesystem name.  The client might not really be running on the
> +     * same machine (eg. it using a proxy).  However it doesn't even
missing 'is'

> +++ b/tests/test-reflection-address.sh
> @@ -0,0 +1,63 @@
> +# Test the relection plugin with mode=address.
reflection (hmm, I missed that typo in patch 1, where this was copied from)
> +# Run nbdkit.
> +start_nbdkit -P reflection-address.pid -U $sock \
> +       reflection mode=address
Is it worth also trying to test a TCP socket (although we have to worry
about finding a free port for the server, as well as heavily filtering
the result as it might be [::1] or 127.0.0.1, and most certainly will
have a different client port number per test run.

But overall, I think this is a useful thing to add to the plugin, and
I'm not seeing any security holes in letting the client know the
client's IP address as seen by the server.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Sep 16, 2019 at 11:01:49AM -0500, Eric Blake
wrote:
> On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> > +Another use for the reflection plugin is to send back the
client's IP
> > +address:
> > +
> > + $ nbdkit reflection mode=address
> > + $ nbdsh -u 'nbd://localhost' -c
'print(h.pread(h.get_size(), 0))'
> > +
> > +which will print something like:
> > +
> > + b'[::1]:58912'
> 
> Do we want a mode that attempts to do DNS lookup to convert an address
> back to a name, so that this could result in b'localhost:58912'?
I suppose this could reveal too much information about the server's
(DNS?) configuration?
> > +
> > +  case AF_UNIX:
> > +    /* We don't want to expose the socket path because it's a
host
> > +     * filesystem name.  The client might not really be running on
the
> > +     * same machine (eg. it using a proxy).  However it doesn't
even
> 
> missing 'is'
Will fix.
> > +++ b/tests/test-reflection-address.sh
> > @@ -0,0 +1,63 @@
> 
> > +# Test the relection plugin with mode=address.
> 
> reflection (hmm, I missed that typo in patch 1, where this was copied from)
Oops - fixed in all 3 places.
> > +# Run nbdkit.
> > +start_nbdkit -P reflection-address.pid -U $sock \
> > +       reflection mode=address
> 
> Is it worth also trying to test a TCP socket (although we have to worry
> about finding a free port for the server, as well as heavily filtering
> the result as it might be [::1] or 127.0.0.1, and most certainly will
> have a different client port number per test run.
Yes it's because of this problem that I'm only testing a Unix domain
socket here.

We do have a few tests which use localhost, but they occasionally fail
in Koji especially because our algorithm to pick a free port is racy:

https://github.com/libguestfs/nbdkit/blob/03a2cc3d766edb17011dafe939e53d0f60f1c99b/tests/functions.sh.in#L128

As far as I know there's not a good way to fix this except to have
nbdkit choose a port, but that way is problematic in other ways.
Perhaps there is some way to have an external process hang on to a
port and pass it to nbdkit (using -s?)
> But overall, I think this is a useful thing to add to the plugin, and
> I'm not seeing any security holes in letting the client know the
> client's IP address as seen by the server.
OK I'll fix up all the things you have mentioned and push it, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 9/17/19 3:14 AM, Richard W.M. Jones wrote:
> On Mon, Sep 16, 2019 at 11:01:49AM -0500, Eric Blake wrote:
>> On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
>>> +Another use for the reflection plugin is to send back the
client's IP
>>> +address:
>>> +
>>> + $ nbdkit reflection mode=address
>>> + $ nbdsh -u 'nbd://localhost' -c
'print(h.pread(h.get_size(), 0))'
>>> +
>>> +which will print something like:
>>> +
>>> + b'[::1]:58912'
>>
>> Do we want a mode that attempts to do DNS lookup to convert an address
>> back to a name, so that this could result in
b'localhost:58912'?
> 
> I suppose this could reveal too much information about the server's
> (DNS?) configuration?
Probably true; the client can do the reverse lookup after the fact (but
then it is the client's configuration, not the server's, that matters).

> We do have a few tests which use localhost, but they occasionally fail
> in Koji especially because our algorithm to pick a free port is racy:
> 
>
https://github.com/libguestfs/nbdkit/blob/03a2cc3d766edb17011dafe939e53d0f60f1c99b/tests/functions.sh.in#L128
> 
> As far as I know there's not a good way to fix this except to have
> nbdkit choose a port, but that way is problematic in other ways.
> Perhaps there is some way to have an external process hang on to a
> port and pass it to nbdkit (using -s?)
Probably.  Another thing I might play with today.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org