Libguestfs - Jun 2019 - [libnbd PATCH] states: Validate error message size

If the server passes us a malformed error reply type with a message
length longer than the overall structured reply, we would blindly obey
that size and get out of sync with the server (perhaps even hanging on
a read for data that will never come).  Broken since its introduction
in commit 28952eda.

Fix it by parsing the tail of an error separate from the message,
which also lets us add other sanity checking, and makes it easier if a
future patch wants to capture instead of ignore the server's message.
---

I'm looking at installing a few other length sanity checks as well: we
ought to require that the server's answers to NBD_OPT and NBD_CMD
don't exceed MAX_REQUEST_SIZE (other than a bit of fudge to allow
payload + header when handling NBD_CMD_READ for 64M). Also, the NBD
spec says strings must not exceed 4k (we need to enforce this on the
export name we send to the server, and should also check any string
field the server replies to us).

 generator/generator                 |  7 ++++
 generator/states-reply-structured.c | 64 ++++++++++++++++++++++++++++-
 lib/internal.h                      |  1 +
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/generator/generator b/generator/generator
index 3b0ca82..deb77f0 100755
--- a/generator/generator
+++ b/generator/generator
@@ -771,6 +771,13 @@ and structured_reply_state_machine = [
     external_events = [ NotifyRead, "" ];
   };

+  State {
+    default_state with
+    name = "RECV_ERROR_TAIL";
+    comment = "Receive a structured reply error tail";
+    external_events = [ NotifyRead, "" ];
+  };
+
   State {
     default_state with
     name = "RECV_OFFSET_DATA";
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 5791360..6d58742 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -149,21 +149,69 @@
   }

  REPLY.STRUCTURED_REPLY.RECV_ERROR:
+  uint32_t length, msglen;
+
   switch (recv_into_rbuf (h)) {
   case -1: SET_NEXT_STATE (%.DEAD); return -1;
   case 0:
+    length = be32toh (h->sbuf.sr.structured_reply.length);
+    msglen = be16toh (h->sbuf.sr.payload.error.len);
+    if (msglen > length - sizeof h->sbuf.sr.payload.error) {
+      SET_NEXT_STATE (%.DEAD);
+      set_error (0, "error message length too large");
+      return -1;
+    }
+
     /* We skip the human readable error for now. XXX */
     h->rbuf = NULL;
-    h->rlen = be16toh (h->sbuf.sr.payload.error.len);
+    h->rlen = msglen;
     SET_NEXT_STATE (%RECV_ERROR_MESSAGE);
   }
   return 0;

  REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
+  uint32_t length, msglen;
+  uint16_t type;
+
+  switch (recv_into_rbuf (h)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return -1;
+  case 0:
+    length = be32toh (h->sbuf.sr.structured_reply.length);
+    msglen = be16toh (h->sbuf.sr.payload.error.len);
+    type = be16toh (h->sbuf.sr.structured_reply.type);
+
+    length -= sizeof h->sbuf.sr.payload.error - msglen;
+
+    /* Special case two specific errors; ignore the tail for all others */
+    h->rbuf = NULL;
+    h->rlen = length;
+    switch (type) {
+    case NBD_REPLY_TYPE_ERROR:
+      if (length != 0) {
+        SET_NEXT_STATE (%.DEAD);
+        set_error (0, "error payload length too large");
+        return -1;
+      }
+      break;
+    case NBD_REPLY_TYPE_ERROR_OFFSET:
+      if (length != sizeof h->sbuf.offset) {
+        SET_NEXT_STATE (%.DEAD);
+        set_error (0, "error payload length too large");
+        return -1;
+      }
+      h->rbuf = &h->sbuf.offset;
+      break;
+    }
+    SET_NEXT_STATE (%RECV_ERROR_TAIL);
+  }
+  return 0;
+
+ REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
   struct command_in_flight *cmd;
   uint16_t flags;
   uint64_t handle;
   uint32_t error;
