Patchwork block: handle filenames with colons better

login
register
mail settings
Submitter Iustin Pop
Date Aug. 16, 2012, 2:58 p.m.
Message ID <20120816145836.GA6793@hq.k1024.org>
Download mbox | patch
Permalink /patch/178056/
State New
Headers show

Comments

Iustin Pop - Aug. 16, 2012, 2:58 p.m.
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(-)
Michael Tokarev - Aug. 16, 2012, 7:24 p.m.
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
Iustin Pop - Aug. 17, 2012, 7:15 a.m.
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
Kevin Wolf - Aug. 17, 2012, 7:56 a.m.
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
Iustin Pop - Aug. 17, 2012, 10:05 a.m.
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
Kevin Wolf - Aug. 17, 2012, 10:15 a.m.
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
Iustin Pop - Aug. 17, 2012, 10:39 p.m.
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

Patch

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)