[ovs-dev] [PATCH] netlink: added check to prevent netlink attribute overflow

[Ben Pfaff]

On Mon, Feb 11, 2019 at 04:23:57PM -0800, Toms Atteka wrote:
> If enough large input is passed to odp_actions_from_string it can
> cause netlink attribute to overflow.
> ovs_assert was added just before the problematic code so it could
> be debugged faster in similar cases if they would arise. Check
> for buffer size was added to prevent entering this function and
> returning appropriate error code.
> 
> Basic manual testing was performed.
> 
> Reported-by:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231
> Signed-off-by: Toms Atteka <cpp.code.lv at gmail.com>

Thank you for the fix!

An nlattr's size is a uint16_t.  I am more comfortable using UINT16_MAX
as the maximum value for this type than I am USHRT_MAX, since the latter
can in theory be bigger than 16 bits.

I am not sure why the assertion in nl_msg_end_nested() is sure to be
correct.  It seems risky to me.  Can you explain?

Thanks,

Ben.

> ---
>  lib/netlink.c  | 1 +
>  lib/odp-util.c | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/lib/netlink.c b/lib/netlink.c
> index de3ebcd..c91c868 100644
> --- a/lib/netlink.c
> +++ b/lib/netlink.c
> @@ -498,6 +498,7 @@ void
>  nl_msg_end_nested(struct ofpbuf *msg, size_t offset)
>  {
>      struct nlattr *attr = ofpbuf_at_assert(msg, offset, sizeof *attr);
> +    ovs_assert(msg->size - offset <= USHRT_MAX);
>      attr->nla_len = msg->size - offset;
>  }
>  
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index e893f46..9f637ca 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names,
>          n += retval;
>      }
>  
> +    if (actions->size > USHRT_MAX) {
> +        return -EFBIG;
> +    }
> +
>      return n;
>  }
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev