[Libguestfs] [libnbd PATCH] states: Validate error message size

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190614/79fc56d3/attachment.sig>