On Mon, Sep 17, 2018 at 11:12 AM Stefan Nuernberger <[email protected]> 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 <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>
> Reviewed-by: Simon Veith <[email protected]>
> Cc: [email protected]
> ---
> 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