+  uint64_t offset;

   switch (recv_into_rbuf (h)) {
   case -1: SET_NEXT_STATE (%.DEAD); return -1;
@@ -183,6 +231,20 @@
     if (cmd->error == 0)
       cmd->error = nbd_internal_errno_of_nbd_error (error);

+    /* Sanity check that any error offset is in range */
+    if (error == NBD_REPLY_TYPE_ERROR_OFFSET) {
+      offset = be64toh (h->sbuf.offset);
+      if (offset < cmd->offset || offset >= cmd->offset +
cmd->count) {
+        SET_NEXT_STATE (%.DEAD);
+        set_error (0, "offset of error reply is out of bounds, "
+                   "offset=%" PRIu64 ", cmd->offset=%"
PRIu64 ", "
+                   "cmd->count=%" PRIu32 ", "
+                   "this is likely to be a bug in the server",
+                   offset, cmd->offset, cmd->count);
+        return -1;
+      }
+    }
+
     if (flags & NBD_REPLY_FLAG_DONE)
       SET_NEXT_STATE (%^FINISH_COMMAND);
     else
diff --git a/lib/internal.h b/lib/internal.h
index e7be05b..7ad6219 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -143,6 +143,7 @@ struct nbd_handle {
     uint32_t len;
     uint16_t nrinfos;
     uint32_t nrqueries;
+    uint64_t offset;
   } sbuf;

   /* Issuing a command must use a buffer separate from sbuf, for the
-- 
2.20.1

On 6/14/19 8:08 AM, Eric Blake wrote:
> If the server passes us a malformed error reply type with a message
> length longer than the overall structured reply, we would blindly obey
> that size and get out of sync with the server (perhaps even hanging on
> a read for data that will never come).  Broken since its introduction
> in commit 28952eda.
> 
> Fix it by parsing the tail of an error separate from the message,
> which also lets us add other sanity checking, and makes it easier if a
> future patch wants to capture instead of ignore the server's message.
> ---
> 
>   REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE:
> +  uint32_t length, msglen;
> +  uint16_t type;
> +
> +  switch (recv_into_rbuf (h)) {
> +  case -1: SET_NEXT_STATE (%.DEAD); return -1;
> +  case 0:
> +    length = be32toh (h->sbuf.sr.structured_reply.length);
> +    msglen = be16toh (h->sbuf.sr.payload.error.len);
> +    type = be16toh (h->sbuf.sr.structured_reply.type);
h->sbuf is a union...
> +
> +    length -= sizeof h->sbuf.sr.payload.error - msglen;
> +
> +    /* Special case two specific errors; ignore the tail for all others */
> +    h->rbuf = NULL;
> +    h->rlen = length;
> +    switch (type) {
> +    case NBD_REPLY_TYPE_ERROR:
> +      if (length != 0) {
> +        SET_NEXT_STATE (%.DEAD);
> +        set_error (0, "error payload length too large");
> +        return -1;
> +      }
> +      break;
> +    case NBD_REPLY_TYPE_ERROR_OFFSET:
> +      if (length != sizeof h->sbuf.offset) {
> +        SET_NEXT_STATE (%.DEAD);
> +        set_error (0, "error payload length too large");
> +        return -1;
> +      }
> +      h->rbuf = &h->sbuf.offset;
...placing offset directly in sbuf means that sbuf.sr is partially
invalidated...
> +      break;
> +    }
> +    SET_NEXT_STATE (%RECV_ERROR_TAIL);
> +  }
> +  return 0;
> +
> + REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL:
>    struct command_in_flight *cmd;
>    uint16_t flags;
>    uint64_t handle;
>    uint32_t error;
> +  uint64_t offset;
> 
>    switch (recv_into_rbuf (h)) {
>    case -1: SET_NEXT_STATE (%.DEAD); return -1;
  case 0:
    flags = be16toh (h->sbuf.sr.structured_reply.flags);
    handle = be64toh (h->sbuf.sr.structured_reply.handle);
    error = be32toh (h->sbuf.sr.payload.error.error);

which is not good for this.
> +++ b/lib/internal.h
> @@ -143,6 +143,7 @@ struct nbd_handle {
>      uint32_t len;
>      uint16_t nrinfos;
>      uint32_t nrqueries;
> +    uint64_t offset;
>    } sbuf;
So I'll need to move offset to a separate location other than sbuf.

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