| Message ID | 20190112175812.27068-4-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
12.01.2019 20:57, Eric Blake wrote: > When the user requests a partition, we were using data read > from the disk as disk offsets without a bounds check. We got > lucky that even when computed offsets are out-of-bounds, > blk_pread() will gracefully catch the error later (so I don't > think a malicious image can crash or exploit qemu-nbd, and am > not treating this as a security flaw), but it's better to > flag the problem up front than to risk permanent EIO death of > the block device down the road. Also, note that the > partition code blindly overwrites any offset passed in by the > user; so make the -o/-P combo an error for less confusion. > > This can be tested with nbdkit: > $ echo hi > file > $ nbdkit -fv --filter=truncate partitioning file truncate=64k > > Pre-patch: > $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 & > $ qemu-io -f raw nbd://localhost:10810 > qemu-io> r -v 0 1 > Disconnect client, due to: Failed to send reply: reading from file failed: Input/output error > Connection closed > read failed: Input/output error > qemu-io> q > [1]+ Done qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 > > Post-patch: > $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 > qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size exceeds file length 65536 > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > v3: new patch > --- > qemu-nbd.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 51b55f2e066..ff4adb9b3eb 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -1013,12 +1013,28 @@ int main(int argc, char **argv) > fd_size -= dev_offset; > > if (partition != -1) { > - ret = find_partition(blk, partition, &dev_offset, &fd_size); > + off_t limit; > + > + if (dev_offset) { > + error_report("Cannot request partition and offset together"); hmm, but you still allow to request partition and set -o 0, I think it's better to forbid it too. > + exit(EXIT_FAILURE); > + } > + ret = find_partition(blk, partition, &dev_offset, &limit); > if (ret < 0) { > error_report("Could not find partition %d: %s", partition, > strerror(-ret)); > exit(EXIT_FAILURE); > } > + /* partition limits are (32-bit << 9); can't overflow 64 bits */ > + assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); hmm, so these values are read from file and may be whatsoever. Why to assert instead of error_report and exit() ? > + if (dev_offset + limit > fd_size) { > + error_report("Discovered partition %d at offset %lld size %lld, " > + "but size exceeds file length %lld", partition, > + (long long int) dev_offset, (long long int) limit, > + (long long int) fd_size); > + exit(EXIT_FAILURE); > + } > + fd_size = limit; > } > > export = nbd_export_new(bs, dev_offset, fd_size, export_name, >
On 1/15/19 10:20 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.01.2019 20:57, Eric Blake wrote: >> When the user requests a partition, we were using data read >> from the disk as disk offsets without a bounds check. We got >> lucky that even when computed offsets are out-of-bounds, >> blk_pread() will gracefully catch the error later (so I don't >> think a malicious image can crash or exploit qemu-nbd, and am >> not treating this as a security flaw), but it's better to >> flag the problem up front than to risk permanent EIO death of >> the block device down the road. Also, note that the >> partition code blindly overwrites any offset passed in by the >> user; so make the -o/-P combo an error for less confusion. >> >> This can be tested with nbdkit: >> $ echo hi > file >> $ nbdkit -fv --filter=truncate partitioning file truncate=64k >> >> Pre-patch: >> $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 & >> $ qemu-io -f raw nbd://localhost:10810 >> qemu-io> r -v 0 1 >> Disconnect client, due to: Failed to send reply: reading from file failed: Input/output error >> Connection closed >> read failed: Input/output error >> qemu-io> q >> [1]+ Done qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 >> >> Post-patch: >> $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 >> qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size exceeds file length 65536 >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> v3: new patch >> --- >> qemu-nbd.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-nbd.c b/qemu-nbd.c >> index 51b55f2e066..ff4adb9b3eb 100644 >> --- a/qemu-nbd.c >> +++ b/qemu-nbd.c >> @@ -1013,12 +1013,28 @@ int main(int argc, char **argv) >> fd_size -= dev_offset; >> >> if (partition != -1) { >> - ret = find_partition(blk, partition, &dev_offset, &fd_size); >> + off_t limit; >> + >> + if (dev_offset) { >> + error_report("Cannot request partition and offset together"); > > hmm, but you still allow to request partition and set -o 0, I think it's better > to forbid it too. Not worth the bother. Someday, we may want to permit -o and -P together, where we first calculate the bounds of the partition from -P, and then use -o to further restrict an even smaller subset of the image exposed to the user. Under that interpretation, the added error makes sense as a short-term stop-gap that prevents us from doing what we want (since we did not actually treat a non-zero offset as an offset within the partition), but an offset of 0 is the same as omitting the offset. So I didn't see the point to complicate the code just to check whether -o had been explicitly supplied with its already-default value. > >> + exit(EXIT_FAILURE); >> + } >> + ret = find_partition(blk, partition, &dev_offset, &limit); >> if (ret < 0) { >> error_report("Could not find partition %d: %s", partition, >> strerror(-ret)); >> exit(EXIT_FAILURE); >> } >> + /* partition limits are (32-bit << 9); can't overflow 64 bits */ >> + assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); > > hmm, so these values are read from file and may be whatsoever. Why to assert instead > of error_report and exit() ? The assertion is letting the compiler know that we can't get a negative value. (off_t)(uint32_t << 9)*2 is always a positive value for 64-bit off_t, but the compiler doesn't necessarily know that ((off_t)a + (off_t)b >= a) is going to be true if it doesn't track the range limitations imposed by find_partition(). You are right that we are doing an assertion based on data obtained by reading from potentially malicious data, but the assertion is safe because it will NEVER fail, but is rather augmenting the compiler's data-type analysis.
On Sat, Jan 12, 2019 at 11:57:56AM -0600, Eric Blake wrote: > When the user requests a partition, we were using data read > from the disk as disk offsets without a bounds check. We got > lucky that even when computed offsets are out-of-bounds, > blk_pread() will gracefully catch the error later (so I don't > think a malicious image can crash or exploit qemu-nbd, and am > not treating this as a security flaw), but it's better to > flag the problem up front than to risk permanent EIO death of > the block device down the road. Also, note that the > partition code blindly overwrites any offset passed in by the > user; so make the -o/-P combo an error for less confusion. > > This can be tested with nbdkit: > $ echo hi > file > $ nbdkit -fv --filter=truncate partitioning file truncate=64k > > Pre-patch: > $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 & > $ qemu-io -f raw nbd://localhost:10810 > qemu-io> r -v 0 1 > Disconnect client, due to: Failed to send reply: reading from file failed: Input/output error > Connection closed > read failed: Input/output error > qemu-io> q > [1]+ Done qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 > > Post-patch: > $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 > qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size exceeds file length 65536 > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > v3: new patch > --- > qemu-nbd.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 51b55f2e066..ff4adb9b3eb 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -1013,12 +1013,28 @@ int main(int argc, char **argv) > fd_size -= dev_offset; > > if (partition != -1) { > - ret = find_partition(blk, partition, &dev_offset, &fd_size); > + off_t limit; I was only vaguely following the other review comments, but off_t does seem odd here. Even though we can assume that _FILE_OFFSET_BITS=64 maybe we should just use {u,}int64_t? Does this represent an offset in a host file? Only in the case where qemu-nbd is serving a raw format file. In other cases (say, qcow2) the partition size exists in an abstract virtual space. > + if (dev_offset) { > + error_report("Cannot request partition and offset together"); > + exit(EXIT_FAILURE); > + } > + ret = find_partition(blk, partition, &dev_offset, &limit); > if (ret < 0) { > error_report("Could not find partition %d: %s", partition, > strerror(-ret)); > exit(EXIT_FAILURE); > } > + /* partition limits are (32-bit << 9); can't overflow 64 bits */ > + assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); > + if (dev_offset + limit > fd_size) { > + error_report("Discovered partition %d at offset %lld size %lld, " > + "but size exceeds file length %lld", partition, > + (long long int) dev_offset, (long long int) limit, > + (long long int) fd_size); > + exit(EXIT_FAILURE); > + } > + fd_size = limit; > } > > export = nbd_export_new(bs, dev_offset, fd_size, export_name, But it's not a big deal, so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich.
On 1/15/19 12:00 PM, Richard W.M. Jones wrote: > On Sat, Jan 12, 2019 at 11:57:56AM -0600, Eric Blake wrote: >> When the user requests a partition, we were using data read >> from the disk as disk offsets without a bounds check. We got >> lucky that even when computed offsets are out-of-bounds, >> blk_pread() will gracefully catch the error later (so I don't >> think a malicious image can crash or exploit qemu-nbd, and am >> not treating this as a security flaw), but it's better to >> flag the problem up front than to risk permanent EIO death of >> the block device down the road. Also, note that the >> partition code blindly overwrites any offset passed in by the >> user; so make the -o/-P combo an error for less confusion. >> >> This can be tested with nbdkit: >> $ echo hi > file >> $ nbdkit -fv --filter=truncate partitioning file truncate=64k >> >> Pre-patch: >> $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 & >> $ qemu-io -f raw nbd://localhost:10810 >> qemu-io> r -v 0 1 >> Disconnect client, due to: Failed to send reply: reading from file failed: Input/output error >> Connection closed >> read failed: Input/output error >> qemu-io> q >> [1]+ Done qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 >> >> Post-patch: >> $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 >> qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size exceeds file length 65536 >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> v3: new patch >> --- >> qemu-nbd.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-nbd.c b/qemu-nbd.c >> index 51b55f2e066..ff4adb9b3eb 100644 >> --- a/qemu-nbd.c >> +++ b/qemu-nbd.c >> @@ -1013,12 +1013,28 @@ int main(int argc, char **argv) >> fd_size -= dev_offset; >> >> if (partition != -1) { >> - ret = find_partition(blk, partition, &dev_offset, &fd_size); >> + off_t limit; > > I was only vaguely following the other review comments, but off_t does > seem odd here. Even though we can assume that _FILE_OFFSET_BITS=64 > maybe we should just use {u,}int64_t? Does this represent an offset > in a host file? Only in the case where qemu-nbd is serving a raw > format file. In other cases (say, qcow2) the partition size exists in > an abstract virtual space. Yes, later patches switch it to int64_t. Here, it remains off_t because find_partition(&limit) still expects off_t. I suppose in my later patches, I could use uint64_t limit in spite of keeping int64_t fd_size, and change the signature of find_partition() accordingly, since I've decoupled the MBR partition lookup (which is a 41-bit value, always positive) from the file size checks. > > But it's not a big deal, so: Yeah, no need to rewrite this patch, since later patches improve the type anyway (whether or not I stick to int64_t or uint64_t for the find_partition() code). > > Reviewed-by: Richard W.M. Jones <rjones@redhat.com> > > Rich. >
12.01.2019 20:57, Eric Blake wrote: > When the user requests a partition, we were using data read > from the disk as disk offsets without a bounds check. We got > lucky that even when computed offsets are out-of-bounds, > blk_pread() will gracefully catch the error later (so I don't > think a malicious image can crash or exploit qemu-nbd, and am > not treating this as a security flaw), but it's better to > flag the problem up front than to risk permanent EIO death of > the block device down the road. Also, note that the > partition code blindly overwrites any offset passed in by the > user; so make the -o/-P combo an error for less confusion. > > This can be tested with nbdkit: > $ echo hi > file > $ nbdkit -fv --filter=truncate partitioning file truncate=64k > > Pre-patch: > $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 & > $ qemu-io -f raw nbd://localhost:10810 > qemu-io> r -v 0 1 > Disconnect client, due to: Failed to send reply: reading from file failed: Input/output error > Connection closed > read failed: Input/output error > qemu-io> q > [1]+ Done qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 > > Post-patch: > $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 > qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size exceeds file length 65536 > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > v3: new patch > --- > qemu-nbd.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 51b55f2e066..ff4adb9b3eb 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -1013,12 +1013,28 @@ int main(int argc, char **argv) > fd_size -= dev_offset; This reuse of file-size variable as export-size is a source of errors, like your assert in the following part. > > if (partition != -1) { > - ret = find_partition(blk, partition, &dev_offset, &fd_size); > + off_t limit; > + > + if (dev_offset) { > + error_report("Cannot request partition and offset together"); > + exit(EXIT_FAILURE); > + } > + ret = find_partition(blk, partition, &dev_offset, &limit); > if (ret < 0) { > error_report("Could not find partition %d: %s", partition, > strerror(-ret)); > exit(EXIT_FAILURE); > } > + /* partition limits are (32-bit << 9); can't overflow 64 bits */ > + assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); > + if (dev_offset + limit > fd_size) { > + error_report("Discovered partition %d at offset %lld size %lld, " > + "but size exceeds file length %lld", partition, > + (long long int) dev_offset, (long long int) limit, > + (long long int) fd_size); > + exit(EXIT_FAILURE); > + } > + fd_size = limit; > } > > export = nbd_export_new(bs, dev_offset, fd_size, export_name, > Ok, anyway, finally I understand the point, thank you for detailed explanation: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/qemu-nbd.c b/qemu-nbd.c index 51b55f2e066..ff4adb9b3eb 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -1013,12 +1013,28 @@ int main(int argc, char **argv) fd_size -= dev_offset; if (partition != -1) { - ret = find_partition(blk, partition, &dev_offset, &fd_size); + off_t limit; + + if (dev_offset) { + error_report("Cannot request partition and offset together"); + exit(EXIT_FAILURE); + } + ret = find_partition(blk, partition, &dev_offset, &limit); if (ret < 0) { error_report("Could not find partition %d: %s", partition, strerror(-ret)); exit(EXIT_FAILURE); } + /* partition limits are (32-bit << 9); can't overflow 64 bits */ + assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); + if (dev_offset + limit > fd_size) { + error_report("Discovered partition %d at offset %lld size %lld, " + "but size exceeds file length %lld", partition, + (long long int) dev_offset, (long long int) limit, + (long long int) fd_size); + exit(EXIT_FAILURE); + } + fd_size = limit; } export = nbd_export_new(bs, dev_offset, fd_size, export_name,
When the user requests a partition, we were using data read from the disk as disk offsets without a bounds check. We got lucky that even when computed offsets are out-of-bounds, blk_pread() will gracefully catch the error later (so I don't think a malicious image can crash or exploit qemu-nbd, and am not treating this as a security flaw), but it's better to flag the problem up front than to risk permanent EIO death of the block device down the road. Also, note that the partition code blindly overwrites any offset passed in by the user; so make the -o/-P combo an error for less confusion. This can be tested with nbdkit: $ echo hi > file $ nbdkit -fv --filter=truncate partitioning file truncate=64k Pre-patch: $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 & $ qemu-io -f raw nbd://localhost:10810 qemu-io> r -v 0 1 Disconnect client, due to: Failed to send reply: reading from file failed: Input/output error Connection closed read failed: Input/output error qemu-io> q [1]+ Done qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 Post-patch: $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size exceeds file length 65536 Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: new patch --- qemu-nbd.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)