netdev - Re: [PATCH net-next V4 03/13] bridge: Validate that vlan is permitted on ingress

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20121221064806.330c6e4d.shmulik.ladkani@gmail.com>
Date:	Fri, 21 Dec 2012 06:48:06 +0200
From:	Shmulik Ladkani <shmulik.ladkani@...il.com>
To:	vyasevic@...hat.com
Cc:	netdev@...r.kernel.org, shemminger@...tta.com, davem@...emloft.net,
	or.gerlitz@...il.com, jhs@...atatu.com, mst@...hat.com,
	erdnetdev@...il.com, jiri@...nulli.us
Subject: Re: [PATCH net-next V4 03/13] bridge: Validate that vlan is
 permitted on ingress

On Thu, 20 Dec 2012 10:41:28 -0500 Vlad Yasevich <vyasevic@...hat.com> wrote:
> >> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb)
> >> +{
> >> +	struct net_port_vlan *pve;
> >> +	u16 vid;
> >> +
> >> +	/* If there are no vlan in the permitted list, all packets are
> >> +	 * permitted.
> >> +	 */
> >> +	if (list_empty(&p->vlan_list))
> >> +		return true;
> >> +	if (!br_allowed_ingress(p, skb))
> >> +		goto drop;
> >> +
> >
> > This condition should be also encorporated upon "ingress" at the "bridge
> > master port" (that is, early at br_dev_xmit).
> > Think of the "bridge master port" as yet another port:
> > upon "ingress" (meaning, tx packets from the ip stack), we should
> > also enforce any ingress permission rules.
> >
> 
> I've tried that before and now can't think of a reason why I rejected 
> it.  I'll try to remember...

(Current code does not allow you to put the test into 'br_dev_xmit',
since br_allow_ingress expects an 'nbp' as its 1st argument, and the
"master port" is not represented as an 'nbp')

But this is a must. The "master port" shouldn't be treated differently
with respect to vlan ingress/egress membership verifications.

Take for example why egress membership verification is important:

     m
     |
+----+----+
|         |   
p1        p2

p1 expects "normal" (untagged) traffic, and should be connected with the
IP stack (master port 'm'), but not leak to p2.
Traffic of some specific service, marked with VLAN 2, arrives at p2 and
should be forwardable with p1 (keeping the tag), but should not go to
the IP stack.

p1 pvid: 1
m  pvid: 1
vlan 1 membership map:     p1 m
vlan 1 egress policy map:  U  U
vlan 2 membership map:     p1    p2
vlan 2 egress policy map:  T     T

If you don't verify membership upon egress towards IP stack (at the
"master port"), VLAN 2 packets from p2 will leak into the stack.

Regards,
Shmulik
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html