Message ID | 20180815220343.25909-1-blp@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev] ofp-ed-props: Fix hang for crafted OpenFlow encap/decap properties. | expand |
On Wed, Aug 15, 2018 at 3:03 PM, Ben Pfaff <blp@ovn.org> wrote: > decode_ed_prop() accepted encap/decap properties with a reported length of > 0, without consuming any data from the property list, which yielded an > infinite loop. > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9918 > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > lib/ofp-ed-props.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c > index 901da2f0dd1b..28382e01235c 100644 > --- a/lib/ofp-ed-props.c > +++ b/lib/ofp-ed-props.c > @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header > **ofp_prop, size_t len = (*ofp_prop)->len; > size_t pad_len = ROUND_UP(len, 8); > > - if (pad_len > *remaining) { > + if (len < sizeof **ofp_prop || pad_len > *remaining) { > Is *remaining > pad_len valid ? If it is, which is not intuitive, maybe a comment will help ? > return OFPERR_OFPBAC_BAD_LEN; > } > > -- > 2.16.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Aug 15, 2018 at 04:55:38PM -0700, Darrell Ball wrote: > On Wed, Aug 15, 2018 at 3:03 PM, Ben Pfaff <blp@ovn.org> wrote: > > > decode_ed_prop() accepted encap/decap properties with a reported length of > > 0, without consuming any data from the property list, which yielded an > > infinite loop. > > > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9918 > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > lib/ofp-ed-props.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c > > index 901da2f0dd1b..28382e01235c 100644 > > --- a/lib/ofp-ed-props.c > > +++ b/lib/ofp-ed-props.c > > @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header > > **ofp_prop, > > size_t len = (*ofp_prop)->len; > > size_t pad_len = ROUND_UP(len, 8); > > > > - if (pad_len > *remaining) { > > + if (len < sizeof **ofp_prop || pad_len > *remaining) { > > > > Is *remaining > pad_len valid ? > If it is, which is not intuitive, maybe a comment will help ? Can you help me understand why it would not be valid?
On Thu, Aug 16, 2018 at 8:51 AM, Ben Pfaff <blp@ovn.org> wrote: > On Wed, Aug 15, 2018 at 04:55:38PM -0700, Darrell Ball wrote: > > On Wed, Aug 15, 2018 at 3:03 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > decode_ed_prop() accepted encap/decap properties with a reported > length of > > > 0, without consuming any data from the property list, which yielded an > > > infinite loop. > > > > > > Reported-at: https://bugs.chromium.org/p/os > s-fuzz/issues/detail?id=9918 > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > --- > > > lib/ofp-ed-props.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c > > > index 901da2f0dd1b..28382e01235c 100644 > > > --- a/lib/ofp-ed-props.c > > > +++ b/lib/ofp-ed-props.c > > > @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header > > > **ofp_prop, > > > > size_t len = (*ofp_prop)->len; > > > size_t pad_len = ROUND_UP(len, 8); > > > > > > - if (pad_len > *remaining) { > > > + if (len < sizeof **ofp_prop || pad_len > *remaining) { > > > > > > > Is *remaining > pad_len valid ? > > If it is, which is not intuitive, maybe a comment will help ? > > Can you help me understand why it would not be valid? > I noted that 'len' here is for a single property vs remaining for all properties; I had somehow thought that the caller, decode_NXAST_RAW_ENCAP(), would verify that the remaining length is sufficient before calling decode_ed_prop() since it is already verifying the overall length - remaining, 'remaining' > 0, meaning it handles the list of properties. Below explains better what I had expected, but I still think the existing code and your incremental is better, hence Acked-by: Darrell Ball<dlu998@gmail.com> diff --git a/include/openvswitch/ofp-ed-props.h b/include/openvswitch/ofp-ed-props.h index 306c6fe..e742d69 100644 --- a/include/openvswitch/ofp-ed-props.h +++ b/include/openvswitch/ofp-ed-props.h @@ -97,7 +97,8 @@ struct ofpact_ed_prop_nsh_tlv { uint8_t data[0]; }; enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, - struct ofpbuf *out, size_t *remaining); + struct ofpbuf *out, size_t remaining, size_t len, + size_t pad_len); enum ofperr encode_ed_prop(const struct ofpact_ed_prop **prop, struct ofpbuf *out); bool parse_ed_prop_class(const char *str, uint16_t *prop_class); diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 93e212f..beca500 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -4343,11 +4343,17 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae, props_len = ntohs(nae->len) - offsetof(struct nx_action_encap, props); n_props = 0; while (props_len > 0) { - err = decode_ed_prop(&ofp_prop, out, &props_len); + size_t pad_len = ROUND_UP(ofp_prop->len, 8); + if (pad_len > props_len) { + return OFPERR_OFPBAC_BAD_LEN; + } + err = decode_ed_prop(&ofp_prop, out, props_len, ofp_prop->len, + pad_len); if (err) { return err; } n_props++; + props_len -= pad_len; } encap->n_props = n_props; out->header = &encap->ofpact; diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index 901da2f..0816659 100644 --- a/lib/ofp-ed-props.c +++ b/lib/ofp-ed-props.c @@ -28,16 +28,10 @@ enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, struct ofpbuf *out OVS_UNUSED, - size_t *remaining) + size_t remaining, size_t len, size_t pad_len) { uint16_t prop_class = ntohs((*ofp_prop)->prop_class); uint8_t prop_type = (*ofp_prop)->type; - size_t len = (*ofp_prop)->len; - size_t pad_len = ROUND_UP(len, 8); - - if (pad_len > *remaining) { - return OFPERR_OFPBAC_BAD_LEN; - } switch (prop_class) { case OFPPPC_NSH: { @@ -45,7 +39,7 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, case OFPPPT_PROP_NSH_MDTYPE: { struct ofp_ed_prop_nsh_md_type *opnmt = ALIGNED_CAST(struct ofp_ed_prop_nsh_md_type *, *ofp_prop); - if (len > sizeof(*opnmt) || len > *remaining) { + if (len > sizeof(*opnmt) || len > remaining) { return OFPERR_NXBAC_BAD_ED_PROP; } struct ofpact_ed_prop_nsh_md_type *pnmt = @@ -60,7 +54,7 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, struct ofp_ed_prop_nsh_tlv *opnt = ALIGNED_CAST(struct ofp_ed_prop_nsh_tlv *, *ofp_prop); size_t tlv_pad_len = ROUND_UP(opnt->tlv_len, 8); - if (len != sizeof(*opnt) + tlv_pad_len || len > *remaining) { + if (len != sizeof(*opnt) + tlv_pad_len || len > remaining) { return OFPERR_NXBAC_BAD_ED_PROP; } struct ofpact_ed_prop_nsh_tlv *pnt = @@ -83,7 +77,6 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, return OFPERR_NXBAC_UNKNOWN_ED_PROP; } - *remaining -= pad_len; *ofp_prop = ALIGNED_CAST(const struct ofp_ed_prop_header *, ((char *)(*ofp_prop) + pad_len)); return 0; (END)
On Thu, Aug 16, 2018 at 10:37:15AM -0700, Darrell Ball wrote: > On Thu, Aug 16, 2018 at 8:51 AM, Ben Pfaff <blp@ovn.org> wrote: > > > On Wed, Aug 15, 2018 at 04:55:38PM -0700, Darrell Ball wrote: > > > On Wed, Aug 15, 2018 at 3:03 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > decode_ed_prop() accepted encap/decap properties with a reported > > length of > > > > 0, without consuming any data from the property list, which yielded an > > > > infinite loop. > > > > > > > > Reported-at: https://bugs.chromium.org/p/os > > s-fuzz/issues/detail?id=9918 > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > > --- > > > > lib/ofp-ed-props.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c > > > > index 901da2f0dd1b..28382e01235c 100644 > > > > --- a/lib/ofp-ed-props.c > > > > +++ b/lib/ofp-ed-props.c > > > > @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header > > > > **ofp_prop, > > > > > > size_t len = (*ofp_prop)->len; > > > > size_t pad_len = ROUND_UP(len, 8); > > > > > > > > - if (pad_len > *remaining) { > > > > + if (len < sizeof **ofp_prop || pad_len > *remaining) { > > > > > > > > > > Is *remaining > pad_len valid ? > > > If it is, which is not intuitive, maybe a comment will help ? > > > > Can you help me understand why it would not be valid? > > > > > I noted that 'len' here is for a single property vs remaining for all > properties; I > had somehow thought that the caller, decode_NXAST_RAW_ENCAP(), > would verify that the remaining length is sufficient before calling > decode_ed_prop() > since it is already verifying the overall length - remaining, 'remaining' > > 0, meaning it handles the list > of properties. > > Below explains better what I had expected, but I still think the existing > code and your incremental is better, hence > > Acked-by: Darrell Ball<dlu998@gmail.com> Thanks. I applied my original fix to master. The following would make this code closer to the style we use elsewhere. I haven't tested it. diff --git a/include/openvswitch/ofp-ed-props.h b/include/openvswitch/ofp-ed-props.h index 306c6fe7302a..6a85fc0f404d 100644 --- a/include/openvswitch/ofp-ed-props.h +++ b/include/openvswitch/ofp-ed-props.h @@ -96,8 +96,7 @@ struct ofpact_ed_prop_nsh_tlv { /* tlv_len octets of metadata value, padded to a multiple of 8 bytes. */ uint8_t data[0]; }; -enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, - struct ofpbuf *out, size_t *remaining); +enum ofperr decode_ed_prop(struct ofpbuf *in, struct ofpbuf *out); enum ofperr encode_ed_prop(const struct ofpact_ed_prop **prop, struct ofpbuf *out); bool parse_ed_prop_class(const char *str, uint16_t *prop_class); diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 93e212f07196..a14fb03c6991 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -4320,13 +4320,7 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae, enum ofp_version ofp_version OVS_UNUSED, struct ofpbuf *out) { - struct ofpact_encap *encap; - const struct ofp_ed_prop_header *ofp_prop; - size_t props_len; - uint16_t n_props = 0; - int err; - - encap = ofpact_put_ENCAP(out); + struct ofpact_encap *encap = ofpact_put_ENCAP(out); encap->ofpact.raw = NXAST_RAW_ENCAP; switch (ntohl(nae->new_pkt_type)) { case PT_ETH: @@ -4339,11 +4333,11 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae, encap->new_pkt_type = nae->new_pkt_type; encap->hdr_size = ntohs(nae->hdr_size); - ofp_prop = nae->props; - props_len = ntohs(nae->len) - offsetof(struct nx_action_encap, props); - n_props = 0; - while (props_len > 0) { - err = decode_ed_prop(&ofp_prop, out, &props_len); + struct ofpbuf properties = ofpbuf_const_initializer( + nae->props, ntohs(nae->len) - offsetof(struct nx_action_encap, props)); + int n_props = 0; + while (properties.size) { + int err = decode_ed_prop(&properties, out); if (err) { return err; } diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index 28382e01235c..3d1e74ff2358 100644 --- a/lib/ofp-ed-props.c +++ b/lib/ofp-ed-props.c @@ -26,30 +26,34 @@ enum ofperr -decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, - struct ofpbuf *out OVS_UNUSED, - size_t *remaining) +decode_ed_prop(struct ofpbuf *in, struct ofpbuf *out) { - uint16_t prop_class = ntohs((*ofp_prop)->prop_class); - uint8_t prop_type = (*ofp_prop)->type; - size_t len = (*ofp_prop)->len; + const struct ofp_ed_prop_header *ofp_prop + = ofpbuf_at(in, 0, sizeof *ofp_prop); + if (!ofp_prop) { + return OFPERR_OFPBAC_BAD_LEN; + } + size_t len = ofp_prop->len; size_t pad_len = ROUND_UP(len, 8); - - if (len < sizeof **ofp_prop || pad_len > *remaining) { + if (len < sizeof *ofp_prop || pad_len > in->size) { return OFPERR_OFPBAC_BAD_LEN; } + ofpbuf_pull(in, pad_len); + + uint16_t prop_class = ntohs(ofp_prop->prop_class); + uint8_t prop_type = ofp_prop->type; switch (prop_class) { case OFPPPC_NSH: { switch (prop_type) { case OFPPPT_PROP_NSH_MDTYPE: { struct ofp_ed_prop_nsh_md_type *opnmt = - ALIGNED_CAST(struct ofp_ed_prop_nsh_md_type *, *ofp_prop); - if (len > sizeof(*opnmt) || len > *remaining) { + ALIGNED_CAST(struct ofp_ed_prop_nsh_md_type *, ofp_prop); + if (len != sizeof *opnmt) { return OFPERR_NXBAC_BAD_ED_PROP; } struct ofpact_ed_prop_nsh_md_type *pnmt = - ofpbuf_put_uninit(out, sizeof(*pnmt)); + ofpbuf_put_uninit(out, sizeof *pnmt); pnmt->header.prop_class = prop_class; pnmt->header.type = prop_type; pnmt->header.len = len; @@ -58,13 +62,13 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, } case OFPPPT_PROP_NSH_TLV: { struct ofp_ed_prop_nsh_tlv *opnt = - ALIGNED_CAST(struct ofp_ed_prop_nsh_tlv *, *ofp_prop); + ALIGNED_CAST(struct ofp_ed_prop_nsh_tlv *, ofp_prop); size_t tlv_pad_len = ROUND_UP(opnt->tlv_len, 8); - if (len != sizeof(*opnt) + tlv_pad_len || len > *remaining) { + if (len != sizeof *opnt + tlv_pad_len) { return OFPERR_NXBAC_BAD_ED_PROP; } struct ofpact_ed_prop_nsh_tlv *pnt = - ofpbuf_put_uninit(out, sizeof(*pnt)); + ofpbuf_put_uninit(out, sizeof *pnt); pnt->header.prop_class = prop_class; pnt->header.type = prop_type; pnt->header.len = len; @@ -83,9 +87,6 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, return OFPERR_NXBAC_UNKNOWN_ED_PROP; } - *remaining -= pad_len; - *ofp_prop = ALIGNED_CAST(const struct ofp_ed_prop_header *, - ((char *)(*ofp_prop) + pad_len)); return 0; }
diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index 901da2f0dd1b..28382e01235c 100644 --- a/lib/ofp-ed-props.c +++ b/lib/ofp-ed-props.c @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, size_t len = (*ofp_prop)->len; size_t pad_len = ROUND_UP(len, 8); - if (pad_len > *remaining) { + if (len < sizeof **ofp_prop || pad_len > *remaining) { return OFPERR_OFPBAC_BAD_LEN; }
decode_ed_prop() accepted encap/decap properties with a reported length of 0, without consuming any data from the property list, which yielded an infinite loop. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9918 Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/ofp-ed-props.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)