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

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