From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54289) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bandA-0005wS-99 for qemu-devel@nongnu.org; Fri, 19 Aug 2016 13:30:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1band6-0006a5-2K for qemu-devel@nongnu.org; Fri, 19 Aug 2016 13:30:55 -0400 Received: from 18.mo1.mail-out.ovh.net ([46.105.35.72]:49021) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1band5-0006a1-Nl for qemu-devel@nongnu.org; Fri, 19 Aug 2016 13:30:51 -0400 Received: from player726.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo1.mail-out.ovh.net (Postfix) with ESMTP id C026FFF9A85 for ; Fri, 19 Aug 2016 19:30:50 +0200 (CEST) Date: Fri, 19 Aug 2016 19:30:42 +0200 From: Greg Kurz Message-ID: <20160819193042.215616b5@bahia.lan> In-Reply-To: References: <1470892391-4917-1-git-send-email-ppandit@redhat.com> <20160819183717.4de8c99d@bahia.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] 9pfs: add check for relative path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: P J P , Qemu Developers , Prasad J Pandit , "Michael S. Tsirkin" , Felix Wilhelm , "Aneesh Kumar K.V" On Fri, 19 Aug 2016 18:03:29 +0100 Peter Maydell wrote: > On 19 August 2016 at 17:37, Greg Kurz wrote: > > Peter Maydell wrote: > >> If (1) is true and "only single path component" is a protocol > >> requirement then probably we should be enforcing this at a > >> higher layer than in 9p-local.c, ie in hw/9pfs/cofs.c. > > > As we discussed on IRC, the / character isn't invalid per-se. It raises > > issues with the local backend on a linux host but does not do harm with > > other backends. > > > > The proxy backend also accesses the linux filesystem but since it > > chroots to the export path, it does not hit the path traversal issue. > > The proxy backend is not actually going to do the right thing with > a component name containing a '/' though (which would be to really > treat it as a filename or whatever with a '/', not to mis-interpret > it as a combined directory-and-filename. For instance opening "foo/bar" > ought to open a file named "foo/bar", not a file bar in directory foo, > if we're going to accept it.) It might not be a security hole, but > it still doesn't actually support '/' in filenames. > > The handle backend also assumes '/' isn't in filenames. > > 'synth' might be able to handle '/' I guess, but I'd want to > audit the code before I put any weight on that assertion. > > I don't really see the point in allowing a theoretical > /-in-names-aware backend to interact with an equally theoretical > /-in-names-aware frontend: nobody in practice is going to > use this. The downside of support in the middle-layer code for > this theoretical case is that we make it harder to write correct > backends and easy to accidentally allow security holes. > I'd prefer it if we made the check in the middle layer and > explicitly said "all QEMU 9p servers insist that '/' is not a > valid character in filenames, and backend code can assume that > the middle layer has validated this". > > thanks > -- PMM Maybe the check can even be made in the top layer then. I should spend more time to see which is best. My main concern now is that, unlike I said on IRC, I'm afraid I won't be able to work on this before next Wednesday... :-\ Hope it is not too late for 2.7... Cheers. -- Greg