Re: [PATCH] server: Consolidate request validation

[Wouter Verhelst <w@uter.be>]

Hi Eric,

On Wed, Nov 15, 2017 at 03:08:05PM -0600, Eric Blake wrote:
> We had a couple of bugs when dealing with bogus requests from
> malicious clients,

You had me going here for a while. Upon seeing "malicious", my mind
screamed "security issue". Good to know it isn't that :-)

> 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

All good points, yes.

> 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>
[...]
> --- 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 */

Might be good to have an assert here, still, so that future changes
don't break things?

Not critical though.

>  	/* 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);
> -

Sightly sad that this reduces our test coverage, but I don't see a
better way (other than moving more stuff to nbdsrv.c, which is my
long-term goal anyway, but not something that I think I should ask for
such a minor change).

-- 
Could you people please use IRC like normal people?!?

  -- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008
     Hacklab