[PATCH] server: Consolidate request validation
We had a couple of bugs when dealing with bogus requests from
malicious clients, when compared to what we document in the spec:
- NBD_CMD_TRIM failed with EINVAL instead of EPERM on a read-only image
- range validation for several commands was implicit based on syscall
failures, rather than being filtered out up front (which means we
are dependent on the right errno value, or even risk succeeding at
doing something out of range)
- At least in the case of NBD_CMD_TRIM, a req->from near 2^64 plus
req->len could overflow into something smaller than client->exportsize
- Duplicating checks everywhere is prone to bitrot
It's easier to consolidate validation up front, so we don't have
multiple copies of range checks, and so that we are more in line
with what the spec documents. This in turn requires updating
the testsuite for trim, now that the validation is done in a
different place.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
nbd-server.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-----------
nbdsrv.c | 11 +---------
tests/code/trim.c | 5 -----
3 files changed, 52 insertions(+), 27 deletions(-)
diff --git a/nbd-server.c b/nbd-server.c
index d861441..cc22222 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -2592,19 +2592,16 @@ static void handle_write(struct work_package *pkg)
DEBUG("handling write request\n");
setup_reply(&rep, req);
- if ((client->server->flags & F_READONLY) ||
- (client->server->flags & F_AUTOREADONLY)) {
- DEBUG("[WRITE to READONLY!]");
- rep.error = nbd_errno(EPERM);
#ifdef HAVE_SPLICE
- } else if (!pkg->data) {
+ if (!pkg->data) {
if (expsplice(pkg->pipefd[0], req->from, req->len, client,
SPLICE_OUT, fua)) {
DEBUG("Splice failed: %M");
rep.error = nbd_errno(errno);
}
+ } else
#endif
- } else {
+ {
if(expwrite(req->from, pkg->data, req->len, client, fua)) {
DEBUG("Write failed: %m");
rep.error = nbd_errno(errno);
@@ -2646,11 +2643,7 @@ static void handle_write_zeroes(CLIENT* client, struct nbd_request* req) {
DEBUG("handling write_zeroes request\n");
int fua = !!(req->type & NBD_CMD_FLAG_FUA);
setup_reply(&rep, req);
- if ((client->server->flags & F_READONLY) ||
- (client->server->flags & F_AUTOREADONLY)) {
- DEBUG("[WRITE to READONLY!]");
- rep.error = nbd_errno(EPERM);
- } else if(expwrite_zeroes(req, client, fua)) {
+ if(expwrite_zeroes(req, client, fua)) {
DEBUG("Write_zeroes failed: %m");
rep.error = nbd_errno(errno);
}
@@ -2662,11 +2655,31 @@ static void handle_write_zeroes(CLIENT* client, struct nbd_request* req) {
pthread_mutex_unlock(&(client->lock));
}
+
+static bool bad_write(CLIENT* client, struct nbd_request* req) {
+ if ((client->server->flags & F_READONLY) ||
+ (client->server->flags & F_AUTOREADONLY)) {
+ DEBUG("[WRITE to READONLY!]");
+ return true;
+ }
+ return false;
+}
+
+static bool bad_range(CLIENT* client, struct nbd_request* req) {
+ if(req->from > client->exportsize ||
+ req->from + req->len > client->exportsize) {
+ DEBUG("[out of bounds!]");
+ return true;
+ }
+ return false;
+}
+
static void handle_request(gpointer data, gpointer user_data) {
struct work_package* package = (struct work_package*) data;
uint32_t type = package->req->type & NBD_CMD_MASK_COMMAND;
uint32_t flags = package->req->type & ~NBD_CMD_MASK_COMMAND;
struct nbd_reply rep;
+ int err = EINVAL;
if(flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
msg(LOG_ERR, "E: received invalid flag %d on command %d, ignoring", flags, type);
@@ -2675,18 +2688,44 @@ static void handle_request(gpointer data, gpointer user_data) {
switch(type) {
case NBD_CMD_READ:
+ if (bad_range(package->client, package->req)) {
+ goto error;
+ }
handle_read(package->client, package->req);
break;
case NBD_CMD_WRITE:
+ if (bad_write(package->client, package->req)) {
+ err = EPERM;
+ goto error;
+ }
+ if (bad_range(package->client, package->req)) {
+ err = ENOSPC;
+ goto error;
+ }
handle_write(package);
break;
case NBD_CMD_FLUSH:
handle_flush(package->client, package->req);
break;
case NBD_CMD_TRIM:
+ if (bad_write(package->client, package->req)) {
+ err = EPERM;
+ goto error;
+ }
+ if (bad_range(package->client, package->req)) {
+ goto error;
+ }
handle_trim(package->client, package->req);
break;
case NBD_CMD_WRITE_ZEROES:
+ if (bad_write(package->client, package->req)) {
+ err = EPERM;
+ goto error;
+ }
+ if (bad_range(package->client, package->req)) {
+ err = ENOSPC;
+ goto error;
+ }
handle_write_zeroes(package->client, package->req);
break;
default:
@@ -2696,7 +2735,7 @@ static void handle_request(gpointer data, gpointer user_data) {
goto end;
error:
setup_reply(&rep, package->req);
- rep.error = nbd_errno(EINVAL);
+ rep.error = nbd_errno(err);
pthread_mutex_lock(&(package->client->lock));
socket_write(package->client, &rep, sizeof rep);
pthread_mutex_unlock(&(package->client->lock));
diff --git a/nbdsrv.c b/nbdsrv.c
index 7d351cf..c4e164a 100644
--- a/nbdsrv.c
+++ b/nbdsrv.c
@@ -236,16 +236,7 @@ uint64_t size_autodetect(int fhandle) {
}
int exptrim(struct nbd_request* req, CLIENT* client) {
- /* Don't trim when we're read only */
- if(client->server->flags & F_READONLY) {
- errno = EINVAL;
- return -1;
- }
- /* Don't trim beyond the size of the device, please */
- if(req->from + req->len > client->exportsize) {
- errno = EINVAL;
- return -1;
- }
+ /* Caller did range checking */
/* For copy-on-write, we should trim on the diff file. Not yet
* implemented. */
if(client->server->flags & F_COPYONWRITE) {
diff --git a/tests/code/trim.c b/tests/code/trim.c
index 11f5ddf..01588e0 100644
--- a/tests/code/trim.c
+++ b/tests/code/trim.c
@@ -71,10 +71,5 @@ int main(void) {
count_assert(g_off == 0);
count_assert(g_len == 1024*1024);
- req.from = 1024 * 1024 * 1024;
- req.len = 1024 * 1024;
- count_assert(exptrim(&req, &cl) == -1);
- count_assert(errno == EINVAL);
-
return 0;
}
--
2.13.6
Reply to: