From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50107) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QOrks-0002tp-V5 for qemu-devel@nongnu.org; Tue, 24 May 2011 09:34:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QOrkr-0007Fk-Q9 for qemu-devel@nongnu.org; Tue, 24 May 2011 09:34:38 -0400 Received: from david.siemens.de ([192.35.17.14]:15430) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QOrkr-0007FB-HL for qemu-devel@nongnu.org; Tue, 24 May 2011 09:34:37 -0400 Message-ID: <4DDBB3E9.8000700@siemens.com> Date: Tue, 24 May 2011 15:34:33 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20110524123721.GS28399@redhat.com> <4DDBA7CF.5090400@siemens.com> <20110524130147.GT28399@redhat.com> In-Reply-To: <20110524130147.GT28399@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov Cc: "qemu-devel@nongnu.org" On 2011-05-24 15:01, Gleb Natapov wrote: > On Tue, May 24, 2011 at 02:42:55PM +0200, Jan Kiszka wrote: >> On 2011-05-24 14:37, Gleb Natapov wrote: >>> On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote: >>>> This aligns the code to what the documentation claims: Allow everything >>>> but requests that would have to be routed outside of the virtual LAN. >>>> >>>> So we need to drop the unneeded IP-level filter, allow TFTP requests, >>>> and add the missing protocol-level filter to ICMP. >>>> >>> May be I am missing something, but how do you disallow requests by >>> removing code that actually does filtering. >> >> All we need to filter are the per-IP-protocol parts that do the >> forwarding via the host IP stack. That does not need to happen at IP level. >> >> Moreover, the existing code contained some practically dead bits anyway: >> >> if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) == >> slirp->vnetwork_addr.s_addr) { >> if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p != >> IPPROTO_UDP) >> goto bad; >> >> This could only trigger if vnetwork_mask.s_addr was 0 (the same applied >> to the original code before my refactoring in 2009). >> > Not sure what do you mean by that. This checks that the ip_dst.s_addr is in > the vnetwork range. It does this by comparing net mask bits of ip_dst.s_addr with > vnetwork_addr.s_addr. Grep for vnetwork_mask.s_addr. This idiom is used > many times throughout the code. Ok, it's a bit more tricky, and I contributed some buglet. Let ip_dst.s_addr = 255.255.255.255 vnetwork_mask.s_addr = 0.255.255.255 vnetwork_addr.s_addr = 10.0.2.0 (QEMU's strange defaults) then dst & vnetwork_mask != vnetwork_addr, so the second condition to exclude network broadcasts can't trigger. Your original code matched the first three bytes of dst against the first three of vnetwork_addr, mine inverted the mask. However, both variants fail to let DHCP broadcasts pass. In short, this was always wrong and unneeded as we can (and partly already did) check for restricted mode in the various IP protocols. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux