netdev - Re: [PATCH ipsec] Fix XFRM-I support for nested ESP tunnels

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: <CAHsH6GubMoLBzDz=-0d5t9=HkU_iGHmH_gGNj4p4y+JzddcyVw@mail.gmail.com>
Date:   Tue, 8 Nov 2022 10:36:18 +0200
From:   Eyal Birger <eyal.birger@...il.com>
To:     Benedict Wong <benedictwong@...gle.com>
Cc:     netdev@...r.kernel.org, nharold@...gle.com, lorenzo@...gle.com
Subject: Re: [PATCH ipsec] Fix XFRM-I support for nested ESP tunnels

Hi,

On Tue, Nov 8, 2022 at 4:31 AM Benedict Wong <benedictwong@...gle.com> wrote:
>
> This change adds support for nested IPsec tunnels by ensuring that
> XFRM-I verifies existing policies before decapsulating a subsequent
> policies. Addtionally, this clears the secpath entries after policies
> are verified, ensuring that previous tunnels with no-longer-valid
> do not pollute subsequent policy checks.
>
> This is necessary especially for nested tunnels, as the IP addresses,
> protocol and ports may all change, thus not matching the previous
> policies. In order to ensure that packets match the relevant inbound
> templates, the xfrm_policy_check should be done before handing off to
> the inner XFRM protocol to decrypt and decapsulate.
>
> Notably, raw ESP/AH packets did not perform policy checks inherently,
> whereas all other encapsulated packets (UDP, TCP encapsulated) do policy
> checks after calling xfrm_input handling in the respective encapsulation
> layer.
>
> Test: Verified with additional Android Kernel Unit tests
> Signed-off-by: Benedict Wong <benedictwong@...gle.com>
> ---
>  net/xfrm/xfrm_interface.c | 54 ++++++++++++++++++++++++++++++++++++---
>  net/xfrm/xfrm_policy.c    |  5 +++-
>  2 files changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> index 5113fa0fbcee..0c41ed112081 100644
> --- a/net/xfrm/xfrm_interface.c
> +++ b/net/xfrm/xfrm_interface.c
> @@ -207,6 +207,52 @@ static void xfrmi_scrub_packet(struct sk_buff *skb, bool xnet)
>         skb->mark = 0;
>  }
>
> +static int xfrmi_input(struct sk_buff *skb, int nexthdr, __be32 spi,
> +                      int encap_type, unsigned short family)
> +{
> +       struct sec_path *sp;
> +
> +       sp = skb_sec_path(skb);
> +       if (sp && (sp->len || sp->olen) &&
> +           !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family))
> +               goto discard;
> +
> +       XFRM_SPI_SKB_CB(skb)->family = family;
> +       if (family == AF_INET) {
> +               XFRM_SPI_SKB_CB(skb)->daddroff = offsetof(struct iphdr, daddr);
> +               XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = NULL;
> +       } else {
> +               XFRM_SPI_SKB_CB(skb)->daddroff = offsetof(struct ipv6hdr, daddr);
> +               XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = NULL;
> +       }
> +
> +       return xfrm_input(skb, nexthdr, spi, encap_type);
> +discard:
> +       kfree_skb(skb);
> +       return 0;
> +}
> +
> +static int xfrmi4_rcv(struct sk_buff *skb)
> +{
> +       return xfrmi_input(skb, ip_hdr(skb)->protocol, 0, 0, AF_INET);
> +}
> +
> +static int xfrmi6_rcv(struct sk_buff *skb)
> +{
> +       return xfrmi_input(skb, skb_network_header(skb)[IP6CB(skb)->nhoff],
> +                          0, 0, AF_INET6);
> +}
> +
> +static int xfrmi4_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
> +{
> +       return xfrmi_input(skb, nexthdr, spi, encap_type, AF_INET);
> +}
> +
> +static int xfrmi6_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
> +{
> +       return xfrmi_input(skb, nexthdr, spi, encap_type, AF_INET6);
> +}
> +
>  static int xfrmi_rcv_cb(struct sk_buff *skb, int err)
>  {
>         const struct xfrm_mode *inner_mode;
> @@ -774,8 +820,8 @@ static struct pernet_operations xfrmi_net_ops = {
>  };
>
>  static struct xfrm6_protocol xfrmi_esp6_protocol __read_mostly = {
> -       .handler        =       xfrm6_rcv,
> -       .input_handler  =       xfrm_input,
> +       .handler        =       xfrmi6_rcv,
> +       .input_handler  =       xfrmi6_input,
>         .cb_handler     =       xfrmi_rcv_cb,
>         .err_handler    =       xfrmi6_err,
>         .priority       =       10,
> @@ -825,8 +871,8 @@ static struct xfrm6_tunnel xfrmi_ip6ip_handler __read_mostly = {
>  #endif
>
>  static struct xfrm4_protocol xfrmi_esp4_protocol __read_mostly = {
> -       .handler        =       xfrm4_rcv,
> -       .input_handler  =       xfrm_input,
> +       .handler        =       xfrmi4_rcv,
> +       .input_handler  =       xfrmi4_input,
>         .cb_handler     =       xfrmi_rcv_cb,
>         .err_handler    =       xfrmi4_err,
>         .priority       =       10,
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index f1a0bab920a5..04f66f6d5729 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>         int xerr_idx = -1;
>         const struct xfrm_if_cb *ifcb;
>         struct sec_path *sp;
> -       struct xfrm_if *xi;
> +       struct xfrm_if *xi = NULL;
>         u32 if_id = 0;
>
>         rcu_read_lock();
> @@ -3667,6 +3667,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>                         goto reject;
>                 }
>
> +               if (xi)
> +                       secpath_reset(skb);
> +

This is different in the current kernel. "xi" no longer exists here.
I think the equivalent check would be to see if if_id isn't zero.

Eyal.


>                 xfrm_pols_put(pols, npols);
>                 return 1;
>         }
> --
> 2.38.1.431.g37b22c650d-goog
>