netdev - Re: [PATCH v2 net] net/ipv4: defensive cipso option parsing

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhQ3FwOCcQmPcMOhXmqU=W6BQY-cXJbAu27Sy9=sjBc5tQ@mail.gmail.com>
Date:   Mon, 17 Sep 2018 16:31:12 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     snu@...zon.com
Cc:     netdev@...r.kernel.org, aams@...zon.de, dwmw@...zon.co.uk,
        yujuan.qi@...iatek.com, sveith@...zon.de, stable@...r.kernel.org
Subject: Re: [PATCH v2 net] net/ipv4: defensive cipso option parsing

On Mon, Sep 17, 2018 at 1:49 PM Stefan Nuernberger <snu@...zon.com> wrote:
> commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
> a possible infinite loop in the IP option parsing of CIPSO. The fix
> assumes that ip_options_compile filtered out all zero length options and
> that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
> While this assumption currently holds true, add explicit checks for zero
> length and invalid length options to be safe for the future. Even though
> ip_options_compile should have validated the options, the introduction of
> new one-byte options can still confuse this code without the additional
> checks.
>
> Signed-off-by: Stefan Nuernberger <snu@...zon.com>
> Cc: David Woodhouse <dwmw@...zon.co.uk>
> Cc: Simon Veith <sveith@...zon.de>
> Cc: stable@...r.kernel.org
> ---
>  net/ipv4/cipso_ipv4.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

See my previous comments about the necessity of this patch, but beyond
that it looks fine to me.

Acked-by: Paul Moore <paul@...l-moore.com>

> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 82178cc69c96..777fa3b7fb13 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1512,7 +1512,7 @@ static int cipso_v4_parsetag_loc(const struct cipso_v4_doi *doi_def,
>   *
>   * Description:
>   * Parse the packet's IP header looking for a CIPSO option.  Returns a pointer
> - * to the start of the CIPSO option on success, NULL if one if not found.
> + * to the start of the CIPSO option on success, NULL if one is not found.
>   *
>   */
>  unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
> @@ -1522,10 +1522,8 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
>         int optlen;
>         int taglen;
>
> -       for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 0; ) {
> +       for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 1; ) {

Not worth re-spinning this patch, but looking at this a bit closer, we
could probably optimize the "optlen > 1" tweak a bit further by using
CIPSO_V4_HDR_LEN instead of "1" since we only care about CIPSO headers
here.

Although given the nature of IPv4 options, I'm not sure this would
ever really have an impact, let alone a noticeable impact.

>                 switch (optptr[0]) {
> -               case IPOPT_CIPSO:
> -                       return optptr;
>                 case IPOPT_END:
>                         return NULL;
>                 case IPOPT_NOOP:
> @@ -1534,6 +1532,11 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
>                 default:
>                         taglen = optptr[1];
>                 }
> +               if (!taglen || taglen > optlen)
> +                       return NULL;
> +               if (optptr[0] == IPOPT_CIPSO)
> +                       return optptr;
> +
>                 optlen -= taglen;
>                 optptr += taglen;
>         }
> --
> 2.19.0

-- 
paul moore
www.paul-moore.com