| Message ID | 20120816145836.GA6793@hq.k1024.org |
|---|---|
| State | New |
| Headers | show |
On 16.08.2012 18:58, Iustin Pop wrote: > Commit 947995c (block: protect path_has_protocol from filenames with > colons) introduced a way to handle filenames with colons based on > whether the path contains a slash or not. IMHO this is not optimal, > since we shouldn't rely on the contents of the path but rather on > whether the given path exists as a file or not. > > As such, this patch tries to handle both files with and without > slashes by falling back to opening them as files if no drivers > supporting the protocol has been identified. I for one dislike this idea entirely: I think there should be a way to stop qemu from trying to open something as a file. It opens a security hole after all, "what if" such a file will actually exist? If I can vote, I'm voting against this with both hands. Thanks, /mjt
On Thu, Aug 16, 2012 at 11:24:11PM +0400, Michael Tokarev wrote: > On 16.08.2012 18:58, Iustin Pop wrote: > > Commit 947995c (block: protect path_has_protocol from filenames with > > colons) introduced a way to handle filenames with colons based on > > whether the path contains a slash or not. IMHO this is not optimal, > > since we shouldn't rely on the contents of the path but rather on > > whether the given path exists as a file or not. > > > > As such, this patch tries to handle both files with and without > > slashes by falling back to opening them as files if no drivers > > supporting the protocol has been identified. > > I for one dislike this idea entirely: I think there should be a > way to stop qemu from trying to open something as a file. It > opens a security hole after all, "what if" such a file will actually > exist? I'm not sure I understand the concern here. You pass what is a file path (and not an existing protocol path), and you want qemu not to open it? Or are you worried that a typo in the protocol name can lead to attacks? > If I can vote, I'm voting against this with both hands. It's fine to have a way to stop QEMU opening something as a file, but please tell me how I can make it so that "qemu -hda x:0" works for both regular files and block/char devices. Right now, it behaves differently for these two, and from the code it looks like this difference is rather accidental than intentional. regards, iustin
Am 17.08.2012 09:15, schrieb Iustin Pop: > On Thu, Aug 16, 2012 at 11:24:11PM +0400, Michael Tokarev wrote: >> On 16.08.2012 18:58, Iustin Pop wrote: >>> Commit 947995c (block: protect path_has_protocol from filenames with >>> colons) introduced a way to handle filenames with colons based on >>> whether the path contains a slash or not. IMHO this is not optimal, >>> since we shouldn't rely on the contents of the path but rather on >>> whether the given path exists as a file or not. >>> >>> As such, this patch tries to handle both files with and without >>> slashes by falling back to opening them as files if no drivers >>> supporting the protocol has been identified. >> >> I for one dislike this idea entirely: I think there should be a >> way to stop qemu from trying to open something as a file. It >> opens a security hole after all, "what if" such a file will actually >> exist? > > I'm not sure I understand the concern here. You pass what is a file path > (and not an existing protocol path), and you want qemu not to open it? > Or are you worried that a typo in the protocol name can lead to attacks? That, or just the fact that you don't know exactly which protocols are supported by this qemu binary and expect one that isn't there. The other concern I have is about the opposite direction: When a new qemu version introduces a new protocol, the same string that meant a file before would start meaning the protocol, which makes it an incompatible change. Essentially, that would mean that we can't add any new protocols any more. Doesn't sound like a great idea to me. >> If I can vote, I'm voting against this with both hands. > > It's fine to have a way to stop QEMU opening something as a file, but > please tell me how I can make it so that "qemu -hda x:0" works for both > regular files and block/char devices. Right now, it behaves differently > for these two, and from the code it looks like this difference is rather > accidental than intentional. It was probably accidental in the beginning, but it's now a well-known misfeature that we can't get rid of for compatibility reasons. People rely on devices with colons being accessible this way. (We once changed that, but had to take this part back) Kevin
On Fri, Aug 17, 2012 at 09:56:35AM +0200, Kevin Wolf wrote: > Am 17.08.2012 09:15, schrieb Iustin Pop: > > On Thu, Aug 16, 2012 at 11:24:11PM +0400, Michael Tokarev wrote: > >> On 16.08.2012 18:58, Iustin Pop wrote: > >>> Commit 947995c (block: protect path_has_protocol from filenames with > >>> colons) introduced a way to handle filenames with colons based on > >>> whether the path contains a slash or not. IMHO this is not optimal, > >>> since we shouldn't rely on the contents of the path but rather on > >>> whether the given path exists as a file or not. > >>> > >>> As such, this patch tries to handle both files with and without > >>> slashes by falling back to opening them as files if no drivers > >>> supporting the protocol has been identified. > >> > >> I for one dislike this idea entirely: I think there should be a > >> way to stop qemu from trying to open something as a file. It > >> opens a security hole after all, "what if" such a file will actually > >> exist? > > > > I'm not sure I understand the concern here. You pass what is a file path > > (and not an existing protocol path), and you want qemu not to open it? > > Or are you worried that a typo in the protocol name can lead to attacks? > > That, or just the fact that you don't know exactly which protocols are > supported by this qemu binary and expect one that isn't there. > > The other concern I have is about the opposite direction: When a new > qemu version introduces a new protocol, the same string that meant a > file before would start meaning the protocol, which makes it an > incompatible change. Essentially, that would mean that we can't add any > new protocols any more. Doesn't sound like a great idea to me. So in this sense, you prefer to have it remain so that files with colons are not accepted by qemu, if I understand you correctly, so that new procotol can be added, right? > >> If I can vote, I'm voting against this with both hands. > > > > It's fine to have a way to stop QEMU opening something as a file, but > > please tell me how I can make it so that "qemu -hda x:0" works for both > > regular files and block/char devices. Right now, it behaves differently > > for these two, and from the code it looks like this difference is rather > > accidental than intentional. > > It was probably accidental in the beginning, but it's now a well-known > misfeature that we can't get rid of for compatibility reasons. People > rely on devices with colons being accessible this way. (We once changed > that, but had to take this part back) OK, then I think what is needed is to update the documentation then. I was not able to find any restrictions on the file names, hence this patch. I think what is needed is to actually add a new 'file:' protocol, that can open paths containing no matter what characters they contain (as long as the filesystem supports them, of course); as the current situation where files have no protocol but all the other do seems asymmetric. thanks, iustin
Am 17.08.2012 12:05, schrieb Iustin Pop: > On Fri, Aug 17, 2012 at 09:56:35AM +0200, Kevin Wolf wrote: >> Am 17.08.2012 09:15, schrieb Iustin Pop: >>> On Thu, Aug 16, 2012 at 11:24:11PM +0400, Michael Tokarev wrote: >>>> On 16.08.2012 18:58, Iustin Pop wrote: >>>>> Commit 947995c (block: protect path_has_protocol from filenames with >>>>> colons) introduced a way to handle filenames with colons based on >>>>> whether the path contains a slash or not. IMHO this is not optimal, >>>>> since we shouldn't rely on the contents of the path but rather on >>>>> whether the given path exists as a file or not. >>>>> >>>>> As such, this patch tries to handle both files with and without >>>>> slashes by falling back to opening them as files if no drivers >>>>> supporting the protocol has been identified. >>>> >>>> I for one dislike this idea entirely: I think there should be a >>>> way to stop qemu from trying to open something as a file. It >>>> opens a security hole after all, "what if" such a file will actually >>>> exist? >>> >>> I'm not sure I understand the concern here. You pass what is a file path >>> (and not an existing protocol path), and you want qemu not to open it? >>> Or are you worried that a typo in the protocol name can lead to attacks? >> >> That, or just the fact that you don't know exactly which protocols are >> supported by this qemu binary and expect one that isn't there. >> >> The other concern I have is about the opposite direction: When a new >> qemu version introduces a new protocol, the same string that meant a >> file before would start meaning the protocol, which makes it an >> incompatible change. Essentially, that would mean that we can't add any >> new protocols any more. Doesn't sound like a great idea to me. > > So in this sense, you prefer to have it remain so that files with colons > are not accepted by qemu, if I understand you correctly, so that new > procotol can be added, right? The current rule is that protocols never contain slashes (and we won't add such protocols), and therefore anything containing a slash is a file name. I already felt a bit uncomfortable with that, but at least it introduces no ambiguity. You're implying that you can't use files that contain a colon. That's not true because you can include a slash in every file name. Instead of "foo:bar" write "./foo:bar" and you get it interpreted as you wanted. >>>> If I can vote, I'm voting against this with both hands. >>> >>> It's fine to have a way to stop QEMU opening something as a file, but >>> please tell me how I can make it so that "qemu -hda x:0" works for both >>> regular files and block/char devices. Right now, it behaves differently >>> for these two, and from the code it looks like this difference is rather >>> accidental than intentional. >> >> It was probably accidental in the beginning, but it's now a well-known >> misfeature that we can't get rid of for compatibility reasons. People >> rely on devices with colons being accessible this way. (We once changed >> that, but had to take this part back) > > OK, then I think what is needed is to update the documentation then. > I was not able to find any restrictions on the file names, hence this > patch. Sure, that makes sense. Care to send a patch? > I think what is needed is to actually add a new 'file:' protocol, that > can open paths containing no matter what characters they contain (as > long as the filesystem supports them, of course); as the current > situation where files have no protocol but all the other do seems > asymmetric. This is what I've been advocating before commit 947995c, but I think other people had concerns (that I can't remember now). Kevin
On Fri, Aug 17, 2012 at 12:15:41PM +0200, Kevin Wolf wrote: > Am 17.08.2012 12:05, schrieb Iustin Pop: > > On Fri, Aug 17, 2012 at 09:56:35AM +0200, Kevin Wolf wrote: > >> Am 17.08.2012 09:15, schrieb Iustin Pop: > >>> On Thu, Aug 16, 2012 at 11:24:11PM +0400, Michael Tokarev wrote: > >>>> On 16.08.2012 18:58, Iustin Pop wrote: > >>>>> Commit 947995c (block: protect path_has_protocol from filenames with > >>>>> colons) introduced a way to handle filenames with colons based on > >>>>> whether the path contains a slash or not. IMHO this is not optimal, > >>>>> since we shouldn't rely on the contents of the path but rather on > >>>>> whether the given path exists as a file or not. > >>>>> > >>>>> As such, this patch tries to handle both files with and without > >>>>> slashes by falling back to opening them as files if no drivers > >>>>> supporting the protocol has been identified. > >>>> > >>>> I for one dislike this idea entirely: I think there should be a > >>>> way to stop qemu from trying to open something as a file. It > >>>> opens a security hole after all, "what if" such a file will actually > >>>> exist? > >>> > >>> I'm not sure I understand the concern here. You pass what is a file path > >>> (and not an existing protocol path), and you want qemu not to open it? > >>> Or are you worried that a typo in the protocol name can lead to attacks? > >> > >> That, or just the fact that you don't know exactly which protocols are > >> supported by this qemu binary and expect one that isn't there. > >> > >> The other concern I have is about the opposite direction: When a new > >> qemu version introduces a new protocol, the same string that meant a > >> file before would start meaning the protocol, which makes it an > >> incompatible change. Essentially, that would mean that we can't add any > >> new protocols any more. Doesn't sound like a great idea to me. > > > > So in this sense, you prefer to have it remain so that files with colons > > are not accepted by qemu, if I understand you correctly, so that new > > procotol can be added, right? > > The current rule is that protocols never contain slashes (and we won't > add such protocols), and therefore anything containing a slash is a file > name. I already felt a bit uncomfortable with that, but at least it > introduces no ambiguity. OK, that makes sense. > You're implying that you can't use files that contain a colon. That's > not true because you can include a slash in every file name. Instead of > "foo:bar" write "./foo:bar" and you get it interpreted as you wanted. Ah no, I know that. What bothered me was the difference between regular files and devices, which caused a lot of head scratching for us in Ganeti (with qemu 1.0) in understanding why some people reported bugs but we couldn't reproduce it. With qemu 1.1 it finally works for Ganeti (since we pass absolute paths always), but I thought to make the difference go away. > >>>> If I can vote, I'm voting against this with both hands. > >>> > >>> It's fine to have a way to stop QEMU opening something as a file, but > >>> please tell me how I can make it so that "qemu -hda x:0" works for both > >>> regular files and block/char devices. Right now, it behaves differently > >>> for these two, and from the code it looks like this difference is rather > >>> accidental than intentional. > >> > >> It was probably accidental in the beginning, but it's now a well-known > >> misfeature that we can't get rid of for compatibility reasons. People > >> rely on devices with colons being accessible this way. (We once changed > >> that, but had to take this part back) > > > > OK, then I think what is needed is to update the documentation then. > > I was not able to find any restrictions on the file names, hence this > > patch. > > Sure, that makes sense. Care to send a patch? Will try to, yes. > > I think what is needed is to actually add a new 'file:' protocol, that > > can open paths containing no matter what characters they contain (as > > long as the filesystem supports them, of course); as the current > > situation where files have no protocol but all the other do seems > > asymmetric. > > This is what I've been advocating before commit 947995c, but I think > other people had concerns (that I can't remember now). Ack. Thanks for the explanations. iustin
diff --git a/block.c b/block.c index 470bdcc..367879f 100644 --- a/block.c +++ b/block.c @@ -499,7 +499,12 @@ BlockDriver *bdrv_find_protocol(const char *filename) return drv1; } } - return NULL; + /* + * No bdrv_driver identified the protocol we extracted from the + * path; maybe this is not actually a protocol, but just a file + * containing a colon? Fallback to bdrv_find_format. + */ + return bdrv_find_format("file"); } static int find_image_format(const char *filename, BlockDriver **pdrv)
Commit 947995c (block: protect path_has_protocol from filenames with colons) introduced a way to handle filenames with colons based on whether the path contains a slash or not. IMHO this is not optimal, since we shouldn't rely on the contents of the path but rather on whether the given path exists as a file or not. As such, this patch tries to handle both files with and without slashes by falling back to opening them as files if no drivers supporting the protocol has been identified. Signed-off-by: Iustin Pop <iusty@k1024.org> --- I've tested this with both slash-containing and slash-free names and it works as expected. I believe falling back to file is safe, as the "file" block driver will just fail and return NULL if the path doesn't exist. Side note: this is the third way that a file can be opened, and as such the code flow is quite unclear; first, paths pointing to block or char devices are opened in any case, even if they have colons, since the call to find_hdev_driver will probe for the and the raw drivers will identify them; second, slash-containing paths will be opened via the first call to bdrv_find_format("file") since path_has_protocol will return false; and finally, slash-free paths will be opened in this second call to said function. I don't know if all this could be cleaned and organised better. block.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)