Re: [Qemu-devel] [PATCH] block: handle filenames with colons better

[Iustin Pop]

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