The packet with passed FD has the following format:

    --------------------------
    | len | header | payload |
    --------------------------

where "payload" has an additional count of FDs before the actual data:

    ------------------
    | nfds | payload |
    ------------------

When the packet is received we parse the "header", which as a side
effect updates msg->bufferOffset to point to the beginning of "payload".
If the message call contains FDs, we need to also parse the count of
FDs, which also updates the msg->bufferOffset.

The issue here is that when we attempt to read the FDs data from the
socket and we receive EAGAIN we finish the reading and call poll()
to wait for the data the we need.  When the data arrives we already have
the packet in our buffer so we read the "header" again but this time
we don't read the count of FDs because we already have it stored.

That means that the msg->bufferOffset is not updated to point to the
actual beginning of the payload data, but it points to the count of
FDs.  After all FDs are processed we dispatch the message to process
it and decode the payload.  Since the msg->bufferOffset points to wrong
data, we decode the wrong payload and the API call fails with
error messages:

    Domain not found: no domain with matching uuid '67656e65-7269-6300-0c87-5003ca6941f2' ()

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/rpc/virnetclient.c       |  3 +--
 src/rpc/virnetmessage.c      | 12 +++++++-----
 src/rpc/virnetserverclient.c |  3 +--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 95cd9a6c7e..eb46e34301 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1428,8 +1428,7 @@ virNetClientIOHandleInput(virNetClientPtr client)
                 if (client->msg.header.type == VIR_NET_REPLY_WITH_FDS) {
                     size_t i;
 
-                    if (client->msg.nfds == 0 &&
-                        virNetMessageDecodeNumFDs(&client->msg) < 0)
+                    if (virNetMessageDecodeNumFDs(&client->msg) < 0)
                         return -1;
 
                     for (i = client->msg.donefds; i < client->msg.nfds; i++) {
diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
index 5908b074a8..94c4c89e4f 100644
--- a/src/rpc/virnetmessage.c
+++ b/src/rpc/virnetmessage.c
@@ -327,11 +327,13 @@ int virNetMessageDecodeNumFDs(virNetMessagePtr msg)
         goto cleanup;
     }
 
-    msg->nfds = numFDs;
-    if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)
-        goto cleanup;
-    for (i = 0; i < msg->nfds; i++)
-        msg->fds[i] = -1;
+    if (msg->nfds == 0) {
+        msg->nfds = numFDs;
+        if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)
+            goto cleanup;
+        for (i = 0; i < msg->nfds; i++)
+            msg->fds[i] = -1;
+    }
 
     VIR_DEBUG("Got %zu FDs from peer", msg->nfds);
 
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index fa4e5daabb..6e086b7b4e 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1189,8 +1189,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
         /* Now figure out if we need to read more data to get some
          * file descriptors */
         if (msg->header.type == VIR_NET_CALL_WITH_FDS) {
-            if (msg->nfds == 0 &&
-                virNetMessageDecodeNumFDs(msg) < 0) {
+            if (virNetMessageDecodeNumFDs(msg) < 0) {
                 virNetMessageQueueServe(&client->rx);
                 virNetMessageFree(msg);
                 client->wantClose = true;
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Sep 26, 2017 at 05:13:37PM +0200, Pavel Hrdina wrote:
>The packet with passed FD has the following format:
>
>    --------------------------
>    | len | header | payload |
>    --------------------------
>
>where "payload" has an additional count of FDs before the actual data:
>
>    ------------------
>    | nfds | payload |
>    ------------------
>
>When the packet is received we parse the "header", which as a side
>effect updates msg->bufferOffset to point to the beginning of "payload".
>If the message call contains FDs, we need to also parse the count of
>FDs, which also updates the msg->bufferOffset.
>
>The issue here is that when we attempt to read the FDs data from the
>socket and we receive EAGAIN we finish the reading and call poll()
>to wait for the data the we need.  When the data arrives we already have
>the packet in our buffer so we read the "header" again but this time
>we don't read the count of FDs because we already have it stored.
>
>That means that the msg->bufferOffset is not updated to point to the
>actual beginning of the payload data, but it points to the count of
>FDs.  After all FDs are processed we dispatch the message to process
>it and decode the payload.  Since the msg->bufferOffset points to wrong
>data, we decode the wrong payload and the API call fails with
>error messages:
>
>    Domain not found: no domain with matching uuid '67656e65-7269-6300-0c87-5003ca6941f2' ()
>

It would be nice to mention the commit that removed the repeated calling
of virNetMessageDecodeNumFDs because of the memleak.

>Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>---
> src/rpc/virnetclient.c       |  3 +--
> src/rpc/virnetmessage.c      | 12 +++++++-----
> src/rpc/virnetserverclient.c |  3 +--
> 3 files changed, 9 insertions(+), 9 deletions(-)
>

ACK regardless of my nosy question below

>diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
>index 5908b074a8..94c4c89e4f 100644
>--- a/src/rpc/virnetmessage.c
>+++ b/src/rpc/virnetmessage.c
>@@ -327,11 +327,13 @@ int virNetMessageDecodeNumFDs(virNetMessagePtr msg)
>         goto cleanup;
>     }
>
>-    msg->nfds = numFDs;
>-    if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)
>-        goto cleanup;
>-    for (i = 0; i < msg->nfds; i++)
>-        msg->fds[i] = -1;
>+    if (msg->nfds == 0) {
>+        msg->nfds = numFDs;
>+        if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)

Could this leak memory if someone sends us maliciously crafted RPC data
where nfds = 0 and we call the VIR_ALLOC_N multiple times?

Jan

>+            goto cleanup;
>+        for (i = 0; i < msg->nfds; i++)
>+            msg->fds[i] = -1;
>+    }
>
>     VIR_DEBUG("Got %zu FDs from peer", msg->nfds);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> On Tue, Sep 26, 2017 at 05:13:37PM +0200, Pavel Hrdina wrote:
> >The packet with passed FD has the following format:
> >
> >    --------------------------
> >    | len | header | payload |
> >    --------------------------
> >
> >where "payload" has an additional count of FDs before the actual data:
> >
> >    ------------------
> >    | nfds | payload |
> >    ------------------
> >
> >When the packet is received we parse the "header", which as a side
> >effect updates msg->bufferOffset to point to the beginning of "payload".
> >If the message call contains FDs, we need to also parse the count of
> >FDs, which also updates the msg->bufferOffset.
> >
> >The issue here is that when we attempt to read the FDs data from the
> >socket and we receive EAGAIN we finish the reading and call poll()
> >to wait for the data the we need.  When the data arrives we already have
> >the packet in our buffer so we read the "header" again but this time
> >we don't read the count of FDs because we already have it stored.
> >
> >That means that the msg->bufferOffset is not updated to point to the
> >actual beginning of the payload data, but it points to the count of
> >FDs.  After all FDs are processed we dispatch the message to process
> >it and decode the payload.  Since the msg->bufferOffset points to wrong
> >data, we decode the wrong payload and the API call fails with
> >error messages:
> >
> >    Domain not found: no domain with matching uuid
> >    '67656e65-7269-6300-0c87-5003ca6941f2' ()
> >
> 
> It would be nice to mention the commit that removed the repeated calling
> of virNetMessageDecodeNumFDs because of the memleak.
> 
> >Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >---
> > src/rpc/virnetclient.c       |  3 +--
> > src/rpc/virnetmessage.c      | 12 +++++++-----
> > src/rpc/virnetserverclient.c |  3 +--
> > 3 files changed, 9 insertions(+), 9 deletions(-)
> >
> 
> ACK regardless of my nosy question below

Thanks, I'll push it with mentioning the commit that broke it.

> >diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
> >index 5908b074a8..94c4c89e4f 100644
> >--- a/src/rpc/virnetmessage.c
> >+++ b/src/rpc/virnetmessage.c
> >@@ -327,11 +327,13 @@ int virNetMessageDecodeNumFDs(virNetMessagePtr msg)
> >         goto cleanup;
> >     }
> >
> >-    msg->nfds = numFDs;
> >-    if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)
> >-        goto cleanup;
> >-    for (i = 0; i < msg->nfds; i++)
> >-        msg->fds[i] = -1;
> >+    if (msg->nfds == 0) {
> >+        msg->nfds = numFDs;
> >+        if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)
> 
> Could this leak memory if someone sends us maliciously crafted RPC data
> where nfds = 0 and we call the VIR_ALLOC_N multiple times?

No, because the virNetSocketRecvFD() is not called in that case and we will
finish successfully to parse that packet.  It will not be called multiple
times.

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list