Re: [Libguestfs] [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. ---

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;

+++ b/lib/internal.h @@ -143,6 +143,7 @@ struct nbd_handle { uint32_t len; uint16_t nrinfos; uint32_t nrqueries; + uint64_t offset; } sbuf;