[ovs-dev] [PATCHv2] lib: added check to prevent int overflow

[Yifeng Sun]

Looks good to me, thanks.

Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>

On Wed, Mar 20, 2019 at 1:43 PM Toms Atteka <cpp.code.lv at gmail.com> wrote:
>
> If enough large input is given ofpact_finish will fail.
> Implemented ofpbuf_oversized function to check for oversized
> buffer. Checks were added for parse functions and error messages
> returned.
>
> Basic manual testing performed.
>
> Reported-by:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12972
> Signed-off-by: Toms Atteka <cpp.code.lv at gmail.com>
>
> v1->v2: added over sized check as a separate function and added
> checks for other parse functions where they might fail.
> ---
>  include/openvswitch/ofpbuf.h |  6 ++++++
>  lib/bundle.c                 |  5 +++++
>  lib/learn.c                  |  5 +++++
>  lib/ofp-actions.c            | 29 +++++++++++++++++++++++++++++
>  4 files changed, 45 insertions(+)
>
> diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h
> index e4cf088..1136ba0 100644
> --- a/include/openvswitch/ofpbuf.h
> +++ b/include/openvswitch/ofpbuf.h
> @@ -162,6 +162,7 @@ char *ofpbuf_to_string(const struct ofpbuf *, size_t maxbytes);
>  static inline struct ofpbuf *ofpbuf_from_list(const struct ovs_list *);
>  void ofpbuf_list_delete(struct ovs_list *);
>  static inline bool ofpbuf_equal(const struct ofpbuf *, const struct ofpbuf *);
> +static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts);
>
>
>  /* Frees memory that 'b' points to, as well as 'b' itself. */
> @@ -272,6 +273,11 @@ static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b)
>             memcmp(a->data, b->data, a->size) == 0;
>  }
>
> +static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts)
> +{
> +    return (char *)ofpbuf_tail(ofpacts) - (char *)ofpacts->header > UINT16_MAX;
> +}
> +
>  #ifdef  __cplusplus
>  }
>  #endif
> diff --git a/lib/bundle.c b/lib/bundle.c
> index 558e890..edb11f6 100644
> --- a/lib/bundle.c
> +++ b/lib/bundle.c
> @@ -183,6 +183,11 @@ bundle_parse__(const char *s, const struct ofputil_port_map *port_map,
>          bundle = ofpacts->header;
>          bundle->n_slaves++;
>      }
> +
> +    if (ofpbuf_oversized(ofpacts)) {
> +        return xasprintf("input too big");
> +    }
> +
>      ofpact_finish_BUNDLE(ofpacts, &bundle);
>      bundle->basis = atoi(basis);
>
> diff --git a/lib/learn.c b/lib/learn.c
> index 642ce18..a40209e 100644
> --- a/lib/learn.c
> +++ b/lib/learn.c
> @@ -455,6 +455,11 @@ learn_parse__(char *orig, char *arg, const struct ofputil_port_map *port_map,
>              learn = ofpacts->header;
>          }
>      }
> +
> +    if (ofpbuf_oversized(ofpacts)) {
> +        return xasprintf("input too big");
> +    }
> +
>      ofpact_finish_LEARN(ofpacts, &learn);
>
>      return NULL;
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index e8e88ac..1340614 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -989,6 +989,11 @@ parse_CONTROLLER(char *arg, const struct ofpact_parse_params *pp)
>              controller = pp->ofpacts->header;
>              controller->userdata_len = userdata_len;
>          }
> +
> +        if (ofpbuf_oversized(pp->ofpacts)) {
> +            return xasprintf("input too big");
> +        }
> +
>          ofpact_finish_CONTROLLER(pp->ofpacts, &controller);
>      }
>
> @@ -3690,6 +3695,11 @@ parse_DEC_TTL(char *arg, const struct ofpact_parse_params *pp)
>              return xstrdup("dec_ttl_cnt_ids: expected at least one controller "
>                             "id.");
>          }
> +
> +        if (ofpbuf_oversized(pp->ofpacts)) {
> +            return xasprintf("input too big");
> +        }
> +
>          ofpact_finish_DEC_TTL(pp->ofpacts, &ids);
>      }
>      return NULL;
> @@ -4443,6 +4453,11 @@ parse_ENCAP(char *arg, const struct ofpact_parse_params *pp)
>      /* ofpbuf may have been re-allocated. */
>      encap = pp->ofpacts->header;
>      encap->n_props = n_props;
> +
> +    if (ofpbuf_oversized(pp->ofpacts)) {
> +        return xasprintf("input too big");
> +    }
> +
>      ofpact_finish_ENCAP(pp->ofpacts, &encap);
>      return NULL;
>  }
> @@ -5772,6 +5787,11 @@ parse_NOTE(const char *arg, const struct ofpact_parse_params *pp)
>      struct ofpact_note *note = ofpbuf_at_assert(pp->ofpacts, start_ofs,
>                                                  sizeof *note);
>      note->length = pp->ofpacts->size - (start_ofs + sizeof *note);
> +
> +    if (ofpbuf_oversized(pp->ofpacts)) {
> +        return xasprintf("input too big");
> +    }
> +
>      ofpact_finish_NOTE(pp->ofpacts, &note);
>      return NULL;
>  }
> @@ -5929,6 +5949,10 @@ parse_CLONE(char *arg, const struct ofpact_parse_params *pp)
>      pp->ofpacts->header = ofpbuf_push_uninit(pp->ofpacts, sizeof *clone);
>      clone = pp->ofpacts->header;
>
> +    if (ofpbuf_oversized(pp->ofpacts)) {
> +        return xasprintf("input too big");
> +    }
> +
>      ofpact_finish_CLONE(pp->ofpacts, &clone);
>      ofpbuf_push_uninit(pp->ofpacts, clone_offset);
>      return error;
> @@ -6615,6 +6639,11 @@ parse_CT(char *arg, const struct ofpact_parse_params *pp)
>      if (!error && oc->flags & NX_CT_F_FORCE && !(oc->flags & NX_CT_F_COMMIT)) {
>          error = xasprintf("\"force\" flag requires \"commit\" flag.");
>      }
> +
> +    if (ofpbuf_oversized(pp->ofpacts)) {
> +        return xasprintf("input too big");
> +    }
> +
>      ofpact_finish_CT(pp->ofpacts, &oc);
>      ofpbuf_push_uninit(pp->ofpacts, ct_offset);
>      return error;
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev