Re: [PATCH] selinux: don't label bridged packets in selinux_ip_forward()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] selinux: NetLabel bypass for routed bridge packets in selinux_ip_forward()
--

commit aa9d29a0c0e705f6d7d92b6c1e12ec004af2b214
Author: Joas Antonio dos Santos <joasantonio108@gmail.com>

selinux: don't label bridged packets in selinux_ip_forward()

This commit avoids labeling bridged IP packets in selinux_ip_forward() to
prevent MAC and EtherType corruption when adding or removing CIPSO or
CALIPSO options. It adds nf_bridge_info_exists() to skip bridged packets
since labeling based on IP addresses is unwanted there.

Link: https://lore.kernel.org/netdev/CAHC9VhS+bdL_YnLQtd0sPmKKfy9BvS-LWV_6EhAuu4qfUFe_mw@mail.gmail.com/

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 18dd28b2bb13d..735e4889bd5b7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
[ ... ]
> @@ -6021,11 +6022,13 @@ static unsigned int selinux_ip_forward(void *priv, struct sk_buff *skb,
>  				 SECCLASS_PACKET, PACKET__FORWARD_IN, &ad))
>  			return NF_DROP;
>  
> -	if (netlbl_enabled())
> -		/* we do this in the FORWARD path and not the POST_ROUTING
> -		 * path because we want to make sure we apply the necessary
> -		 * labeling before IPsec is applied so we can leverage AH
> -		 * protection */
> +	/* we do this in the FORWARD path and not the POST_ROUTING path
> +	 * because we want to make sure we apply the necessary labeling
> +	 * before IPsec is applied so we can leverage AH protection; bridged
> +	 * packets are skipped as changing their IP options would leave the
> +	 * Ethernet header behind
> +	 */
> +	if (netlbl_enabled() && !nf_bridge_info_exists(skb))
>  		if (selinux_netlbl_skbuff_setsid(skb, family, peer_sid) != 0)
>  			return NF_DROP;

[Severity: High]
Does this logic inadvertently skip NetLabel tagging for packets that are
legitimately routed rather than just bridged?

If a packet arrives on a bridge port, receives the SKB_EXT_BRIDGE_NF extension
in br_nf_pre_routing(), and is destined for the host's MAC address, it is
passed up to the local IP stack via br_pass_frame_up().

Since the bridge extension is not freed on the local input path, the packet
retains it.

When the packet is subsequently L3 forwarded via ip_forward(), the
NF_INET_FORWARD hook calls selinux_ip_forward().

Because nf_bridge_info_exists(skb) evaluates to true for these routed packets,
it appears SELinux will erroneously skip the NetLabel tagging, potentially
allowing a remote or local unprivileged attacker to bypass network security
policies.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179216146138.25460.7866279004288174372@gmail.com?part=1