[PATCH net] net/ipv4: defensive cipso option parsing

[PATCH net] net/ipv4: defensive cipso option parsing

[Stefan Nuernberger]

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@amazon.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Simon Veith <sveith@amazon.de>
Cc: stable@vger.kernel.org
---
 net/ipv4/cipso_ipv4.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 82178cc69c96..f291b57b8474 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,9 +1522,11 @@ 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; ) {
 		switch (optptr[0]) {
 		case IPOPT_CIPSO:
+			if (!optptr[1] || optptr[1] > optlen)
+				return NULL;
 			return optptr;
 		case IPOPT_END:
 			return NULL;
@@ -1534,6 +1536,10 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
 		default:
 			taglen = optptr[1];
 		}
+
+		if (!taglen || taglen > optlen)
+			break;
+
 		optlen -= taglen;
 		optptr += taglen;
 	}
-- 
2.19.0

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

[Paul Moore]

On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger <snu@amazon.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@amazon.com>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Simon Veith <sveith@amazon.de>
> Cc: stable@vger.kernel.org
> ---
>  net/ipv4/cipso_ipv4.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 82178cc69c96..f291b57b8474 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,9 +1522,11 @@ 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; ) {
>                 switch (optptr[0]) {
>                 case IPOPT_CIPSO:
> +                       if (!optptr[1] || optptr[1] > optlen)
> +                               return NULL;
>                         return optptr;
>                 case IPOPT_END:
>                         return NULL;
> @@ -1534,6 +1536,10 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
>                 default:
>                         taglen = optptr[1];
>                 }
> +
> +               if (!taglen || taglen > optlen)
> +                       break;

I tend to think that you reach a point where you simply need to trust
that the stack is doing the right thing and that by the time you hit a
certain point you can safely assume that the packet is well formed,
but I'm not going to fight about that here.

Regardless of the above, I don't like how you're doing the option
length check twice in this code, that looks ugly to me, I think we can
do better.  How about something like this:

  for (...) {
    switch(optptr[0]) {
    case IPOPT_END:
      return NULL;
    case IPOPT_NOOP:
      taglen = 1;
    default:
      taglen = optptr[1];
    }
    if (taglen == 0 || taglen > optlen)
      return NULL;
    if (optptr[0] == IPOPT_CIPSO)
      return optptr;
    ....
  }

>                 optlen -= taglen;
>                 optptr += taglen;
>         }

-- 
paul moore
www.paul-moore.com

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

[Stefan Nuernberger]

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@amazon.com>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: Simon Veith <sveith@amazon.de>
Cc: stable@vger.kernel.org
---
 net/ipv4/cipso_ipv4.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

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; ) {
 		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

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

[Nuernberger, Stefan]

On Mon, 2018-09-17 at 12:35 -0400, Paul Moore wrote:
> On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger <snu@amazon.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@amazon.com>
> > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> > Reviewed-by: Simon Veith <sveith@amazon.de>
> > Cc: stable@vger.kernel.org
> > ---
> >  net/ipv4/cipso_ipv4.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> > index 82178cc69c96..f291b57b8474 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,9 +1522,11 @@ 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; ) {
> >                 switch (optptr[0]) {
> >                 case IPOPT_CIPSO:
> > +                       if (!optptr[1] || optptr[1] > optlen)
> > +                               return NULL;
> >                         return optptr;
> >                 case IPOPT_END:
> >                         return NULL;
> > @@ -1534,6 +1536,10 @@ unsigned char *cipso_v4_optptr(const struct
> > sk_buff *skb)
> >                 default:
> >                         taglen = optptr[1];
> >                 }
> > +
> > +               if (!taglen || taglen > optlen)
> > +                       break;
> I tend to think that you reach a point where you simply need to trust
> that the stack is doing the right thing and that by the time you hit
> a
> certain point you can safely assume that the packet is well formed,
> but I'm not going to fight about that here.
> 
> Regardless of the above, I don't like how you're doing the option
> length check twice in this code, that looks ugly to me, I think we
> can
> do better.  How about something like this:
> 
>   for (...) {
>     switch(optptr[0]) {
>     case IPOPT_END:
>       return NULL;
>     case IPOPT_NOOP:
>       taglen = 1;
>     default:
>       taglen = optptr[1];
>     }
>     if (taglen == 0 || taglen > optlen)
>       return NULL;
>     if (optptr[0] == IPOPT_CIPSO)
>       return optptr;
>     ....
>   }
> 

You're right, that looks much better. I sent around a new patch.

> > 
> >                 optlen -= taglen;
> >                 optptr += taglen;
> >         }


Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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

[Paul Moore]

On Mon, Sep 17, 2018 at 1:49 PM Stefan Nuernberger <snu@amazon.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@amazon.com>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: Simon Veith <sveith@amazon.de>
> Cc: stable@vger.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@paul-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

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

[David Miller]

From: Stefan Nuernberger <snu@amazon.com>
Date: Mon, 17 Sep 2018 19:46:53 +0200

> 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@amazon.com>

Applied to net-next.

This is not 'net' nor -stable material.  I'm hesitent about this
change as-is, and ip_options_compile() is not changing semantics in
-stable in the way that you say can cause problems.