2017-07-28 15:12:46

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH] Cipso: cipso_v4_optptr enter infinite loop

On Fri, Jul 28, 2017 at 7:30 AM, Yujuan Qi <[email protected]> wrote:
> From: "yujuan.qi" <[email protected]>
>
> in for(),if((optlen > 0) && (optptr[1] == 0)), enter infinite loop.
>
> Test: receive a packet which the ip length > 20 and the first byte of ip option is 0, produce this issue
>
> Signed-off-by: yujuan.qi <[email protected]>
> ---
> net/ipv4/cipso_ipv4.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index ae20616..b3d97c7 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1528,6 +1528,8 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
> taglen = optptr[1];
> optlen -= taglen;
> optptr += taglen;
> + if (!taglen)
> + break;
> }
>
> return NULL;

Thanks for catching this and submitting a patch. Unfortunately
checking taglen/optptr[1] isn't going to help with the NOP and EOL
options as they are single byte options. I think a for-loop like the
following, or something similar, is probably the better solution:

for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 0; ) {
switch (optptr[0]) {
case IPOPT_CIPSO:
return optptr;
case IPOPT_END:
return NULL;
case IPOPT_NOOP:
taglen = 1;
break;
default:
taglen = optptr[1];
}
optlen -= taglen;
optptr += taglen;
}

--
paul moore
security @ redhat