Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
Am 09.03.2020 um 16:44 hat Daniel P. Berrangé geschrieben: > We could support "-F ..." and validate any non-raw formats, while raising a > runtime error in the case of "-F raw", as only the "raw" backing format has > the probing security risk. > > Users who need to use qcow, with a backing file, without a format can > just not pass "-F" and in doing so will be insecure. Hm, this is actually an interesting option. We wouldn't lose features compared to today without -F, but we would allow -F when we can verify that the operation is safe (the image is already non-raw). > We could take this opportunity to deprecate 'qcow' perhaps, declare it > a read-only format, restricted to qemu-img/qemu-io for purpose of data > liberation ? I'm against making any format read-only because that immediately means that it becomes untestable. > For sheepdog, if it is something we genuinely still care about, then > adding a backing file format record seems neccessary, unless we either > forbid use of raw backing files, or forbid use of non-raw backing files, > either way would be safe. In case of doubt, we can use the same logic as you suggested for qcow (accept only non-raw with -F, but no restrictions without -F). Kevin -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On 3/9/20 10:48 AM, Kevin Wolf wrote: Still, the point of this patch is that I want to add -F into all the iotests, and without something along the lines of this patch, all of those iotests are broken for these image formats. Patch 2 is a lot harder to write if we have to make our use of -F conditional on the image format in question. Hm... Maybe _make_test_img can insert/filter out -F depending on $IMGFMT? I was hoping to avoid that, but yes, if that's what we have to do... :( The complication is that even if I filter out -F from the command line based on $IMGFMT, then I have conditional output (whether backing_fmt= or the warning message is output), which means doubling the number of expected output cases, or else adding a multi-line filter which is also smart enough based on $IMGFMT to translate a warning on one line into an addition of a faked backing_fmt= on the next line. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On 3/9/20 10:36 AM, Daniel P. Berrangé wrote: On Mon, Mar 09, 2020 at 04:21:12PM +0100, Kevin Wolf wrote: Am 06.03.2020 um 23:51 hat Eric Blake geschrieben: For qcow2 and qed, we want to encourage the use of -F always, as these formats can suffer from data corruption or security holes if backing format is probed. But for other formats, the backing format cannot be recorded. Making the user decide on a per-format basis whether to supply a backing format string is awkward, better is to just blindly accept a backing format argument even if it is ignored by the contraints of the format at hand. Signed-off-by: Eric Blake I'm not sure if I agree with this reasoning. Accepting and silently ignoring -F could give users a false sense of security. If I specify a -F raw and QEMU later probes qcow2, that would be very surprising. And if the user specifies "-F raw" and we probe qcow2, and the user does not realize this, they can become silently reliant on always probing qcow2. If we then honour the "-F raw" option in a later QEMU release, we'll break the behaviour they've relied on. IMHO, we must not accept "-F fmt" unless we're in a position to honour it. So I'm thinking: qemu-img create -f qcow -b backing.qcow -F qcow img.qcow => okay qemu-img create -f qcow -b backing.raw -F raw img.qcow => okay, slightly risky (if backing.raw is ever changed to be non-raw), but then again, backing files tend to be read-only (do we even support commit on qcow images, or do we limit that to qcow2?) qemu-img create -f qcow -b backing.qcow -F raw img.qcow => fails, due to mismatch qemu-img create -u -f qcow -b anything -F anything img.qcow $size => fails: we can't write -F into the image, nor can we open anything to probe its type to check that -F was correct qemu-img create -f qcow -b backing.qcow img.qcow => warns, but okay (we did not get -F, but the probe works out) qemu-img create -f qcow -b backing.raw img.qcow=> likewise warns qemu-img create -f qcow -b backing.qcow2 img.qcow => error; new qcow images (which you should avoid where possible anyways) must be backed by only raw or qcow, going forward Other scenarios? Do the above ideas look reasonable? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On 3/9/20 10:44 AM, Daniel P. Berrangé wrote: Consider the user creates an image with "-F raw". We can validate the backing image is raw, and so our check succeeds. Later the malicious can write a qcow header into this raw file and QEMU will thereafter probe the image as qcow, not raw. IOW, in the case of "-F raw", even if we immediately check the format, we're still not offering the protection promised by the "-F" flag, because that promise refers to the runtime behaviour of the QEMU emulator, not the immediate qemu-img cmd. We could support "-F ..." and validate any non-raw formats, while raising a runtime error in the case of "-F raw", as only the "raw" backing format has the probing security risk. Users who need to use qcow, with a backing file, without a format can just not pass "-F" and in doing so will be insecure. And the warning will remind them of that. We could take this opportunity to deprecate 'qcow' perhaps, declare it a read-only format, restricted to qemu-img/qemu-io for purpose of data liberation ? I'm fine with that, although it makes for a bigger task. For sheepdog, if it is something we genuinely still care about, then adding a backing file format record seems neccessary, unless we either forbid use of raw backing files, or forbid use of non-raw backing files, either way would be safe. I concur - as long as you either have ONLY non-raw (in which case probing is safe), or ONLY raw (in which case no probing is necessary), then not recording the backing format is safe. It is only for formats that allow both raw and non-raw backing, but which do not have space in the image to document which of the two backing formats is expected, where we have problems. I'm guessing that qcow works with either raw or qcow as backing format (and anything else is odd - a qcow2 backing to a qcow is unusual, and would be better to reject). I'm not sure if sheepdog can be backed by anything but another sheepdog, similarly, I'm not sure if a vmdk can be backed by anything but another vmdk. If so, it should be simple enough to do a v4 of this patch which requires -F to be a known-acceptable probe type for these images. Still, the point of this patch is that I want to add -F into all the iotests, and without something along the lines of this patch, all of those iotests are broken for these image formats. Patch 2 is a lot harder to write if we have to make our use of -F conditional on the image format in question. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org Regards, Daniel -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
Am 09.03.2020 um 16:32 hat Eric Blake geschrieben: > On 3/9/20 10:21 AM, Kevin Wolf wrote: > > Am 06.03.2020 um 23:51 hat Eric Blake geschrieben: > > > For qcow2 and qed, we want to encourage the use of -F always, as these > > > formats can suffer from data corruption or security holes if backing > > > format is probed. But for other formats, the backing format cannot be > > > recorded. Making the user decide on a per-format basis whether to > > > supply a backing format string is awkward, better is to just blindly > > > accept a backing format argument even if it is ignored by the > > > contraints of the format at hand. > > > > > > Signed-off-by: Eric Blake > > > > I'm not sure if I agree with this reasoning. Accepting and silently > > ignoring -F could give users a false sense of security. If I specify a > > -F raw and QEMU later probes qcow2, that would be very surprising. > > Do we know what formats qcow, sheepdog, and vmdk expect to probe? I'm > wondering if we can compromise by checking that the requested backing image > has the specified format, and error if it is not, rather than completely > ignoring it - but at the same time, the image formats have no where to > record a backing format. The important distinction (and in fact the only one that qed makes) is raw and non-raw. Problems only arise if a guest can write an image header to a raw file and get it probed as non-raw when opening the image the next time. If you start with a non-raw format, at least the first 512 bytes (which are used for probing) are used for metadata and not accessible for the guest. > I'm guessing that qcow works with either raw or qcow as backing format (and > anything else is odd - a qcow2 backing to a qcow is unusual, and would be > better to reject). I'm not sure if sheepdog can be backed by anything but > another sheepdog, similarly, I'm not sure if a vmdk can be backed by > anything but another vmdk. I think vmdk only expects vmdk as backing files, even though QEMU supports everything else, too. However, this is a format for compatibility with another hypervisor and you're unlikely to find QEMU-only VMDK images, so requiring non-raw unconditionally might make sense. I have no idea about how backing files in Sheepdog are used in practice. However, QEMU is a primary target for Sheepdog. It wouldn't surprise me if it's used for both raw and non-raw. qcow is definitely used for both, as you already said. Allowing only raw and qcow and forbidding other formats doesn't improve the situation because the problem is with supporting raw and non-raw at the same time and you would still have this. > > If so, it should be simple enough to do a v4 of > this patch which requires -F to be a known-acceptable probe type for these > images. > > Still, the point of this patch is that I want to add -F into all the > iotests, and without something along the lines of this patch, all of those > iotests are broken for these image formats. Patch 2 is a lot harder to > write if we have to make our use of -F conditional on the image format in > question. Hm... Maybe _make_test_img can insert/filter out -F depending on $IMGFMT? Kevin -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On Mon, Mar 09, 2020 at 10:32:52AM -0500, Eric Blake wrote: > On 3/9/20 10:21 AM, Kevin Wolf wrote: > > Am 06.03.2020 um 23:51 hat Eric Blake geschrieben: > > > For qcow2 and qed, we want to encourage the use of -F always, as these > > > formats can suffer from data corruption or security holes if backing > > > format is probed. But for other formats, the backing format cannot be > > > recorded. Making the user decide on a per-format basis whether to > > > supply a backing format string is awkward, better is to just blindly > > > accept a backing format argument even if it is ignored by the > > > contraints of the format at hand. > > > > > > Signed-off-by: Eric Blake > > > > I'm not sure if I agree with this reasoning. Accepting and silently > > ignoring -F could give users a false sense of security. If I specify a > > -F raw and QEMU later probes qcow2, that would be very surprising. > > Do we know what formats qcow, sheepdog, and vmdk expect to probe? I'm > wondering if we can compromise by checking that the requested backing image > has the specified format, and error if it is not, rather than completely > ignoring it - but at the same time, the image formats have no where to > record a backing format. Consider the user creates an image with "-F raw". We can validate the backing image is raw, and so our check succeeds. Later the malicious can write a qcow header into this raw file and QEMU will thereafter probe the image as qcow, not raw. IOW, in the case of "-F raw", even if we immediately check the format, we're still not offering the protection promised by the "-F" flag, because that promise refers to the runtime behaviour of the QEMU emulator, not the immediate qemu-img cmd. We could support "-F ..." and validate any non-raw formats, while raising a runtime error in the case of "-F raw", as only the "raw" backing format has the probing security risk. Users who need to use qcow, with a backing file, without a format can just not pass "-F" and in doing so will be insecure. We could take this opportunity to deprecate 'qcow' perhaps, declare it a read-only format, restricted to qemu-img/qemu-io for purpose of data liberation ? For sheepdog, if it is something we genuinely still care about, then adding a backing file format record seems neccessary, unless we either forbid use of raw backing files, or forbid use of non-raw backing files, either way would be safe. > I'm guessing that qcow works with either raw or qcow as backing format (and > anything else is odd - a qcow2 backing to a qcow is unusual, and would be > better to reject). I'm not sure if sheepdog can be backed by anything but > another sheepdog, similarly, I'm not sure if a vmdk can be backed by > anything but another vmdk. If so, it should be simple enough to do a v4 of > this patch which requires -F to be a known-acceptable probe type for these > images. > > Still, the point of this patch is that I want to add -F into all the > iotests, and without something along the lines of this patch, all of those > iotests are broken for these image formats. Patch 2 is a lot harder to > write if we have to make our use of -F conditional on the image format in > question. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On 3/9/20 10:21 AM, Kevin Wolf wrote: Am 06.03.2020 um 23:51 hat Eric Blake geschrieben: For qcow2 and qed, we want to encourage the use of -F always, as these formats can suffer from data corruption or security holes if backing format is probed. But for other formats, the backing format cannot be recorded. Making the user decide on a per-format basis whether to supply a backing format string is awkward, better is to just blindly accept a backing format argument even if it is ignored by the contraints of the format at hand. Signed-off-by: Eric Blake I'm not sure if I agree with this reasoning. Accepting and silently ignoring -F could give users a false sense of security. If I specify a -F raw and QEMU later probes qcow2, that would be very surprising. Do we know what formats qcow, sheepdog, and vmdk expect to probe? I'm wondering if we can compromise by checking that the requested backing image has the specified format, and error if it is not, rather than completely ignoring it - but at the same time, the image formats have no where to record a backing format. I'm guessing that qcow works with either raw or qcow as backing format (and anything else is odd - a qcow2 backing to a qcow is unusual, and would be better to reject). I'm not sure if sheepdog can be backed by anything but another sheepdog, similarly, I'm not sure if a vmdk can be backed by anything but another vmdk. If so, it should be simple enough to do a v4 of this patch which requires -F to be a known-acceptable probe type for these images. Still, the point of this patch is that I want to add -F into all the iotests, and without something along the lines of this patch, all of those iotests are broken for these image formats. Patch 2 is a lot harder to write if we have to make our use of -F conditional on the image format in question. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On Mon, Mar 09, 2020 at 04:21:12PM +0100, Kevin Wolf wrote: > Am 06.03.2020 um 23:51 hat Eric Blake geschrieben: > > For qcow2 and qed, we want to encourage the use of -F always, as these > > formats can suffer from data corruption or security holes if backing > > format is probed. But for other formats, the backing format cannot be > > recorded. Making the user decide on a per-format basis whether to > > supply a backing format string is awkward, better is to just blindly > > accept a backing format argument even if it is ignored by the > > contraints of the format at hand. > > > > Signed-off-by: Eric Blake > > I'm not sure if I agree with this reasoning. Accepting and silently > ignoring -F could give users a false sense of security. If I specify a > -F raw and QEMU later probes qcow2, that would be very surprising. And if the user specifies "-F raw" and we probe qcow2, and the user does not realize this, they can become silently reliant on always probing qcow2. If we then honour the "-F raw" option in a later QEMU release, we'll break the behaviour they've relied on. IMHO, we must not accept "-F fmt" unless we're in a position to honour it. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
Re: [sheepdog] [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
Am 06.03.2020 um 23:51 hat Eric Blake geschrieben: > For qcow2 and qed, we want to encourage the use of -F always, as these > formats can suffer from data corruption or security holes if backing > format is probed. But for other formats, the backing format cannot be > recorded. Making the user decide on a per-format basis whether to > supply a backing format string is awkward, better is to just blindly > accept a backing format argument even if it is ignored by the > contraints of the format at hand. > > Signed-off-by: Eric Blake I'm not sure if I agree with this reasoning. Accepting and silently ignoring -F could give users a false sense of security. If I specify a -F raw and QEMU later probes qcow2, that would be very surprising. Kevin -- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
