viced: cope with signed length/position in FetchData 23/15223/2
authorBenjamin Kaduk <kaduk@mit.edu>
Sat, 9 Oct 2021 03:11:19 +0000 (20:11 -0700)
committerBenjamin Kaduk <kaduk@mit.edu>
Thu, 15 Dec 2022 18:41:58 +0000 (13:41 -0500)
commit1fbbcbee0183aa7855c0e5d9d38aa89af75902db
tree195785ee2bb9ceb1c5868658fc79827b7ba24045
parente7737edb932a1c4d55a2551a44e481b40310c96d
viced: cope with signed length/position in FetchData

For legacy reasons, the "Pos" (initial position) and "Len" (length)
inputs to the RXAFS_FetchData and RXAFS_FetchData64 RPCs are represented
as signed integers (the corresponding StoreData RPCs use unsigned values).

The use of signed values allows for the possibility of negative inputs,
and of signed integer overflow (undefined behavior in C), though the latter
is unlikely to arise naturally given that the implementation uses a
common backend with 64-bit values.

In particular, if a negative "Pos" value is supplied, we end up in
FetchData_RXStyle() that performs either FDH_PREAD() or FDH_PREADV()
with the negative value as the position from which to read, which is
an error.  The error handling for those calls treats any error as
indicative of a problem with the volume or its underlying storage,
and takes the volume offline for salvage.  Furthermore, after the
maximum number of automatic salvages the volume is left offline for
administrator action.  This presents a simple route for
(unauthenticated) denial of service, as root.cell.readonly must be
available to all users of the cell, and can be brought offline in this
way; rendering root.cell.readonly unavailable would bring essentially
all access to the cell to a halt.  (Other volumes could be targeted as
well, subject to their corresponding ACLs.)

Since there is no valid use for a negative position or length input,
reject them outright from the common_FetchData64() implementation.
Also check for whether the combination requests a read that would
overflow a signed integer and reject that as well.

Thanks to Jeffrey Altman and Chaskiel Grundman for collaborating on
this change.

FIXES 135263

Change-Id: I99c4c264c846b01fbbf4fecc60b1f34504bcc977
Reviewed-on: https://gerrit.openafs.org/15223
Reviewed-by: Stephan Wiesand <stephan.wiesand@desy.de>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
src/viced/afsfileprocs.c