[Nbd] [PATCH] server: Read client's length data before next option

[Eric Blake <eblake@...696...>]

Any client attempting to probe support for a new option, such as
NBD_OPT_STARTTLS or NBD_OPT_GO, with plans to do a graceful
fallback to older methods, will fail in its attempt if the server
does not ignore the length field and potential payload of the
unrecognized (or rejected) option, because the server will then
be reading out of sync and not see the client's magic for the
next option.  This bug has been latent in the reference
server since commit 626c2a3 in 2012, even though it is EXACTLY
the bug that NBD_FLAG_FIXED_NEWSTYLE was designed to prevent.

The only reason it has been buggy for so long is that it has
taken us this long to finally want to implement clients that use
a new option.

Signed-off-by: Eric Blake <eblake@...696...>
---

Perhaps you want to split this into two patches, one adding
consume_len() and fixing the default case that responds with
NBD_REP_ERR_UNSUP (with intent of backporting it to existing
releases, since they are all broken), and another that fixes
the STARTTLS stuff (since it is new and not in a release).

 nbd-server.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/nbd-server.c b/nbd-server.c
index 25c335b..7ae48c5 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -359,7 +359,7 @@ static void socket_read(CLIENT* client, void *buf, size_t len) {
 /**
  * Consume data from a socket that we don't want
  *
- * @param f a file descriptor
+ * @param c the client data stream
  * @param buf a buffer
  * @param len the number of bytes to consume
  * @param bufsiz the size of the buffer
@@ -373,6 +373,21 @@ static inline void consume(CLIENT* c, void * buf, size_t len, size_t bufsiz) {
 	}
 }

+/**
+ * Consume a length field and subsequent data from a socket that we don't want
+ *
+ * @param c the client data stream
+ **/
+static inline void consume_len(CLIENT* c) {
+	uint32_t len;
+	char buf[1024];
+
+	socket_read(c, &len, sizeof(len));
+	len = ntohl(len);
+	consume(c, buf, len, sizeof(buf));
+}
+
+
 static void socket_write(CLIENT* client, void *buf, size_t len) {
 	g_assert(client->socket_write != NULL);
 	client->socket_write(client, buf, len);
@@ -1491,6 +1506,11 @@ CLIENT* negotiate(int net, GArray* servers, const gchar* tlsdir) {
 				// so must do hard close
 				goto hard_close;
 			}
+			if(opt == NBD_OPT_ABORT) {
+				// handled below
+				break;
+			}
+			consume_len(client);
 			send_reply(client, opt, NBD_REP_ERR_TLS_REQD, -1, "TLS is required on this server");
 			continue;
 		}
@@ -1513,17 +1533,21 @@ CLIENT* negotiate(int net, GArray* servers, const gchar* tlsdir) {
 			break;
 		case NBD_OPT_STARTTLS:
 			if(client->tls_session != NULL) {
+				consume_len(client);
 				send_reply(client, opt, NBD_REP_ERR_INVALID, -1, "Invalid STARTTLS request: TLS has already been negotiated!");
 				continue;
 			}
 			if(tlsdir == NULL) {
+				consume_len(client);
 				send_reply(client, opt, NBD_REP_ERR_POLICY, -1, "TLS not allowed on this server");
+				continue;
 			}
 			if(handle_starttls(client, opt, servers, cflags, tlsdir) == NULL) {
 				// can't recover from failed TLS negotiation.
 				goto hard_close;
 			}
 		default:
+			consume_len(client);
 			send_reply(client, opt, NBD_REP_ERR_UNSUP, -1, "The given option is unknown to this server implementation");
 			break;
 		}
-- 
2.7.4